Fix shift+tab behavior to move to toolbar when the preceding block ha…

…s a form element (WordPress#51548) There was a bug in the tab navigation that allowed tabbing between form elements within a block. Shift + tab in this instance should move to the preceding tabbable element within the current block. However, it wasn't checking if the preceding form element was _within_ the current block, so when at the block boundary, a shift+tab would move to the preceding block's form element rather than the block toolbar. This changes the behavior to constrain the form elements _within the selected block_ and only handle shift+tab or tab if the element being tabbed to is within the current block. * Combine tests for checking for a navigable toolbar into one test to improve performance (WordPress#51550)
pewresearch · Jul 13, 2023 · 7e7ea16 · 7e7ea16
1 parent 14c6c03
commit 7e7ea16
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 46 deletions.
diff --git a/packages/block-editor/src/components/writing-flow/use-tab-nav.js b/packages/block-editor/src/components/writing-flow/use-tab-nav.js
@@ -116,17 +116,53 @@ export default function useTabNav() {
 				return;
 			}
 
+			// We want to constrain the tabbing to the block and its child blocks.
+			// If the preceding form element is within a different block,
+			// such as two sibling image blocks in the placeholder state,
+			// we want shift + tab from the first form element to move to the image
+			// block toolbar and not the previous image block's form element.
+			// TODO: Should this become a utility function?
+			/**
+			 * Determine whether an element is part of or is the selected block.
+			 *
+			 * @param {Object} selectedBlockElement
+			 * @param {Object} element
+			 * @return {boolean} Whether the element is part of or is the selected block.
+			 */
+			const isElementPartOfSelectedBlock = (
+				selectedBlockElement,
+				element
+			) => {
+				// Check if the element is or is within the selected block by finding the
+				// closest element with a data-block attribute and seeing if
+				// it matches our current selected block ID
+				const elementBlockId = element
+					.closest( '[data-block]' )
+					?.getAttribute( 'data-block' );
+				const isElementSameBlock =
+					elementBlockId === getSelectedBlockClientId();
+
+				// Check if the element is a child of the selected block. This could be a
+				// child block in a group or column block, etc.
+				const isElementChildOfBlock =
+					selectedBlockElement.contains( element );
+
+				return isElementSameBlock || isElementChildOfBlock;
+			};
+
+			const nextTabbable = focus.tabbable[ direction ]( event.target );
 			// Allow tabbing from the block wrapper to a form element,
-			// and between form elements rendered in a block,
+			// and between form elements rendered in a block and its child blocks,
 			// such as inside a placeholder. Form elements are generally
 			// meant to be UI rather than part of the content. Ideally
 			// these are not rendered in the content and perhaps in the
 			// future they can be rendered in an iframe or shadow DOM.
 			if (
-				( isFormElement( event.target ) ||
-					event.target.getAttribute( 'data-block' ) ===
-						getSelectedBlockClientId() ) &&
-				isFormElement( focus.tabbable[ direction ]( event.target ) )
+				isFormElement( nextTabbable ) &&
+				isElementPartOfSelectedBlock(
+					event.target.closest( '[data-block]' ),
+					nextTabbable
+				)
 			) {
 				return;
 			}

diff --git a/test/e2e/specs/editor/various/toolbar-roving-tabindex.spec.js b/test/e2e/specs/editor/various/toolbar-roving-tabindex.spec.js
@@ -16,11 +16,13 @@ test.describe( 'Toolbar roving tabindex', () => {
 		await page.keyboard.type( 'First block' );
 	} );
 
-	test( 'ensures paragraph block toolbar uses roving tabindex', async ( {
+	test( 'ensures base block toolbars use roving tabindex', async ( {
 		editor,
 		page,
+		pageUtils,
 		ToolbarRovingTabindexUtils,
 	} ) => {
+		// ensures paragraph block toolbar uses roving tabindex
 		await editor.insertBlock( { name: 'core/paragraph' } );
 		await page.keyboard.type( 'Paragraph' );
 		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
@@ -34,13 +36,8 @@ test.describe( 'Toolbar roving tabindex', () => {
 			'Paragraph block',
 			'Paragraph'
 		);
-	} );
 
-	test( 'ensures heading block toolbar uses roving tabindex', async ( {
-		editor,
-		page,
-		ToolbarRovingTabindexUtils,
-	} ) => {
+		// test: ensures heading block toolbar uses roving tabindex
 		await editor.insertBlock( { name: 'core/heading' } );
 		await page.keyboard.type( 'Heading' );
 		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
@@ -52,13 +49,8 @@ test.describe( 'Toolbar roving tabindex', () => {
 			'Block: Heading',
 			'Heading'
 		);
-	} );
 
-	test( 'ensures list block toolbar uses roving tabindex', async ( {
-		editor,
-		page,
-		ToolbarRovingTabindexUtils,
-	} ) => {
+		// ensures list block toolbar uses roving tabindex
 		await editor.insertBlock( { name: 'core/list' } );
 		await page.keyboard.type( 'List' );
 		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
@@ -71,36 +63,15 @@ test.describe( 'Toolbar roving tabindex', () => {
 			'Block: List',
 			'List'
 		);
-	} );
-
-	test( 'ensures image block toolbar uses roving tabindex', async ( {
-		editor,
-		ToolbarRovingTabindexUtils,
-	} ) => {
-		await editor.insertBlock( { name: 'core/image' } );
-		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
-			'Block: Image',
-			'Image'
-		);
-		await ToolbarRovingTabindexUtils.wrapCurrentBlockWithGroup( 'Image' );
-		await ToolbarRovingTabindexUtils.testGroupKeyboardNavigation(
-			'Block: Image',
-			'Image'
-		);
-	} );
 
-	test( 'ensures table block toolbar uses roving tabindex', async ( {
-		editor,
-		page,
-		ToolbarRovingTabindexUtils,
-		pageUtils,
-	} ) => {
+		// ensures table block toolbar uses roving tabindex
 		await editor.insertBlock( { name: 'core/table' } );
 		await page.keyboard.press( 'ArrowLeft' );
 		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
 			'Block: Table',
 			'Table'
 		);
+
 		// Move focus to the first toolbar item.
 		await page.keyboard.press( 'Home' );
 		await ToolbarRovingTabindexUtils.expectLabelToHaveFocus( 'Table' );
@@ -115,12 +86,8 @@ test.describe( 'Toolbar roving tabindex', () => {
 			'Block: Table',
 			'Table'
 		);
-	} );
 
-	test( 'ensures custom html block toolbar uses roving tabindex', async ( {
-		editor,
-		ToolbarRovingTabindexUtils,
-	} ) => {
+		// ensures custom html block toolbar uses roving tabindex
 		await editor.insertBlock( { name: 'core/html' } );
 		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
 			'HTML',
@@ -133,6 +100,19 @@ test.describe( 'Toolbar roving tabindex', () => {
 			'Block: Custom HTML',
 			'Custom HTML'
 		);
+
+		// ensures image block toolbar uses roving tabindex
+		// This also tests if shift + tab works as expected to move focus to the toolbar when the preceding block has a form element.
+		await editor.insertBlock( { name: 'core/image' } );
+		await ToolbarRovingTabindexUtils.testBlockToolbarKeyboardNavigation(
+			'Block: Image',
+			'Image'
+		);
+		await ToolbarRovingTabindexUtils.wrapCurrentBlockWithGroup( 'Image' );
+		await ToolbarRovingTabindexUtils.testGroupKeyboardNavigation(
+			'Block: Image',
+			'Image'
+		);
 	} );
 
 	test( 'ensures block toolbar remembers the last focused item', async ( {