Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple tooltips showing on NavigableToolbars #49644

Merged
merged 28 commits into from
Apr 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
820035d
Add useKeyboardShortcut prop to NavigableToolbar and useToolbarFocus
jeryj Apr 6, 2023
2c99327
Conditionally useKeyboardFocusShortcut in HeaderToolbar
jeryj Apr 6, 2023
3db06db
Remove accidentally added line break
jeryj Apr 6, 2023
87eabd8
Check all the conditions when a toolbar might be showing
jeryj Apr 6, 2023
b7e3091
Remove unnecesary line break
jeryj Apr 6, 2023
9222cc6
Change logic to use isBlockWithToolbarSelected instead of isUnmodifie…
jeryj Apr 7, 2023
d4ed2ba
Start on e2e tests to cover various scenarios
jeryj Apr 7, 2023
c05a396
e2e test: should focus the top level toolbar when in select mode
jeryj Apr 7, 2023
3146b4e
ede test: should focus the top level toolbar when on an empty block
jeryj Apr 7, 2023
ec44ad5
ede test: grouping into describe test sections
jeryj Apr 7, 2023
c201962
Scaffold top toolbar mode e2e tests
jeryj Apr 7, 2023
964d06d
e2e tests: Top Toolbar in edit mode
jeryj Apr 11, 2023
8050ffb
e2e tests: Top toolbar + select mode
jeryj Apr 11, 2023
a65fd0a
Refactor toggle fixed toolbar to playwright editor class
jeryj Apr 11, 2023
02cfb03
Refactor: consolodate e2e locators
jeryj Apr 11, 2023
abeb991
Refactor: Combine tests into fewer cases to save execution time
jeryj Apr 11, 2023
26d79a2
Use POM-style ToolbarUtils for e2e test
jeryj Apr 12, 2023
a4c4633
Improve naming of isFixed to setIsFixedToolbar
jeryj Apr 12, 2023
29ccd55
Improve naming to shouldUseKeyboardFocusShortcut
jeryj Apr 12, 2023
f51fb72
Simplify logic for setting isBlockWithToolbarSelected
jeryj Apr 12, 2023
6bfb00f
Rename toggleFixedToolbar util to setIsFixedToolbar
jeryj Apr 13, 2023
766ef57
Refactor shouldShowContextualToolbar and canFocusHiddenToolbar into u…
jeryj Apr 13, 2023
9657c9d
Export useShouldContextualToolbarShow
jeryj Apr 13, 2023
82784d6
Add hasClientId check, as we can't focus a block toolbar without a bl…
jeryj Apr 14, 2023
aede555
Implement useShouldContextualToolbarShow in header toolbar
jeryj Apr 14, 2023
21deaa2
Add tests for smaller viewports
jeryj Apr 14, 2023
47c6561
Update e2e tests to address new unified toolbar view
jeryj Apr 19, 2023
3eef424
Make useShouldContextualToolbarShow private
jeryj Apr 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { useRef, useEffect } from '@wordpress/element';
import { isUnmodifiedDefaultBlock } from '@wordpress/blocks';
import { useDispatch, useSelect } from '@wordpress/data';
import { useShortcut } from '@wordpress/keyboard-shortcuts';
import { useViewportMatch } from '@wordpress/compose';

