From 8afcdaa4f3147b71455333db4b27d9507c7bb958 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Thu, 15 Jun 2023 11:20:57 -0500 Subject: [PATCH 1/2] Fix shift+tab behavior to move to toolbar when the preceding block has a form element 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. --- .../components/writing-flow/use-tab-nav.js | 46 +++++++++++++++++-- 1 file changed, 41 insertions(+), 5 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 index bb1d22b2865de..558182b7b5ac4 100644 --- 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; } From 5aabf438861238d7e3dfdc3b3047ecf79758070c Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Tue, 20 Jun 2023 06:18:35 -0500 Subject: [PATCH 2/2] Combine tests for checking for a navigable toolbar into one test to improve performance (#51550) --- .../various/toolbar-roving-tabindex.spec.js | 62 +++++++------------ 1 file changed, 21 insertions(+), 41 deletions(-) diff --git a/test/e2e/specs/editor/various/toolbar-roving-tabindex.spec.js b/test/e2e/specs/editor/various/toolbar-roving-tabindex.spec.js index 834bed77e8742..d17cef9215f4a 100644 --- 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 ( {