From d1ea431f824610a350b7b44170d3ac8a5e52976a Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Tue, 11 Oct 2022 12:13:01 -0500 Subject: [PATCH 01/12] Store current region in state so region navigation remains in-order. --- .../components/src/higher-order/navigate-regions/index.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index 8244d26d78c6e..d3675fe6027c4 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -35,6 +35,7 @@ const defaultShortcuts = { export function useNavigateRegions( shortcuts = defaultShortcuts ) { const ref = useRef(); const [ isFocusingRegions, setIsFocusingRegions ] = useState( false ); + const [ selectedIndex, setSelectedIndex ] = useState( 0 ); function focusRegion( offset ) { const regions = Array.from( @@ -44,13 +45,11 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { return; } let nextRegion = regions[ 0 ]; - const selectedIndex = regions.indexOf( - ref.current.ownerDocument.activeElement - ); if ( selectedIndex !== -1 ) { let nextIndex = selectedIndex + offset; nextIndex = nextIndex === -1 ? regions.length - 1 : nextIndex; nextIndex = nextIndex === regions.length ? 0 : nextIndex; + setSelectedIndex( nextIndex ); nextRegion = regions[ nextIndex ]; } From 976746e6de76d563c5a5702a24f835580a821610 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Tue, 11 Oct 2022 14:09:49 -0500 Subject: [PATCH 02/12] Remove constant tabindex from regions. Add back when needed. Try to find better focus locations. --- .../higher-order/navigate-regions/index.js | 27 +++++++++++++------ .../components/interface-skeleton/index.js | 8 ------ 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index d3675fe6027c4..c89754ec269ff 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -8,6 +8,7 @@ import { useMergeRefs, } from '@wordpress/compose'; import { isKeyboardEvent } from '@wordpress/keycodes'; +import { focus } from '@wordpress/dom'; const defaultShortcuts = { previous: [ @@ -35,6 +36,7 @@ const defaultShortcuts = { export function useNavigateRegions( shortcuts = defaultShortcuts ) { const ref = useRef(); const [ isFocusingRegions, setIsFocusingRegions ] = useState( false ); + // Track the current region index since there is not a super reliable way to get it from DOM. const [ selectedIndex, setSelectedIndex ] = useState( 0 ); function focusRegion( offset ) { @@ -45,15 +47,24 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { return; } let nextRegion = regions[ 0 ]; - if ( selectedIndex !== -1 ) { - let nextIndex = selectedIndex + offset; - nextIndex = nextIndex === -1 ? regions.length - 1 : nextIndex; - nextIndex = nextIndex === regions.length ? 0 : nextIndex; - setSelectedIndex( nextIndex ); - nextRegion = regions[ nextIndex ]; - } + let nextIndex = selectedIndex + offset; + nextIndex = nextIndex === -1 ? regions.length - 1 : nextIndex; + nextIndex = nextIndex === regions.length ? 0 : nextIndex; + // Update the selected index for the current region. + setSelectedIndex( nextIndex ); + nextRegion = regions[ nextIndex ]; + + // Add the tabindex of -1 so tabbable elements can be located. + nextRegion.setAttribute( 'tabindex', '-1' ); + // Find the next tabbable within the region. + const nextTabbable = focus.tabbable.findNext( nextRegion ); + // Is there a next tabbable available? If not, focus the region instead. + const nextFocus = nextTabbable ? nextTabbable : nextRegion; + // Focus our chosen area, either the first tabbable within the region or the region itself. + nextFocus.focus(); + // Remove the tabindex, no longer needed after focus. + nextRegion.removeAttribute( 'tabindex' ); - nextRegion.focus(); setIsFocusingRegions( true ); } diff --git a/packages/interface/src/components/interface-skeleton/index.js b/packages/interface/src/components/interface-skeleton/index.js index 766ef73a2fcf5..984f0e9d6aa25 100644 --- a/packages/interface/src/components/interface-skeleton/index.js +++ b/packages/interface/src/components/interface-skeleton/index.js @@ -93,7 +93,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__drawer" role="region" aria-label={ mergedLabels.drawer } - tabIndex="-1" > { drawer } @@ -108,7 +107,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__header" role="region" aria-label={ mergedLabels.header } - tabIndex="-1" > { header } @@ -118,7 +116,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__header" role="region" aria-label={ mergedLabels.header } - tabIndex="-1" > { header } @@ -134,7 +131,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__secondary-sidebar" role="region" aria-label={ mergedLabels.secondarySidebar } - tabIndex="-1" > { secondarySidebar } @@ -148,7 +144,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__content" role="region" aria-label={ mergedLabels.body } - tabIndex="-1" > { content } @@ -157,7 +152,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__sidebar" role="region" aria-label={ mergedLabels.sidebar } - tabIndex="-1" > { sidebar } @@ -167,7 +161,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__actions" role="region" aria-label={ mergedLabels.actions } - tabIndex="-1" > { actions } @@ -179,7 +172,6 @@ function InterfaceSkeleton( className="interface-interface-skeleton__footer" role="region" aria-label={ mergedLabels.footer } - tabIndex="-1" > { footer } From 4e48751f10fe5171534887df344c860d5d060e47 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Fri, 14 Oct 2022 00:17:04 -0500 Subject: [PATCH 03/12] Use closest to determine the current region since it operates in a DOM upwards motion. From the current element, I should get the closest wrapping region. --- .../src/higher-order/navigate-regions/index.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index c89754ec269ff..4bc0bcadcc27e 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -36,8 +36,6 @@ const defaultShortcuts = { export function useNavigateRegions( shortcuts = defaultShortcuts ) { const ref = useRef(); const [ isFocusingRegions, setIsFocusingRegions ] = useState( false ); - // Track the current region index since there is not a super reliable way to get it from DOM. - const [ selectedIndex, setSelectedIndex ] = useState( 0 ); function focusRegion( offset ) { const regions = Array.from( @@ -47,12 +45,16 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { return; } let nextRegion = regions[ 0 ]; - let nextIndex = selectedIndex + offset; - nextIndex = nextIndex === -1 ? regions.length - 1 : nextIndex; - nextIndex = nextIndex === regions.length ? 0 : nextIndex; - // Update the selected index for the current region. - setSelectedIndex( nextIndex ); - nextRegion = regions[ nextIndex ]; + // Based off the current element, find the next region. + const selectedIndex = regions.indexOf( + ref.current.ownerDocument.activeElement.closest( '[role="region"]' ) + ); + if ( selectedIndex !== -1 ) { + let nextIndex = selectedIndex + offset; + nextIndex = nextIndex === -1 ? regions.length - 1 : nextIndex; + nextIndex = nextIndex === regions.length ? 0 : nextIndex; + nextRegion = regions[ nextIndex ]; + } // Add the tabindex of -1 so tabbable elements can be located. nextRegion.setAttribute( 'tabindex', '-1' ); From 00e15622f39f1b2ba9e96d88e715e673cc6f8ce1 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Fri, 14 Oct 2022 00:22:30 -0500 Subject: [PATCH 04/12] Better code comment. --- packages/components/src/higher-order/navigate-regions/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index 4bc0bcadcc27e..4c6a56a22d07f 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -45,7 +45,7 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { return; } let nextRegion = regions[ 0 ]; - // Based off the current element, find the next region. + // Based off the current element, use closest to determine the wrapping region since this operates up the DOM. const selectedIndex = regions.indexOf( ref.current.ownerDocument.activeElement.closest( '[role="region"]' ) ); From 9393dd997c8978549fd33727eafc897b680cc1fa Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Mon, 17 Oct 2022 11:04:18 -0500 Subject: [PATCH 05/12] Revert focus/tabindex changes. --- .../src/higher-order/navigate-regions/index.js | 13 +------------ .../src/components/interface-skeleton/index.js | 8 ++++++++ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index 4c6a56a22d07f..4085ead8a4e1f 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -8,7 +8,6 @@ import { useMergeRefs, } from '@wordpress/compose'; import { isKeyboardEvent } from '@wordpress/keycodes'; -import { focus } from '@wordpress/dom'; const defaultShortcuts = { previous: [ @@ -56,17 +55,7 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { nextRegion = regions[ nextIndex ]; } - // Add the tabindex of -1 so tabbable elements can be located. - nextRegion.setAttribute( 'tabindex', '-1' ); - // Find the next tabbable within the region. - const nextTabbable = focus.tabbable.findNext( nextRegion ); - // Is there a next tabbable available? If not, focus the region instead. - const nextFocus = nextTabbable ? nextTabbable : nextRegion; - // Focus our chosen area, either the first tabbable within the region or the region itself. - nextFocus.focus(); - // Remove the tabindex, no longer needed after focus. - nextRegion.removeAttribute( 'tabindex' ); - + nextRegion.focus(); setIsFocusingRegions( true ); } diff --git a/packages/interface/src/components/interface-skeleton/index.js b/packages/interface/src/components/interface-skeleton/index.js index 984f0e9d6aa25..766ef73a2fcf5 100644 --- a/packages/interface/src/components/interface-skeleton/index.js +++ b/packages/interface/src/components/interface-skeleton/index.js @@ -93,6 +93,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__drawer" role="region" aria-label={ mergedLabels.drawer } + tabIndex="-1" > { drawer } @@ -107,6 +108,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__header" role="region" aria-label={ mergedLabels.header } + tabIndex="-1" > { header } @@ -116,6 +118,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__header" role="region" aria-label={ mergedLabels.header } + tabIndex="-1" > { header } @@ -131,6 +134,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__secondary-sidebar" role="region" aria-label={ mergedLabels.secondarySidebar } + tabIndex="-1" > { secondarySidebar } @@ -144,6 +148,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__content" role="region" aria-label={ mergedLabels.body } + tabIndex="-1" > { content } @@ -152,6 +157,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__sidebar" role="region" aria-label={ mergedLabels.sidebar } + tabIndex="-1" > { sidebar } @@ -161,6 +167,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__actions" role="region" aria-label={ mergedLabels.actions } + tabIndex="-1" > { actions } @@ -172,6 +179,7 @@ function InterfaceSkeleton( className="interface-interface-skeleton__footer" role="region" aria-label={ mergedLabels.footer } + tabIndex="-1" > { footer } From 75a566fef36e3798bd7b68889f8052bc7fd389f8 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Mon, 17 Oct 2022 21:36:14 -0500 Subject: [PATCH 06/12] Try fixing E2E tests. --- packages/e2e-tests/specs/editor/various/a11y.test.js | 4 +++- .../specs/editor/various/keyboard-navigable-blocks.test.js | 3 +++ packages/e2e-tests/specs/editor/various/sidebar.test.js | 2 -- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/e2e-tests/specs/editor/various/a11y.test.js b/packages/e2e-tests/specs/editor/various/a11y.test.js index cb9b62dbcdf63..1f87cea42fa3c 100644 --- a/packages/e2e-tests/specs/editor/various/a11y.test.js +++ b/packages/e2e-tests/specs/editor/various/a11y.test.js @@ -16,7 +16,9 @@ describe( 'a11y', () => { it( 'tabs header bar', async () => { await pressKeyWithModifier( 'ctrl', '`' ); - + await pressKeyWithModifier( 'ctrl', '`' ); + await pressKeyWithModifier( 'ctrl', '`' ); + await pressKeyWithModifier( 'ctrl', '`' ); await page.keyboard.press( 'Tab' ); const isFocusedToggle = await page.$eval( diff --git a/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js b/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js index a5297a38f537b..fdebfe18ad72b 100644 --- a/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js +++ b/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js @@ -23,6 +23,9 @@ const navigateToContentEditorTop = async () => { // Use 'Ctrl+`' to return to the top of the editor. await pressKeyWithModifier( 'ctrl', '`' ); await pressKeyWithModifier( 'ctrl', '`' ); + await pressKeyWithModifier( 'ctrl', '`' ); + await pressKeyWithModifier( 'ctrl', '`' ); + await pressKeyWithModifier( 'ctrl', '`' ); }; const tabThroughParagraphBlock = async ( paragraphText ) => { diff --git a/packages/e2e-tests/specs/editor/various/sidebar.test.js b/packages/e2e-tests/specs/editor/various/sidebar.test.js index 39409b0fbeb7b..2e5d46eec2f7a 100644 --- a/packages/e2e-tests/specs/editor/various/sidebar.test.js +++ b/packages/e2e-tests/specs/editor/various/sidebar.test.js @@ -96,8 +96,6 @@ describe( 'Sidebar', () => { // Region navigate to Sidebar. await pressKeyWithModifier( 'ctrl', '`' ); - await pressKeyWithModifier( 'ctrl', '`' ); - await pressKeyWithModifier( 'ctrl', '`' ); // Tab lands at first (presumed selected) option "Post". await page.keyboard.press( 'Tab' ); From e63e73546dc4fd71ad2ffae97a5a573f0a196d0c Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Mon, 17 Oct 2022 21:39:11 -0500 Subject: [PATCH 07/12] Add changelog entry. --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 19b38025d7aa9..b0222ed0b8110 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -34,6 +34,7 @@ - `TabPanel`: Call `onSelect()` on every tab selection, regardless of whether it was triggered by user interaction ([#44028](https://github.com/WordPress/gutenberg/pull/44028)). - `FontSizePicker`: Fallback to font size `slug` if `name` is undefined ([#45041](https://github.com/WordPress/gutenberg/pull/45041)). - `AutocompleterUI`: fix issue where autocompleter UI would appear on top of other UI elements ([#44795](https://github.com/WordPress/gutenberg/pull/44795/)) +- `useNavigateRegions`: Ensure region navigation picks the next region based on where the current user focus is located instead of starting at the beginning. ([#44883](https://github.com/WordPress/gutenberg/pull/44883)) ### Internal From aa7b34534b3d13f4c2788e216f04151991cdddc7 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Tue, 18 Oct 2022 14:28:00 -0500 Subject: [PATCH 08/12] Fix edge case with selector matching regions without our negative tabindex. --- .../components/src/higher-order/navigate-regions/index.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index 4085ead8a4e1f..41b80ca4a1f47 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -44,9 +44,11 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { return; } let nextRegion = regions[ 0 ]; - // Based off the current element, use closest to determine the wrapping region since this operates up the DOM. + // Based off the current element, use closest to determine the wrapping region since this operates up the DOM. Also, match tabindex to avoid edge cases with regions we do not want. const selectedIndex = regions.indexOf( - ref.current.ownerDocument.activeElement.closest( '[role="region"]' ) + ref.current.ownerDocument.activeElement.closest( + '[role="region"][tabindex="-1"]' + ) ); if ( selectedIndex !== -1 ) { let nextIndex = selectedIndex + offset; From 02291fa3f382db6a31100ff76b9950e8c86bb58f Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Tue, 18 Oct 2022 14:42:43 -0500 Subject: [PATCH 09/12] Try again to only select regions with tabindex of -1. Fix E2E. --- packages/components/src/higher-order/navigate-regions/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/higher-order/navigate-regions/index.js b/packages/components/src/higher-order/navigate-regions/index.js index 41b80ca4a1f47..218b9304d3773 100644 --- a/packages/components/src/higher-order/navigate-regions/index.js +++ b/packages/components/src/higher-order/navigate-regions/index.js @@ -38,7 +38,7 @@ export function useNavigateRegions( shortcuts = defaultShortcuts ) { function focusRegion( offset ) { const regions = Array.from( - ref.current.querySelectorAll( '[role="region"]' ) + ref.current.querySelectorAll( '[role="region"][tabindex="-1"]' ) ); if ( ! regions.length ) { return; From faefdc3c0e2278016a411ea50dd3ec213c2b6a6b Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Wed, 26 Oct 2022 10:36:46 -0500 Subject: [PATCH 10/12] Fix E2E. --- .../specs/editor/various/keyboard-navigable-blocks.test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js b/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js index fdebfe18ad72b..221c1b9145f5e 100644 --- a/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js +++ b/packages/e2e-tests/specs/editor/various/keyboard-navigable-blocks.test.js @@ -39,6 +39,10 @@ const tabThroughParagraphBlock = async ( paragraphText ) => { await page.keyboard.press( 'Tab' ); await expect( await getActiveLabel() ).toBe( 'Open document settings' ); + + // Need to shift+tab here to end back in the block. If not, we'll be in the next region and it will only require 4 region jumps instead of 5. + await pressKeyWithModifier( 'shift', 'Tab' ); + await expect( await getActiveLabel() ).toBe( 'Paragraph block' ); }; const tabThroughBlockToolbar = async () => { From 466ada3211aca32b97051b12eccad1d9f1dc0676 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Mon, 14 Nov 2022 07:11:35 -0600 Subject: [PATCH 11/12] Fix E2E. --- test/e2e/specs/editor/various/a11y-region-navigation.spec.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/e2e/specs/editor/various/a11y-region-navigation.spec.js b/test/e2e/specs/editor/various/a11y-region-navigation.spec.js index 7eee32af2b08e..2c28ba311d8c7 100644 --- a/test/e2e/specs/editor/various/a11y-region-navigation.spec.js +++ b/test/e2e/specs/editor/various/a11y-region-navigation.spec.js @@ -22,7 +22,10 @@ test.describe( 'Region navigation (@firefox, @webkit)', () => { attributes: { content: 'Dummy text' }, } ); - // Navigate to first region and check that we made it. + // Navigate to first region and check that we made it. Must navigate forward 4 times as initial focus is placed in post title field. + await page.keyboard.press( 'Control+`' ); + await page.keyboard.press( 'Control+`' ); + await page.keyboard.press( 'Control+`' ); await page.keyboard.press( 'Control+`' ); const editorTopBar = page.locator( 'role=region[name="Editor top bar"i]' From 253ecb29cf22db19a79583241c278c61b8c2fd28 Mon Sep 17 00:00:00 2001 From: Alex Stine Date: Tue, 15 Nov 2022 21:03:41 -0600 Subject: [PATCH 12/12] Fix E2E. --- .../specs/editor/various/block-hierarchy-navigation.test.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/e2e-tests/specs/editor/various/block-hierarchy-navigation.test.js b/packages/e2e-tests/specs/editor/various/block-hierarchy-navigation.test.js index fef0f11eca67a..fa92ac2779bb5 100644 --- a/packages/e2e-tests/specs/editor/various/block-hierarchy-navigation.test.js +++ b/packages/e2e-tests/specs/editor/various/block-hierarchy-navigation.test.js @@ -111,16 +111,14 @@ describe( 'Navigating the block hierarchy', () => { // Move focus to the sidebar area. await pressKeyWithModifier( 'ctrl', '`' ); - await pressKeyWithModifier( 'ctrl', '`' ); - await pressKeyWithModifier( 'ctrl', '`' ); await tabToColumnsControl(); // Tweak the columns count by increasing it by one. await page.keyboard.press( 'ArrowRight' ); // Navigate to the third column in the columns block. - await pressKeyWithModifier( 'ctrl', '`' ); - await pressKeyWithModifier( 'ctrl', '`' ); + await pressKeyWithModifier( 'ctrlShift', '`' ); + await pressKeyWithModifier( 'ctrlShift', '`' ); await pressKeyTimes( 'Tab', 4 ); await pressKeyTimes( 'ArrowDown', 4 ); await page.waitForSelector(