/**
* Internal dependencies
Expand All @@ -22,26 +21,20 @@ import BlockPopover from '../block-popover';
import useBlockToolbarPopoverProps from './use-block-toolbar-popover-props';
import Inserter from '../inserter';
import { unlock } from '../../lock-unlock';
import { privateApis } from '../../private-apis';

function selector( select ) {
const {
__unstableGetEditorMode,
isMultiSelecting,
hasMultiSelection,
isTyping,
isBlockInterfaceHidden,
getSettings,
getLastMultiSelectedBlockClientId,
} = unlock( select( blockEditorStore ) );

return {
editorMode: __unstableGetEditorMode(),
hasMultiSelection: hasMultiSelection(),
isMultiSelecting: isMultiSelecting(),
isTyping: isTyping(),
isBlockInterfaceHidden: isBlockInterfaceHidden(),
hasFixedToolbar: getSettings().hasFixedToolbar,
isDistractionFree: getSettings().isDistractionFree,
lastClientId: hasMultiSelection()
? getLastMultiSelectedBlockClientId()
: null,
Expand All @@ -52,21 +45,16 @@ function SelectedBlockPopover( {
clientId,
rootClientId,
isEmptyDefaultBlock,
showContents, // we may need to mount an empty popover because we reuse
capturingClientId,
__unstablePopoverSlot,
__unstableContentRef,
} ) {
const {
editorMode,
hasMultiSelection,
isMultiSelecting,
isTyping,
isBlockInterfaceHidden,
hasFixedToolbar,
isDistractionFree,
lastClientId,
} = useSelect( selector, [] );
const { editorMode, hasMultiSelection, isTyping, lastClientId } = useSelect(
selector,
[]
);

const { useShouldContextualToolbarShow } = unlock( privateApis );
const isInsertionPointVisible = useSelect(
( select ) => {
const {
Expand All @@ -85,29 +73,17 @@ function SelectedBlockPopover( {
},
[ clientId ]
);
const isLargeViewport = useViewportMatch( 'medium' );
const isToolbarForced = useRef( false );
const { shouldShowContextualToolbar, canFocusHiddenToolbar } =
useShouldContextualToolbarShow( clientId );

const { stopTyping } = useDispatch( blockEditorStore );

const showEmptyBlockSideInserter =
! isTyping && editorMode === 'edit' && isEmptyDefaultBlock;
const shouldShowBreadcrumb =
! hasMultiSelection &&
( editorMode === 'navigation' || editorMode === 'zoom-out' );
const shouldShowContextualToolbar =
editorMode === 'edit' &&
! hasFixedToolbar &&
isLargeViewport &&
! isMultiSelecting &&
! showEmptyBlockSideInserter &&
! isTyping &&
! isBlockInterfaceHidden;
const canFocusHiddenToolbar =
editorMode === 'edit' &&
! shouldShowContextualToolbar &&
! hasFixedToolbar &&
! isDistractionFree &&
! isEmptyDefaultBlock;

useShortcut(
'core/block-editor/focus-toolbar',
Expand Down Expand Up @@ -179,7 +155,7 @@ function SelectedBlockPopover( {
resize={ false }
{ ...popoverProps }
>
{ shouldShowContextualToolbar && showContents && (
{ shouldShowContextualToolbar && (
<BlockContextualToolbar
// If the toolbar is being shown because of being forced
// it should focus the toolbar right after the mount.
Expand Down Expand Up @@ -215,8 +191,6 @@ function wrapperSelector( select ) {
getBlockRootClientId,
getBlock,
getBlockParents,
getSettings,
isNavigationMode: _isNavigationMode,
__experimentalGetBlockListSettingsForBlocks,
} = select( blockEditorStore );

Expand All @@ -242,14 +216,10 @@ function wrapperSelector( select ) {
?.__experimentalCaptureToolbars
);

const settings = getSettings();

return {
clientId,
rootClientId: getBlockRootClientId( clientId ),
name,
isDistractionFree: settings.isDistractionFree,
isNavigationMode: _isNavigationMode(),
isEmptyDefaultBlock:
name && isUnmodifiedDefaultBlock( { name, attributes } ),
capturingClientId,
Expand All @@ -272,8 +242,6 @@ export default function WrappedBlockPopover( {
name,
isEmptyDefaultBlock,
capturingClientId,
isDistractionFree,
isNavigationMode,
} = selected;

if ( ! name ) {
Expand All @@ -285,7 +253,6 @@ export default function WrappedBlockPopover( {
clientId={ clientId }
rootClientId={ rootClientId }
isEmptyDefaultBlock={ isEmptyDefaultBlock }
showContents={ ! isDistractionFree || isNavigationMode }
capturingClientId={ capturingClientId }
__unstablePopoverSlot={ __unstablePopoverSlot }
__unstableContentRef={ __unstableContentRef }
Expand Down
15 changes: 12 additions & 3 deletions packages/block-editor/src/components/navigable-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ function useToolbarFocus(
focusOnMount,
isAccessibleToolbar,
defaultIndex,
onIndexChange
onIndexChange,
shouldUseKeyboardFocusShortcut
) {
// Make sure we don't use modified versions of this prop.
const [ initialFocusOnMount ] = useState( focusOnMount );
Expand All @@ -103,8 +104,14 @@ function useToolbarFocus(
focusFirstTabbableIn( ref.current );
}, [] );

const focusToolbarViaShortcut = () => {
if ( shouldUseKeyboardFocusShortcut ) {
focusToolbar();
}
};

// Focus on toolbar when pressing alt+F10 when the toolbar is visible.
useShortcut( 'core/block-editor/focus-toolbar', focusToolbar );
useShortcut( 'core/block-editor/focus-toolbar', focusToolbarViaShortcut );
draganescu marked this conversation as resolved.
Show resolved Hide resolved

useEffect( () => {
if ( initialFocusOnMount ) {
Expand Down Expand Up @@ -147,6 +154,7 @@ function useToolbarFocus(
function NavigableToolbar( {
children,
focusOnMount,
shouldUseKeyboardFocusShortcut = true,
__experimentalInitialIndex: initialIndex,
__experimentalOnIndexChange: onIndexChange,
...props
Expand All @@ -159,7 +167,8 @@ function NavigableToolbar( {
focusOnMount,
isAccessibleToolbar,
initialIndex,
onIndexChange
onIndexChange,
shouldUseKeyboardFocusShortcut
);

if ( isAccessibleToolbar ) {
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/private-apis.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import ResizableBoxPopover from './components/resizable-box-popover';
import { ComposedPrivateInserter as PrivateInserter } from './components/inserter';
import { PrivateListView } from './components/list-view';
import BlockInfo from './components/block-info-slot-fill';
import { useShouldContextualToolbarShow } from './utils/use-should-contextual-toolbar-show';

/**
* Private @wordpress/block-editor APIs.
Expand All @@ -24,4 +25,5 @@ lock( privateApis, {
PrivateListView,
ResizableBoxPopover,
BlockInfo,
useShouldContextualToolbarShow,
} );
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,
};
}
4 changes: 4 additions & 0 deletions packages/e2e-test-utils-playwright/src/editor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { selectBlocks } from './select-blocks';
import { setContent } from './set-content';
import { showBlockToolbar } from './show-block-toolbar';
import { saveSiteEditorEntities } from './site-editor';
import { setIsFixedToolbar } from './set-is-fixed-toolbar';
import { transformBlockTo } from './transform-block-to';

type EditorConstructorProps = {
Expand Down Expand Up @@ -68,6 +69,9 @@ export class Editor {
setContent: typeof setContent = setContent.bind( this );
/** @borrows showBlockToolbar as this.showBlockToolbar */
showBlockToolbar: typeof showBlockToolbar = showBlockToolbar.bind( this );
/** @borrows setIsFixedToolbar as this.setIsFixedToolbar */
setIsFixedToolbar: typeof setIsFixedToolbar =
setIsFixedToolbar.bind( this );
/** @borrows transformBlockTo as this.transformBlockTo */
transformBlockTo: typeof transformBlockTo = transformBlockTo.bind( this );
}
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 );
}
31 changes: 28 additions & 3 deletions packages/edit-post/src/components/header/header-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
NavigableToolbar,
ToolSelector,
store as blockEditorStore,
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';
import {
EditorHistoryRedo,
Expand All @@ -23,6 +24,7 @@ import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';
* Internal dependencies
*/
import { store as editPostStore } from '../../../store';
import { unlock } from '../../../private-apis';

const preventDefault = ( event ) => {
event.preventDefault();
Expand All @@ -39,15 +41,27 @@ function HeaderToolbar() {
showIconLabels,
isListViewOpen,
listViewShortcut,
selectedBlockId,
hasFixedToolbar,
} = useSelect( ( select ) => {
const { hasInserterItems, getBlockRootClientId, getBlockSelectionEnd } =
select( blockEditorStore );
const {
hasInserterItems,
getBlockRootClientId,
getBlockSelectionEnd,
getSelectedBlockClientId,
getFirstMultiSelectedBlockClientId,
getSettings,
} = select( blockEditorStore );
const { getEditorSettings } = select( editorStore );
const { getEditorMode, isFeatureActive, isListViewOpened } =
select( editPostStore );
const { getShortcutRepresentation } = select( keyboardShortcutsStore );

return {
hasFixedToolbar: getSettings().hasFixedToolbar,
selectedBlockId:
getSelectedBlockClientId() ||
getFirstMultiSelectedBlockClientId(),
// This setting (richEditingEnabled) should not live in the block editor's setting.
isInserterEnabled:
getEditorMode() === 'visual' &&
Expand All @@ -64,9 +78,19 @@ function HeaderToolbar() {
),
};
}, [] );

const { useShouldContextualToolbarShow } = unlock( blockEditorPrivateApis );

const isLargeViewport = useViewportMatch( 'medium' );
const isWideViewport = useViewportMatch( 'wide' );

const { shouldShowContextualToolbar, canFocusHiddenToolbar } =
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 =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

shouldShowContextualToolbar ||
canFocusHiddenToolbar ||
( ( hasFixedToolbar || ! isLargeViewport ) && selectedBlockId );
/* translators: accessibility text for the editor toolbar */
const toolbarAriaLabel = __( 'Document tools' );

Expand Down Expand Up @@ -114,6 +138,7 @@ function HeaderToolbar() {
<NavigableToolbar
className="edit-post-header-toolbar"
aria-label={ toolbarAriaLabel }
shouldUseKeyboardFocusShortcut={ ! blockToolbarCanBeFocused }
>
<div className="edit-post-header-toolbar__left">
<ToolbarItem
Expand Down
Loading