-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix multiple tooltips showing on NavigableToolbars (#49644)
* Add useKeyboardShortcut prop to NavigableToolbar and useToolbarFocus 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. * Conditionally useKeyboardFocusShortcut in HeaderToolbar 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. * Remove accidentally added line break * Check all the conditions when a toolbar might be showing 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? * Remove unnecesary line break * Change logic to use isBlockWithToolbarSelected instead of isUnmodifiedDefaultBlock 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. * Start on e2e tests to cover various scenarios * e2e test: should focus the top level toolbar when in select mode * ede test: should focus the top level toolbar when on an empty block * ede test: grouping into describe test sections * Scaffold top toolbar mode e2e tests * e2e tests: Top Toolbar in edit mode * e2e tests: Top toolbar + select mode * Refactor toggle fixed toolbar to playwright editor class * Refactor: consolodate e2e locators * Refactor: Combine tests into fewer cases to save execution time 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. * Use POM-style ToolbarUtils for e2e test * Improve naming of isFixed to setIsFixedToolbar * Improve naming to shouldUseKeyboardFocusShortcut * Simplify logic for setting isBlockWithToolbarSelected * Rename toggleFixedToolbar util to setIsFixedToolbar * Refactor shouldShowContextualToolbar and canFocusHiddenToolbar into useShouldContextualToolbarShow 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. * Export useShouldContextualToolbarShow * Add hasClientId check, as we can't focus a block toolbar without a block ID * Implement useShouldContextualToolbarShow in header toolbar * Add tests for smaller viewports * Update e2e tests to address new unified toolbar view * Make useShouldContextualToolbarShow private
- Loading branch information
Showing
8 changed files
with
422 additions
and
50 deletions.
There are no files selected for viewing
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
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
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
75 changes: 75 additions & 0 deletions
75
packages/block-editor/src/utils/use-should-contextual-toolbar-show.js
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useSelect } from '@wordpress/data'; | ||
| import { isUnmodifiedDefaultBlock } from '@wordpress/blocks'; | ||
| import { useViewportMatch } from '@wordpress/compose'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { store as blockEditorStore } from '../store'; | ||
| import { unlock } from '../lock-unlock'; | ||
|
|
||
| /** | ||
| * Returns true if the contextual block toolbar should show, or false if it should be hidden. | ||
| * | ||
| * @param {string} clientId The client ID of the block. | ||
| * | ||
| * @return {boolean} Whether the block toolbar is hidden. | ||
| */ | ||
| export function useShouldContextualToolbarShow( clientId ) { | ||
| const isLargeViewport = useViewportMatch( 'medium' ); | ||
|
|
||
| const { shouldShowContextualToolbar, canFocusHiddenToolbar } = useSelect( | ||
| ( select ) => { | ||
| const { | ||
| __unstableGetEditorMode, | ||
| isMultiSelecting, | ||
| isTyping, | ||
| isBlockInterfaceHidden, | ||
| getBlock, | ||
| getSettings, | ||
| isNavigationMode, | ||
| } = unlock( select( blockEditorStore ) ); | ||
|
|
||
| const isEditMode = __unstableGetEditorMode() === 'edit'; | ||
| const hasFixedToolbar = getSettings().hasFixedToolbar; | ||
| const isDistractionFree = getSettings().isDistractionFree; | ||
| const hasClientId = !! clientId; | ||
| const isEmptyDefaultBlock = isUnmodifiedDefaultBlock( | ||
| getBlock( clientId ) || {} | ||
| ); | ||
|
|
||
| const _shouldShowContextualToolbar = | ||
| isEditMode && | ||
| ! hasFixedToolbar && | ||
| ( ! isDistractionFree || isNavigationMode() ) && | ||
| isLargeViewport && | ||
| ! isMultiSelecting() && | ||
| ! isTyping() && | ||
| hasClientId && | ||
| ! isEmptyDefaultBlock && | ||
| ! isBlockInterfaceHidden(); | ||
|
|
||
| const _canFocusHiddenToolbar = | ||
| isEditMode && | ||
| hasClientId && | ||
| ! _shouldShowContextualToolbar && | ||
| ! hasFixedToolbar && | ||
| ! isDistractionFree && | ||
| ! isEmptyDefaultBlock; | ||
|
|
||
| return { | ||
| shouldShowContextualToolbar: _shouldShowContextualToolbar, | ||
| canFocusHiddenToolbar: _canFocusHiddenToolbar, | ||
| }; | ||
| }, | ||
| [ clientId, isLargeViewport ] | ||
| ); | ||
|
|
||
| return { | ||
| shouldShowContextualToolbar, | ||
| canFocusHiddenToolbar, | ||
| }; | ||
| } |
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
21 changes: 21 additions & 0 deletions
21
packages/e2e-test-utils-playwright/src/editor/set-is-fixed-toolbar.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { Editor } from './index'; | ||
|
|
||
| /** | ||
| * Toggles the fixed toolbar option. | ||
| * | ||
| * @param this | ||
| * @param isFixed Boolean value true/false for on/off. | ||
| */ | ||
| export async function setIsFixedToolbar( this: Editor, isFixed: boolean ) { | ||
| await this.page.evaluate( ( _isFixed ) => { | ||
| const { select, dispatch } = window.wp.data; | ||
| const isCurrentlyFixed = | ||
| select( 'core/edit-post' ).isFeatureActive( 'fixedToolbar' ); | ||
| if ( isCurrentlyFixed !== _isFixed ) { | ||
| dispatch( 'core/edit-post' ).toggleFeature( 'fixedToolbar' ); | ||
| } | ||
| }, isFixed ); | ||
| } |
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
Oops, something went wrong.