Only render top toolbar in header slot if large viewport

There were two forms of the small screen top toolbar -- one when the top toolbar option was on, and one when it was off. Both were intended to look and operate in the same way. Rather than maintain two identical systems, it makes more sense to only have one. This also makes the visual and tab order match for all versions, which is better accessibility too.
WordPress · Nov 6, 2023 · 57e2f67 · 57e2f67
1 parent 9befeb0
commit 57e2f67
Show file tree

Hide file tree

Showing 10 changed files with 122 additions and 61 deletions.
diff --git a/packages/block-editor/src/components/block-toolbar/style.scss b/packages/block-editor/src/components/block-toolbar/style.scss
@@ -56,6 +56,14 @@
 	}
 }
 
+.block-editor-block-contextual-toolbar.is-fixed {
+	position: sticky;
+	top: 0;
+	z-index: z-index(".block-editor-block-popover");
+	display: block;
+	width: 100%;
+}
+
 // on desktop browsers the fixed toolbar has tweaked borders
 @include break-medium() {
 	.block-editor-block-contextual-toolbar.is-fixed {

diff --git a/packages/block-editor/src/components/block-tools/index.js b/packages/block-editor/src/components/block-tools/index.js
@@ -170,6 +170,13 @@ export default function BlockTools( {
 	const blockToolbarRef = usePopoverScroll( __unstableContentRef );
 	const blockToolbarAfterRef = usePopoverScroll( __unstableContentRef );
 
+	// Conditions for fixed toolbar
+	// 1. Not zoom out mode
+	// 2. It's a large viewport. If it's a smaller viewport, let the floating toolbar handle it as it already has styles attached to make it render that way.
+	// 3. Fixed toolbar is enabled
+	const isTopToolbar = ! isZoomOutMode && hasFixedToolbar && isLargeViewport;
+	const isTopToolbarFill = isTopToolbar && blockToolsSlot?.ref?.current;
+
 	return (
 		// eslint-disable-next-line jsx-a11y/no-static-element-interactions
 		<div { ...props } onKeyDown={ onKeyDown }>
@@ -180,21 +187,23 @@ export default function BlockTools( {
 					/>
 				) }
 				{ /* If there is no slot available, such as in the standalone block editor, render within the editor */ }
-				{ ! isZoomOutMode &&
-					( hasFixedToolbar || ! isLargeViewport ) &&
-					( blockToolsSlot?.ref?.current ? (
-						<Fill name="__experimentalSelectedBlockTools">
-							<BlockContextualToolbar
-								ref={ selectedBlockToolsRef }
-								isFixed
-							/>
-						</Fill>
-					) : (
+				{ isTopToolbarFill && (
+					<Fill name="__experimentalSelectedBlockTools">
 						<BlockContextualToolbar
 							ref={ selectedBlockToolsRef }
 							isFixed
 						/>
-					) ) }
+					</Fill>
+				) }
+
+				{ ! isTopToolbarFill &&
+					( isTopToolbar || ! isLargeViewport ) && ( // Small viewports always get a fixed toolbar
+						<BlockContextualToolbar
+							ref={ selectedBlockToolsRef }
+							isFixed
+						/>
+					) }
+
 				{ showEmptyBlockSideInserter && (
 					<EmptyBlockInserter
 						__unstableContentRef={ __unstableContentRef }
@@ -212,8 +221,8 @@ export default function BlockTools( {
 				) }
 
 				{ /* Used for the inline rich text toolbar. */ }
-				{ /* If there is no slot available, such as in the standalone block editor, render within the editor */ }
-				{ blockToolsSlot?.ref?.current ? (
+				{ /* Only render in the fill if we're also using the top toolbar fill */ }
+				{ isTopToolbarFill ? (
 					<Fill name="__experimentalSelectedBlockTools">
 						<Popover.Slot
 							name="block-toolbar"

diff --git a/packages/edit-post/src/components/header/index.js b/packages/edit-post/src/components/header/index.js
@@ -75,7 +75,7 @@ function Header( {
 					hasFixedToolbar={ hasFixedToolbar }
 					setListViewToggleElement={ setListViewToggleElement }
 				/>
-				{ hasFixedToolbar && (
+				{ hasFixedToolbar && isLargeViewport && (
 					<Slot
 						className="selected-block-tools-wrapper"
 						name="__experimentalSelectedBlockTools"

diff --git a/packages/edit-post/src/components/header/style.scss b/packages/edit-post/src/components/header/style.scss
@@ -52,19 +52,9 @@
 	}
 
 	.block-editor-block-contextual-toolbar.is-fixed {
-		position: fixed;
-		top: $admin-bar-height-big + $header-height + 1px; // +1px to avoid overlap with the header border
-		left: 0;
-		width: 100%;
-
-		@include break-medium() {
-			width: auto;
-			position: static;
-			// remove the border
-			border: none;
-			flex-shrink: 2;
-			display: flex; // Allow for flex-shrink on the block toolbar
-		}
+		border: none;
+		flex-shrink: 2;
+		display: flex; // Allow for flex-shrink on the block toolbar
 
 		.block-editor-block-toolbar {
 			overflow-x: auto; // Allow only the block tools to overflow scroll to make room in the header.

diff --git a/packages/edit-site/src/components/header-edit-mode/index.js b/packages/edit-site/src/components/header-edit-mode/index.js
@@ -146,21 +146,19 @@ export default function HeaderEditMode( { setListViewToggleElement } ) {
 						showIconLabels={ showIconLabels }
 						setListViewToggleElement={ setListViewToggleElement }
 					/>
-					{ hasFixedToolbar && (
+					{ hasFixedToolbar && isLargeViewport && (
 						<>
 							<Slot
 								className={ classnames(
 									'selected-block-tools-wrapper',
 									{
-										'is-collapsed':
-											isBlockToolsCollapsed &&
-											isLargeViewport,
+										'is-collapsed': isBlockToolsCollapsed,
 									}
 								) }
 								name="__experimentalSelectedBlockTools"
 								bubblesVirtually
 							/>
-							{ isLargeViewport && hasBlockSelected && (
+							{ hasBlockSelected && (
 								<Button
 									className="edit-site-header-edit-mode__block-tools-toggle"
 									icon={

diff --git a/packages/edit-site/src/components/header-edit-mode/style.scss b/packages/edit-site/src/components/header-edit-mode/style.scss
@@ -81,16 +81,11 @@ $header-toolbar-min-width: 335px;
 	}
 }
 
-.edit-site-header-edit-mode .block-editor-block-contextual-toolbar.is-fixed {
-	position: fixed;
-	top: $header-height; // +1px to avoid overlap with the header border
-	left: 0;
+.block-editor-block-contextual-toolbar.is-fixed {
 	width: 100%;
 
 	@include break-medium() {
 		width: auto;
-		position: relative;
-		top: 0;
 		// remove the border
 		border: none;
 	}

diff --git a/packages/edit-site/src/components/layout/style.scss b/packages/edit-site/src/components/layout/style.scss
@@ -40,15 +40,6 @@
 	z-index: z-index(".edit-site-layout__header-container");
 }
 
-// Make room for the header when a block is selected.
-.is-block-toolbar-visible .edit-site-layout__header-container {
-	padding-bottom: $admin-bar-height-big + 1px;
-
-	@include break-medium() {
-		padding-bottom: 0;
-	}
-}
-
 .edit-site-layout__header {
 	height: $header-height;
 	display: flex;

diff --git a/packages/edit-widgets/src/components/header/index.js b/packages/edit-widgets/src/components/header/index.js
@@ -17,6 +17,7 @@ import MoreMenu from '../more-menu';
 
 function Header( { setListViewToggleElement } ) {
 	const isMediumViewport = useViewportMatch( 'medium' );
+	const isLargeViewport = useViewportMatch( 'large' );
 	const { hasFixedToolbar } = useSelect(
 		( select ) => ( {
 			hasFixedToolbar: !! select( preferencesStore ).get(
@@ -47,7 +48,7 @@ function Header( { setListViewToggleElement } ) {
 					<DocumentTools
 						setListViewToggleElement={ setListViewToggleElement }
 					/>
-					{ hasFixedToolbar && (
+					{ hasFixedToolbar && isLargeViewport && (
 						<Slot
 							className="selected-block-tools-wrapper"
 							name="__experimentalSelectedBlockTools"

diff --git a/packages/edit-widgets/src/components/header/style.scss b/packages/edit-widgets/src/components/header/style.scss
@@ -17,19 +17,10 @@
 	}
 
 	.block-editor-block-contextual-toolbar.is-fixed {
-		position: fixed;
-		top: $admin-bar-height-big + $header-height + 1px; // +1px to avoid overlap with the header border
-		left: 0;
-		width: 100%;
-
-		@include break-medium() {
-			width: auto;
-			position: static;
-			// remove the border
-			border: none;
-			display: flex; // Allow for flex-shrink on the block toolbar
-			flex-shrink: 2;
-		}
+		// remove the border
+		border: none;
+		display: flex; // Allow for flex-shrink on the block toolbar
+		flex-shrink: 2;
 
 		.block-editor-block-toolbar {
 			overflow-x: auto; // Allow only the block tools to overflow scroll to make room in the header.

diff --git a/test/e2e/specs/editor/various/keyboard-navigable-blocks.spec.js b/test/e2e/specs/editor/various/keyboard-navigable-blocks.spec.js
@@ -195,6 +195,84 @@ test.describe( 'Order of block keyboard navigation', () => {
 		// If active label matches, that means focus did not change from group block wrapper.
 		await KeyboardNavigableBlocks.expectLabelToHaveFocus( 'Block: Group' );
 	} );
+
+	test( 'Tab order of the block toolbar aligns with visual order', async ( {
+		editor,
+		KeyboardNavigableBlocks,
+		page,
+		pageUtils,
+	} ) => {
+		// On default floating toolbar
+		await editor.insertBlock( { name: 'core/paragraph' } );
+		await page.keyboard.type( 'Paragraph' );
+
+		// shift + tab
+		await pageUtils.pressKeys( 'shift+Tab' );
+		// check focus is within the block toolbar
+		const blockToolbarParagraphButton = page.getByRole( 'button', {
+			name: 'Paragraph',
+			exact: true,
+		} );
+		await expect( blockToolbarParagraphButton ).toBeFocused();
+		await pageUtils.pressKeys( 'Tab' );
+		// check focus is on the block
+		await KeyboardNavigableBlocks.expectLabelToHaveFocus(
+			'Block: Paragraph'
+		);
+
+		// set the screen size to mobile
+		await pageUtils.setBrowserViewport( 'small' );
+
+		// TEST: Small screen toolbar without fixed toolbar setting should be the first tabstop before the editor
+		await pageUtils.pressKeys( 'shift+Tab' );
+		// check focus is within the block toolbar
+		await expect( blockToolbarParagraphButton ).toBeFocused();
+		await pageUtils.pressKeys( 'Tab' );
+		// check focus is on the block
+		await KeyboardNavigableBlocks.expectLabelToHaveFocus(
+			'Block: Paragraph'
+		);
+		// TEST: Fixed toolbar should be within the header dom
+		// Changed to Fixed top toolbar setting and large viewport to test fixed toolbar
+		await pageUtils.setBrowserViewport( 'large' );
+		await editor.setIsFixedToolbar( true );
+		// shift + tab
+		await pageUtils.pressKeys( 'shift+Tab' );
+
+		// Options button is the last one in the top toolbar, the first item outside of the editor canvas, so it should get focused.
+		await KeyboardNavigableBlocks.expectLabelToHaveFocus( 'Options' );
+
+		await pageUtils.pressKeys( 'Tab' );
+		// check focus is on the block
+		await KeyboardNavigableBlocks.expectLabelToHaveFocus(
+			'Block: Paragraph'
+		);
+		// Move to block, alt + f10
+		await pageUtils.pressKeys( 'alt+F10' );
+		// check focus in block toolbar
+		await expect( blockToolbarParagraphButton ).toBeFocused();
+		// escape back to block
+		await pageUtils.pressKeys( 'Escape' );
+		// check block focus
+		await KeyboardNavigableBlocks.expectLabelToHaveFocus(
+			'Block: Paragraph'
+		);
+
+		// TEST: Small screen toolbar with fixed toolbar setting should be the first tabstop before the editor. Even though the fixed toolbar setting is on, it should not render within the header since it's visually after it.
+		await pageUtils.setBrowserViewport( 'small' );
+		await pageUtils.pressKeys( 'shift+Tab' );
+		// check focus is within the block toolbar
+		await expect( blockToolbarParagraphButton ).toBeFocused();
+		await pageUtils.pressKeys( 'Tab' );
+		// check focus is on the block
+		await KeyboardNavigableBlocks.expectLabelToHaveFocus(
+			'Block: Paragraph'
+		);
+
+		// Test cleanup
+		await editor.setIsFixedToolbar( false );
+		await pageUtils.setBrowserViewport( 'large' );
+	} );
 } );
 
 class KeyboardNavigableBlocks {