Fix multiple tooltips showing on NavigableToolbars #49644
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
jeryj
changed the title
|
Size Change: -2.76 kB (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3ccbc8e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4700797082
|
|
I like this solution:
Let's move forward with this solution, 👏🏻 |
jeryj
commented
Apr 11, 2023
Comment on lines
6
to
27
| const documentToolbarButton = ( page ) => { | ||
| return page.getByRole( 'button', { | ||
| name: 'Toggle block inserter', | ||
| exact: true, | ||
| } ); | ||
| }; | ||
|
|
||
| const documentToolbarTooltip = ( page ) => { | ||
| return page.locator( 'text=Toggle block inserter' ); | ||
| }; | ||
|
|
||
| const blockToolbarButton = ( page ) => { | ||
| return page.getByRole( 'button', { name: 'Paragraph', exact: true } ); | ||
| }; | ||
|
|
||
| const useSelectMode = async ( page ) => { | ||
| await page.keyboard.press( 'Escape' ); | ||
| }; | ||
|
|
||
| const moveToToolbarShortcut = async ( pageUtils ) => { | ||
| await pageUtils.pressKeys( 'alt+F10' ); | ||
| }; |
There was a problem hiding this comment.
Even though these are mostly one-liners, I found they made the test cases below much easier to read. They behave as comments when named like this.
There was a problem hiding this comment.
It's recommended to place these inside a POM-style util. You can see some prior examples in other tests.
There was a problem hiding this comment.
Ah, nice! Thanks for the tip!
jeryj
commented
Apr 11, 2023
Comment on lines
39
to
55
| // Test: Focus the top level toolbar from title | ||
| await moveToToolbarShortcut( pageUtils ); | ||
| await expect( documentToolbarButton( page ) ).toBeFocused(); | ||
|
|
||
| // Test: Focus document toolbar from empty block | ||
| await editor.insertBlock( { name: 'core/paragraph' } ); | ||
| await moveToToolbarShortcut( pageUtils ); | ||
| await expect( documentToolbarButton( page ) ).toBeFocused(); | ||
|
|
||
| // Test: Focus block toolbar from block content when block toolbar isn't visible | ||
| await editor.insertBlock( { name: 'core/paragraph' } ); | ||
| await page.keyboard.type( | ||
| "Focus to block toolbar when block toolbar isn't visible" | ||
| ); | ||
| await moveToToolbarShortcut( pageUtils ); | ||
| await expect( blockToolbarButton( page ) ).toBeFocused(); | ||
| await expect( documentToolbarTooltip( page ) ).not.toBeVisible(); |
There was a problem hiding this comment.
Initially these were separate tests. I combined the tests into groups based into view and mode instead. This cuts the test execution time down by about half, (18s for running all the tests vs 9s for the current setup). This is recommended by Playwright Best Practices of "write fewer tests but longer tests": https://playwright.dev/docs/best-practices#write-fewer-tests-but-longer-tests. I think the time saved per commit is worth having more assertions per test.
jeryj
commented
Apr 11, 2023
| * @param this | ||
| * @param isFixed Boolean value true/false for on/off. | ||
| */ | ||
| export async function toggleFixedToolbar( this: Editor, isFixed: boolean ) { |
There was a problem hiding this comment.
This is taken from https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-tests/specs/editor/various/links.test.js#L275-L284. If the plan is to move the older e2e tests into playwright, then this will be useful.
There was a problem hiding this comment.
Maybe we should rename it to setIsFixedToolbar because toggle seems a bit weird with the isFixed parameter 🤔 .
There was a problem hiding this comment.
Yeah, that's much better to read.
jeryj
commented
Apr 11, 2023
kevin940726
reviewed
Apr 12, 2023
| @@ -147,6 +154,7 @@ function useToolbarFocus( | |||
| function NavigableToolbar( { | |||
| children, | |||
| focusOnMount, | |||
| useKeyboardFocusShortcut = true, | |||
There was a problem hiding this comment.
Could we rename this to shouldUseKeyboardFocusShortcut as seen below? use* variable often suggests a hook in React and that might cause some confusion. shouuld* also has the benefit of suggesting the variable to be a boolean too.
ajlende
reviewed
Apr 12, 2023
packages/edit-post/src/components/header/header-toolbar/index.js
Outdated
Show resolved
Hide resolved
kevin940726
reviewed
Apr 13, 2023
| * @param this | ||
| * @param setIsFixedToolbar Boolean value true/false for on/off. | ||
| */ | ||
| export async function toggleFixedToolbar( |
There was a problem hiding this comment.
Oh oops, I mean the util name, not the parameter name. isFixed is actually a more readable parameter name too.
If we want these conditions to be the same, would it be better to make a hook that controls both of them based on the application state? Basically match this? |
Done! |
kevin940726
reviewed
Apr 18, 2023
packages/block-editor/README.md
Outdated
| @@ -880,6 +880,18 @@ _Returns_ | |||
|
|
|||
| - `any`: Returns the value defined for the setting. | |||
|
|
|||
| ### useShouldContextualToolbarShow | |||
There was a problem hiding this comment.
Question: Do we want to publicize this API just now? As soon as we make it public we have to make it backward-compatible. Maybe we can keep it internal for now?
There was a problem hiding this comment.
I agree with @kevin940726 - this looks useful only for internal use for now.
There was a problem hiding this comment.
oh I saw later why this is exported, so we can use it in the post editor later. Do we really need this useShouldContextualToolbarShow refactoring?
There was a problem hiding this comment.
Do we want to publicize this API just now?
Erring on the side of caution sounds good. What does making it internal look like? Adding experimental or unstable in front of the name?
There was a problem hiding this comment.
At the time I left this review, it was only used in the same package. If it's also used in other packages and it's useful then I don't mind publicizing it 😅 .
There was a problem hiding this comment.
I guess there's a fine line between whether it should be implemented as a @wordpress/data selector or a hook. I don't mind either but would rather keep it consistent. It's just one small thing to consider before we publicizing it :).
There was a problem hiding this comment.
@kevin940726 - I believe we tried to do use selector first, but we were blocked due to the useViewportMatch. @ajlende can confirm/deny this statement 😅
There was a problem hiding this comment.
Yeah, the useViewportMatch is the problem with making it a selector. We could, in the future, do the same thing as useViewportMatch and update the store when the media query changes. So it's a good idea to make it private.
draganescu
previously requested changes
Apr 18, 2023
packages/block-editor/README.md
Outdated
| @@ -880,6 +880,18 @@ _Returns_ | |||
|
|
|||
| - `any`: Returns the value defined for the setting. | |||
|
|
|||
| ### useShouldContextualToolbarShow | |||
There was a problem hiding this comment.
oh I saw later why this is exported, so we can use it in the post editor later. Do we really need this useShouldContextualToolbarShow refactoring?
| useShouldContextualToolbarShow( selectedBlockId ); | ||
| // If there's a block toolbar to be focused, disable the focus shortcut for the document toolbar. | ||
| // There's a fixed block toolbar when the fixed toolbar option is enabled or when the browser width is less than the large viewport. | ||
| const blockToolbarCanBeFocused = |
There was a problem hiding this comment.
Is there a similar thing going on in edit-site too? What is the advantage of using useShouldContextualToolbarShow here instead of leaving all the conditions inlined where they were and just looking at ( ( hasFixedToolbar || ! isLargeViewport ) && selectedBlockId ) here?
There was a problem hiding this comment.
There was a lot of conditions in the block toolbar and @ajlende pointed out that we might be missing some conditions. For example, my conditions didn't account for smaller viewports. The way to be sure we weren't sending focus to the top toolbar was to use all the same conditions as we are for the block toolbar. That's why we decided to consolidate them.
All that said, I don't have a strong preference. I do like this hook though. It can be very useful for knowing if the block toolbar should be showing or if it will be focusable from a keyboard shortcut anywhere else in Gutenberg.
There was a problem hiding this comment.
Having the hook means that we know it will be the same in both places which is what we want. From previous experience, if you don't do things this way in gutenberg, someone is going to update the condition in one place and miss the other place.
The 'core/block-editor/focus-toolbar' shortcut (alt + F10) selects the active navigable toolbar's fi rst tabbable item, but when there are multiple navigable toolbars, it places focus on all of the act ive navigable toolbar components. This results in an odd state of two tooltips being open. This comm it scaffolds the ability to disable the keyboard shortcut from applying.
I couldn't find a reliable way of determining if there is a block toolbar showing, so I approximated it by checking if there are no selected blocks or we're not in 'edit' mode. If one of those conditions applies, then a focusable block toolbar won't be present (there may other instances I'm missing though), and we can enable the shortcut for the header toolbar.
I hope I'm missing a reliable isBlockToolbarShowing() instead of doingn all these checks, but I didn't see a function like that. Maybe it's worth adding?
…dDefaultBlock We don't care if it's an unmodified default block or not in the condition to use the shortcut for the top level toolbar, we need to know if there's a chance the toolbar is showing or not. This simplifies the logic so anytime a block with a toolbar is selected, we set maybeBlockToolbarShowing to true, then check the extra condition of a fixed toolbar with ANY block selected.
There were 12 tests and each time had to go through all of the test set-up. I combined the tests into groups based into view and mode for four total tests. This cuts the test time down by about half, (18s for the previous 12 tests vs 9s for these 4 tests). This is recommended by Playwright Best Practices of "write fewer tests but longer tests": https://playwright.dev/docs/best-practices#write-fewer-tests-but-longer-tests. I think the time saved per commit is worth having more assertions per test.
…seShouldContextualToolbarShow hook In order to decide if the block toolbar or document toolbar should be focused from the alt+F10 shortcut, we need to do duplicate the same code in both toolbar wrappers. This moves them into a shared hook to keep things consistent.
draganescu
dismissed
their stale review
I understand the reasons for the refactoring but also recommend we make the hook a private API.
jeryj
force-pushed
the
fix/multiple-tooltips-on-navigable-toolbar
branch
from
9ad643a
to
3eef424
Compare
|
We should create issues for the follow ups here. |
This was referenced Apr 19, 2023
Thelmachido
added
[Type] Bug
Mamaduka
reviewed
Jun 16, 2023
Comment on lines
+80
to
+83
| test.beforeEach( async ( { editor } ) => { | ||
| // Ensure the fixed toolbar option is on | ||
| await editor.setIsFixedToolbar( true ); | ||
| } ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice catch! Put in a fix here: #51600
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.
The 'core/block-editor/focus-toolbar' shortcut (alt + F10) selects the active navigable toolbar's first tabbable item, but when there are multiple navigable toolbars, it places focus on all of the active navigable toolbar components. This results in an odd state of two tooltips being open. This commit scaffolds the ability to disable the keyboard shortcut from applying.
TODO: Figure out how to identify if multiple
NavigableToolbarsare present, or at least tell the document toolbar not to show if the block toolbar is present.What?
Only apply the shortcut to one
NavigableToolbarcomponent.Why?
Each
NavigableToolbargets analt + F10(orfn + option + F10on mac keyboards) shortcut added to it. When there are multipleNavigableToolbars components on the page, they all try to focus the first tabbable element within it, displaying multiple popovers at once.How?
Add
useKeyboardShortcutprop toNavigableToolbaranduseToolbarFocuscomponents so the documentHeaderToolbarcan disable itself from using the shortcut if the block toolbar is also showing.If there are no selected blocks OR we're not in
editmode, then we'll allow the keyboard shortcut for the header toolbar to be active. There may be other conditions I'm missing to identify if a focusable block toolbar is showing.The shortcut says
Navigate to the nearest toolbarwhich means to me, "Move to the toolbar closest to my current focus in the DOM," but the implementation here is, "Move to the most specific toolbar." As in, if there is a block toolbar open, move to it. If there is only one toolbar open, move to it.Testing Instructions
These are all documented in the e2e tests. To see them all running, add:
to the top of
test/e2e/specs/editor/various/shortcut-focus-toolbar.spec.js(like, line 5) and then run these tests withnpm run test:e2e:playwright -- shortcut-focus-toolbar --headedTesting Instructions for Keyboard
Block toolbar open
alt + F10(orfn + option + F10on mac keyboards)No selected blocks
alt + F10(orfn + option + F10on mac keyboards)In Select Mode
alt + F10(orfn + option + F10on mac keyboards)Basically, try to break it using all of the different view modes (Top Toolbar on/off, Spotlight mode, etc) and with an empty default blocks selected, a block selected, multiple blocks selected, etc. Anytime a block toolbar is showing, it should receive focus with the
alt + F10shortcut, and anytime it's not showing, focus should be in the top toolbar.Screenshots or screencast
Broken state:
https://user-images.githubusercontent.com/967608/230490886-5849ba0e-1910-47b0-b522-750b2f99e70c.mov