From 57e2f6759d3fea0a466b75b85734d9d421b3306b Mon Sep 17 00:00:00 2001
From: Jerry Jones <jones.jeremydavid@gmail.com>
Date: Fri, 3 Nov 2023 16:15:13 -0500
Subject: [PATCH] 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.
---
 .../src/components/block-toolbar/style.scss   |  8 ++
 .../src/components/block-tools/index.js       | 35 +++++----
 .../edit-post/src/components/header/index.js  |  2 +-
 .../src/components/header/style.scss          | 16 +---
 .../src/components/header-edit-mode/index.js  |  8 +-
 .../components/header-edit-mode/style.scss    |  7 +-
 .../src/components/layout/style.scss          |  9 ---
 .../src/components/header/index.js            |  3 +-
 .../src/components/header/style.scss          | 17 +---
 .../various/keyboard-navigable-blocks.spec.js | 78 +++++++++++++++++++
 10 files changed, 122 insertions(+), 61 deletions(-)

diff --git a/packages/block-editor/src/components/block-toolbar/style.scss b/packages/block-editor/src/components/block-toolbar/style.scss
index eef1678e2dfab..3f8a7057aef84 100644
--- 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
index de4ada16a7dd4..884bda02e75f9 100644
--- 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
index 130b8ec8b0ccb..2d293aa2bb57d 100644
--- 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
index 4f5e16474e909..5f8f913397783 100644
--- 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
index c35f1f1ddd681..3146eaa11d6fd 100644
--- 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
index 6f9eeb9c16459..75689e8f56cf3 100644
--- 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
index 66f2bf11c8f61..3bea97862b1c4 100644
--- 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
index acbefe28f3ebf..3df4c90055c59 100644
--- 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
index 8a2dcee046150..8b801844f1371 100644
--- 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
index 080abe011206a..e04a62499c2e8 100644
--- 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 {