Block Toolbar: Don't use effects for focus management #50497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
requested review from
draganescu,
talldan and
jasmussen
and removed request for
ellatrix
Mamaduka
added
[Type] Bug
|
Size Change: -110 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4507279. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4934117756
|
|
Thanks for the PR. Taking it for a spin, I'm not entirely sure what it fixes, apologies if I'm missing anything. For full context, the issue is somewhat complex, so I'll try and outline it:
That's the main gist, and where focus management becomes an issue. Take a use case where you type out 5 paragraphs in succession. While still having a block selected, you should be able to click the "Undo" button multiple times in succession, to undo every paragraph. But instead, this happens:
That is — once you undo the "new paragraph" action, focus is transferred from the paragraph that gets undone, to the paragraph before it. When that happens, focus is also transferred to the previous block, and so the block toolbar appears again, covering the Undo button, and preventing you from just clicking undo in succession. The reason for this is a bit nuanced. What happens is that WritingFlow handles the focus. The idea being — and this is standard in Google Docs and other editors — is that if the text caret is inside the paragraph, then even if you click the Undo button with your mouse, the text caret should remain in the text. Which is to say, focus is not transferred to the Undo button, it remains in the text. (Or more technically, it is transferred back). This default text behavior is useful for the default block toolbar behavior, but becomes a stumbling block in top toolbar mode. So the question is: without the code being hacky or diluted, is it possible to change that undo behavior for the top toolbar mode? Change it so that when you do have that mode enabled, clicking the undo (or redo) buttons also transfers focus there? Hopefully that makes sense. Thanks for diving in, Andei and George. I understand this is really difficult. |
The PR fixes a different focus issue. Sorry if the screencast weren't enough to highlight the problem. If you switch blocks, when the document tools (undo/redo/etc.) are revealed, the focus is moved to the "Show document tools" button. This is the broken behavior. Changing the block selection shouldn't re-focus the buttons, only clicking them.
I also noticed this on the trunk, and it's a different issue. Undo action selects a different block, so the fixed toolbar state is re-setted, hiding the document tools. Which is technically the exact behavior your describe here - #50485 (comment) 😅 |
draganescu
approved these changes
May 10, 2023
There was a problem hiding this comment.
Yes let's merge this, it's a much better implementation than my PR and also does some nice cleanup there. Thank you @Mamaduka 🙇🏻
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #50337.
Alternative to #50346 and #50485
PR moves the "collapse/expand" button's focus management from effect to user action.
Why?
In my opinion, managing the focus of this component through events instead of rendering side effects would be more logical, as it depends on user action.
How?
ExpandFixedToolbarButtonandCollapseFixedToolbarButtoncomponents to avoid unmounting the toolbar item based on the state.isCollapsedstate.onClickevent, and therefwill always be accessible since we're working with a single component.Testing Instructions
Screenshots or screencast
Before
CleanShot.2023-05-09.at.21.57.14.mp4
After
CleanShot.2023-05-10.at.10.45.46.mp4