From e96a222df3246900e4280622bf6413ea6f244215 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 4 Jan 2024 18:15:31 -0500 Subject: [PATCH 01/14] limit focus update to currently selected tab --- packages/components/src/tabs/index.tsx | 52 ++++++++++++++++++++------ 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx index 7f738cb9f08a96..f83922a8c55f04 100644 --- a/packages/components/src/tabs/index.tsx +++ b/packages/components/src/tabs/index.tsx @@ -7,8 +7,13 @@ import * as Ariakit from '@ariakit/react'; /** * WordPress dependencies */ -import { useInstanceId } from '@wordpress/compose'; -import { useLayoutEffect, useMemo, useRef } from '@wordpress/element'; +import { useInstanceId, usePrevious } from '@wordpress/compose'; +import { + useEffect, + useLayoutEffect, + useMemo, + useRef, +} from '@wordpress/element'; /** * Internal dependencies @@ -45,7 +50,7 @@ function Tabs( { const isControlled = selectedTabId !== undefined; const { items, selectedId } = store.useState(); - const { setSelectedId, move } = store; + const { setSelectedId, setActiveId, move } = store; // Keep track of whether tabs have been populated. This is used to prevent // certain effects from firing too early while tab data and relevant @@ -154,26 +159,49 @@ function Tabs( { setSelectedId, ] ); - // In controlled mode, make sure browser focus follows the selected tab if - // the selection is changed while a tab is already being focused. - useLayoutEffect( () => { - if ( ! isControlled || ! selectOnMove ) { + const previousSelectedId = usePrevious( selectedId ); + + useEffect( () => { + if ( ! isControlled ) { return; } const currentItem = items.find( ( item ) => item.id === selectedId ); const activeElement = currentItem?.element?.ownerDocument.activeElement; - const tabsHasFocus = items.some( ( item ) => { - return activeElement && activeElement === item.element; - } ); - if ( + const tabsHasFocus = activeElement && + items.some( ( item ) => { + return activeElement === item.element; + } ); + const previousSelectedTabHadFocus = + previousSelectedId === activeElement?.id; + + // If the previously selected tab had focus when the selection changed, + // move focus to the newly selected tab. + if ( tabsHasFocus && + previousSelectedTabHadFocus && selectedId !== activeElement.id ) { move( selectedId ); + return; } - }, [ isControlled, items, move, selectOnMove, selectedId ] ); + + // If a tab other than the one previously selected had focus when the + // selection changed, update the activeId to the currently focused tab. + // The activeId controls how arrow key navigation behaves. Keeping them + // in sync avoids confusion when navigating tabs with the keyboard. + if ( tabsHasFocus && ! previousSelectedTabHadFocus ) { + setActiveId( activeElement.id ); + } + }, [ + isControlled, + items, + move, + previousSelectedId, + selectedId, + setActiveId, + ] ); const contextValue = useMemo( () => ( { From af1e5fd7e99b2eb53ac9464e4225725862f17a33 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Tue, 16 Jan 2024 16:08:34 -0500 Subject: [PATCH 02/14] update unit tests --- packages/components/src/tabs/test/index.tsx | 99 ++++++++++++--------- 1 file changed, 55 insertions(+), 44 deletions(-) diff --git a/packages/components/src/tabs/test/index.tsx b/packages/components/src/tabs/test/index.tsx index 8c2c2d0fd2fa2a..7962a5e1942c57 100644 --- a/packages/components/src/tabs/test/index.tsx +++ b/packages/components/src/tabs/test/index.tsx @@ -1172,37 +1172,79 @@ describe( 'Tabs', () => { ).not.toBeInTheDocument(); } ); } ); - - describe( 'When `selectOnMove` is `true`', () => { - it( 'should automatically select a newly focused tab', async () => { - render( ); - - await press.Tab(); + describe( 'When `selectedId` is changed by the controlling component', () => { + it( 'should automatically update focus if the selected tab already had focus', async () => { + const { rerender } = render( + + ); // Tab key should focus the currently selected tab, which is Beta. + await press.Tab(); expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); expect( await getSelectedTab() ).toHaveFocus(); - // Arrow keys should select and move focus to the next tab. - await press.ArrowRight(); + rerender( + + ); + + // When the selected tab is changed, it should automatically receive focus. expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); expect( await getSelectedTab() ).toHaveFocus(); } ); - it( 'should automatically update focus when the selected tab is changed by the controlling component', async () => { + it( 'should not automatically update focus if the selected tab was not already focused', async () => { const { rerender } = render( - + ); + expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); + // Tab key should focus the currently selected tab, which is Beta. await press.Tab(); - expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); expect( await getSelectedTab() ).toHaveFocus(); + // Arrow left to focus Alpha, which is not the currently + // selected tab + await press.ArrowLeft(); rerender( - + ); - // When the selected tab is changed, it should automatically receive focus. + // When the selected tab is changed, it should not automatically receive focus. + expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); + expect( + screen.getByRole( 'tab', { name: 'Alpha' } ) + ).toHaveFocus(); + } ); + } ); + + describe( 'When `selectOnMove` is `true`', () => { + it( 'should automatically select a newly focused tab', async () => { + render( ); + + await press.Tab(); + + // Tab key should focus the currently selected tab, which is Beta. + expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); + expect( await getSelectedTab() ).toHaveFocus(); + + // Arrow keys should select and move focus to the next tab. + await press.ArrowRight(); expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); expect( await getSelectedTab() ).toHaveFocus(); } ); @@ -1247,37 +1289,6 @@ describe( 'Tabs', () => { await press.Enter(); expect( await getSelectedTab() ).toHaveTextContent( 'Alpha' ); } ); - it( 'should not automatically update focus when the selected tab is changed by the controlling component', async () => { - const { rerender } = render( - - ); - - expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); - - // Tab key should focus the currently selected tab, which is Beta. - await press.Tab(); - await waitFor( async () => - expect( await getSelectedTab() ).toHaveFocus() - ); - - rerender( - - ); - - // When the selected tab is changed, it should not automatically receive focus. - expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); - expect( - screen.getByRole( 'tab', { name: 'Beta' } ) - ).toHaveFocus(); - } ); } ); } ); it( 'should associate each `Tab` with the correct `TabPanel`, even if they are not rendered in the same order', async () => { From 73a8414b23f5c2ec60c99eff2e47c3155f10fdef Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Tue, 16 Jan 2024 16:12:34 -0500 Subject: [PATCH 03/14] changelog --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 6080498d5e92ba..cbac6dffccea64 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -84,6 +84,7 @@ - `DropdownMenuV2`: remove temporary radix UI based implementation ([#55626](https://github.com/WordPress/gutenberg/pull/55626)). - `Tabs`: do not render hidden content ([#57046](https://github.com/WordPress/gutenberg/pull/57046)). - `Tabs`: improve hover and text alignment styles ([#57275](https://github.com/WordPress/gutenberg/pull/57275)). +- `Tabs`: improve controlled mode focus handling ([#57696](https://github.com/WordPress/gutenberg/pull/57696)). - `Tabs`: make sure `Tab`s are associated to the right `TabPanel`s, regardless of the order they're rendered in ([#57033](https://github.com/WordPress/gutenberg/pull/57033)). ## 25.14.0 (2023-12-13) From b4427ed98969fcd06d47ff34267c69b0da77dd7e Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Tue, 16 Jan 2024 16:40:14 -0500 Subject: [PATCH 04/14] changelog. no, really this time. --- packages/components/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index cbac6dffccea64..4637326a604051 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -45,6 +45,7 @@ - `BoxControl`: Update design ([#56665](https://github.com/WordPress/gutenberg/pull/56665)). - `CustomSelect`: adjust `renderSelectedValue` to fix sizing ([#57865](https://github.com/WordPress/gutenberg/pull/57865)). - `Theme`: Set `color` on wrapper div ([#58095](https://github.com/WordPress/gutenberg/pull/58095)). +- `Tabs`: improve controlled mode focus handling and keyboard navigation ([#57696](https://github.com/WordPress/gutenberg/pull/57696)). ## 25.15.0 (2024-01-10) @@ -84,7 +85,6 @@ - `DropdownMenuV2`: remove temporary radix UI based implementation ([#55626](https://github.com/WordPress/gutenberg/pull/55626)). - `Tabs`: do not render hidden content ([#57046](https://github.com/WordPress/gutenberg/pull/57046)). - `Tabs`: improve hover and text alignment styles ([#57275](https://github.com/WordPress/gutenberg/pull/57275)). -- `Tabs`: improve controlled mode focus handling ([#57696](https://github.com/WordPress/gutenberg/pull/57696)). - `Tabs`: make sure `Tab`s are associated to the right `TabPanel`s, regardless of the order they're rendered in ([#57033](https://github.com/WordPress/gutenberg/pull/57033)). ## 25.14.0 (2023-12-13) From 4745c07d0004b1b741293976c824c3cdeed29167 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 17 Jan 2024 12:51:33 -0500 Subject: [PATCH 05/14] implement feedback --- packages/components/src/tabs/index.tsx | 1 + packages/components/src/tabs/test/index.tsx | 132 +++++++++++--------- 2 files changed, 75 insertions(+), 58 deletions(-) diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx index f83922a8c55f04..cf6443c8bdbebc 100644 --- a/packages/components/src/tabs/index.tsx +++ b/packages/components/src/tabs/index.tsx @@ -174,6 +174,7 @@ function Tabs( { return activeElement === item.element; } ); const previousSelectedTabHadFocus = + typeof previousSelectedId === 'string' && previousSelectedId === activeElement?.id; // If the previously selected tab had focus when the selection changed, diff --git a/packages/components/src/tabs/test/index.tsx b/packages/components/src/tabs/test/index.tsx index 7962a5e1942c57..f8a40b3704a34e 100644 --- a/packages/components/src/tabs/test/index.tsx +++ b/packages/components/src/tabs/test/index.tsx @@ -14,6 +14,7 @@ import { useEffect, useState } from '@wordpress/element'; */ import Tabs from '..'; import type { TabsProps } from '../types'; +import { act } from 'react-dom/test-utils'; type Tab = { tabId: string; @@ -1173,64 +1174,79 @@ describe( 'Tabs', () => { } ); } ); describe( 'When `selectedId` is changed by the controlling component', () => { - it( 'should automatically update focus if the selected tab already had focus', async () => { - const { rerender } = render( - - ); - - // Tab key should focus the currently selected tab, which is Beta. - await press.Tab(); - expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); - expect( await getSelectedTab() ).toHaveFocus(); - - rerender( - - ); - - // When the selected tab is changed, it should automatically receive focus. - expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); - expect( await getSelectedTab() ).toHaveFocus(); - } ); - it( 'should not automatically update focus if the selected tab was not already focused', async () => { - const { rerender } = render( - - ); - - expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); - - // Tab key should focus the currently selected tab, which is Beta. - await press.Tab(); - expect( await getSelectedTab() ).toHaveFocus(); - // Arrow left to focus Alpha, which is not the currently - // selected tab - await press.ArrowLeft(); - - rerender( - - ); - - // When the selected tab is changed, it should not automatically receive focus. - expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); - expect( - screen.getByRole( 'tab', { name: 'Alpha' } ) - ).toHaveFocus(); - } ); + describe.each( [ true, false ] )( + 'When `selectOnMove` is `%s`', + ( selectOnMove ) => { + it( 'should move focus to the newly selected tab if the previously selected tab was focused', async () => { + const { rerender } = render( + + ); + + // Tab key should focus the currently selected tab, which is Beta. + await press.Tab(); + expect( await getSelectedTab() ).toHaveTextContent( + 'Beta' + ); + expect( await getSelectedTab() ).toHaveFocus(); + + rerender( + + ); + + // When the selected tab is changed, it should automatically receive focus. + expect( await getSelectedTab() ).toHaveTextContent( + 'Gamma' + ); + expect( await getSelectedTab() ).toHaveFocus(); + } ); + it( 'should not move focus to the newly selected tab if the previously selected tab was not focused', async () => { + const { rerender } = render( + + ); + + expect( await getSelectedTab() ).toHaveTextContent( + 'Beta' + ); + + // Tab key should focus the currently selected tab, which is Beta. + await press.Tab(); + expect( await getSelectedTab() ).toHaveFocus(); + // Focus Alpha, which is not the currently selected tab + // (not the most elegant way, but it does the job). + act( () => + screen.getByRole( 'tab', { name: 'Alpha' } ).focus() + ); + + rerender( + + ); + + // When the selected tab is changed, it should not automatically receive focus. + expect( await getSelectedTab() ).toHaveTextContent( + 'Gamma' + ); + expect( + screen.getByRole( 'tab', { name: 'Alpha' } ) + ).toHaveFocus(); + } ); + } + ); } ); describe( 'When `selectOnMove` is `true`', () => { From bb55148274db5424ce7eaa13155bdd777db533b5 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 18 Jan 2024 12:09:31 -0500 Subject: [PATCH 06/14] implement feedback --- packages/components/src/tabs/index.tsx | 20 +++++++++---------- .../src/tabs/stories/index.story.tsx | 19 +++++++++++++++--- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx index cf6443c8bdbebc..bf857526dc7900 100644 --- a/packages/components/src/tabs/index.tsx +++ b/packages/components/src/tabs/index.tsx @@ -168,22 +168,20 @@ function Tabs( { const currentItem = items.find( ( item ) => item.id === selectedId ); const activeElement = currentItem?.element?.ownerDocument.activeElement; - const tabsHasFocus = - activeElement && - items.some( ( item ) => { - return activeElement === item.element; - } ); + if ( + ! activeElement || + ! items.some( ( item ) => activeElement === item.element ) + ) { + return; // Return early if no tabs are focused. + } + const previousSelectedTabHadFocus = typeof previousSelectedId === 'string' && previousSelectedId === activeElement?.id; // If the previously selected tab had focus when the selection changed, // move focus to the newly selected tab. - if ( - tabsHasFocus && - previousSelectedTabHadFocus && - selectedId !== activeElement.id - ) { + if ( previousSelectedTabHadFocus && selectedId !== activeElement.id ) { move( selectedId ); return; } @@ -192,7 +190,7 @@ function Tabs( { // selection changed, update the activeId to the currently focused tab. // The activeId controls how arrow key navigation behaves. Keeping them // in sync avoids confusion when navigating tabs with the keyboard. - if ( tabsHasFocus && ! previousSelectedTabHadFocus ) { + if ( ! previousSelectedTabHadFocus ) { setActiveId( activeElement.id ); } }, [ diff --git a/packages/components/src/tabs/stories/index.story.tsx b/packages/components/src/tabs/stories/index.story.tsx index 632aaf0ac9242c..74bf1971b74ea3 100644 --- a/packages/components/src/tabs/stories/index.story.tsx +++ b/packages/components/src/tabs/stories/index.story.tsx @@ -274,17 +274,29 @@ const ControlledModeTemplate: StoryFn< typeof Tabs > = ( props ) => { setSelectedTabId( 'tab1' ), + onClick: () => + setTimeout( + () => setSelectedTabId( 'tab1' ), + 3000 + ), title: 'Tab 1', isActive: selectedTabId === 'tab1', }, { - onClick: () => setSelectedTabId( 'tab2' ), + onClick: () => + setTimeout( + () => setSelectedTabId( 'tab2' ), + 3000 + ), title: 'Tab 2', isActive: selectedTabId === 'tab2', }, { - onClick: () => setSelectedTabId( 'tab3' ), + onClick: () => + setTimeout( + () => setSelectedTabId( 'tab3' ), + 3000 + ), title: 'Tab 3', isActive: selectedTabId === 'tab3', }, @@ -300,6 +312,7 @@ const ControlledModeTemplate: StoryFn< typeof Tabs > = ( props ) => { export const ControlledMode = ControlledModeTemplate.bind( {} ); ControlledMode.args = { selectedTabId: 'tab3', + selectOnMove: false, }; const TabBecomesDisabledTemplate: StoryFn< typeof Tabs > = ( props ) => { From 3791c4fc6ba0125a8bfe9f806bb16166f2f67374 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Fri, 19 Jan 2024 16:26:44 -0500 Subject: [PATCH 07/14] remove testing changes in storybook --- .../src/tabs/stories/index.story.tsx | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/packages/components/src/tabs/stories/index.story.tsx b/packages/components/src/tabs/stories/index.story.tsx index 74bf1971b74ea3..632aaf0ac9242c 100644 --- a/packages/components/src/tabs/stories/index.story.tsx +++ b/packages/components/src/tabs/stories/index.story.tsx @@ -274,29 +274,17 @@ const ControlledModeTemplate: StoryFn< typeof Tabs > = ( props ) => { - setTimeout( - () => setSelectedTabId( 'tab1' ), - 3000 - ), + onClick: () => setSelectedTabId( 'tab1' ), title: 'Tab 1', isActive: selectedTabId === 'tab1', }, { - onClick: () => - setTimeout( - () => setSelectedTabId( 'tab2' ), - 3000 - ), + onClick: () => setSelectedTabId( 'tab2' ), title: 'Tab 2', isActive: selectedTabId === 'tab2', }, { - onClick: () => - setTimeout( - () => setSelectedTabId( 'tab3' ), - 3000 - ), + onClick: () => setSelectedTabId( 'tab3' ), title: 'Tab 3', isActive: selectedTabId === 'tab3', }, @@ -312,7 +300,6 @@ const ControlledModeTemplate: StoryFn< typeof Tabs > = ( props ) => { export const ControlledMode = ControlledModeTemplate.bind( {} ); ControlledMode.args = { selectedTabId: 'tab3', - selectOnMove: false, }; const TabBecomesDisabledTemplate: StoryFn< typeof Tabs > = ( props ) => { From 68ed014087c14fdf7bb1083443b2b37b496e3eed Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Fri, 26 Jan 2024 17:48:12 -0500 Subject: [PATCH 08/14] remove focus updating --- packages/components/src/tabs/index.tsx | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx index bf857526dc7900..7497043f2a5167 100644 --- a/packages/components/src/tabs/index.tsx +++ b/packages/components/src/tabs/index.tsx @@ -179,13 +179,6 @@ function Tabs( { typeof previousSelectedId === 'string' && previousSelectedId === activeElement?.id; - // If the previously selected tab had focus when the selection changed, - // move focus to the newly selected tab. - if ( previousSelectedTabHadFocus && selectedId !== activeElement.id ) { - move( selectedId ); - return; - } - // If a tab other than the one previously selected had focus when the // selection changed, update the activeId to the currently focused tab. // The activeId controls how arrow key navigation behaves. Keeping them From a46c25349f045b2f6dcf1828bd0e89d98bed7908 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 31 Jan 2024 15:06:22 -0500 Subject: [PATCH 09/14] update tests --- packages/components/src/tabs/test/index.tsx | 100 +++++------------- .../various/keyboard-navigable-blocks.spec.js | 2 +- 2 files changed, 27 insertions(+), 75 deletions(-) diff --git a/packages/components/src/tabs/test/index.tsx b/packages/components/src/tabs/test/index.tsx index f8a40b3704a34e..3c006cf6c96786 100644 --- a/packages/components/src/tabs/test/index.tsx +++ b/packages/components/src/tabs/test/index.tsx @@ -14,7 +14,6 @@ import { useEffect, useState } from '@wordpress/element'; */ import Tabs from '..'; import type { TabsProps } from '../types'; -import { act } from 'react-dom/test-utils'; type Tab = { tabId: string; @@ -1174,79 +1173,32 @@ describe( 'Tabs', () => { } ); } ); describe( 'When `selectedId` is changed by the controlling component', () => { - describe.each( [ true, false ] )( - 'When `selectOnMove` is `%s`', - ( selectOnMove ) => { - it( 'should move focus to the newly selected tab if the previously selected tab was focused', async () => { - const { rerender } = render( - - ); - - // Tab key should focus the currently selected tab, which is Beta. - await press.Tab(); - expect( await getSelectedTab() ).toHaveTextContent( - 'Beta' - ); - expect( await getSelectedTab() ).toHaveFocus(); - - rerender( - - ); - - // When the selected tab is changed, it should automatically receive focus. - expect( await getSelectedTab() ).toHaveTextContent( - 'Gamma' - ); - expect( await getSelectedTab() ).toHaveFocus(); - } ); - it( 'should not move focus to the newly selected tab if the previously selected tab was not focused', async () => { - const { rerender } = render( - - ); - - expect( await getSelectedTab() ).toHaveTextContent( - 'Beta' - ); - - // Tab key should focus the currently selected tab, which is Beta. - await press.Tab(); - expect( await getSelectedTab() ).toHaveFocus(); - // Focus Alpha, which is not the currently selected tab - // (not the most elegant way, but it does the job). - act( () => - screen.getByRole( 'tab', { name: 'Alpha' } ).focus() - ); - - rerender( - - ); - - // When the selected tab is changed, it should not automatically receive focus. - expect( await getSelectedTab() ).toHaveTextContent( - 'Gamma' - ); - expect( - screen.getByRole( 'tab', { name: 'Alpha' } ) - ).toHaveFocus(); - } ); - } - ); + it( 'should continue to handle arrow key navigation properly', async () => { + const { rerender } = render( + + ); + + // Tab key should focus the currently selected tab, which is Beta. + await press.Tab(); + expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); + expect( await getSelectedTab() ).toHaveFocus(); + + rerender( + + ); + + // When the selected tab is changed, it should not automatically receive focus. + expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); + expect( + screen.getByRole( 'tab', { name: 'Beta' } ) + ).toHaveFocus(); + + // Arrow keys should move focus to the next tab, which is Gamma + await press.ArrowRight(); + expect( + screen.getByRole( 'tab', { name: 'Gamma' } ) + ).toHaveFocus(); + } ); } ); describe( 'When `selectOnMove` is `true`', () => { 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 84536c88227ce9..6b55f68897cbd0 100644 --- a/test/e2e/specs/editor/various/keyboard-navigable-blocks.spec.js +++ b/test/e2e/specs/editor/various/keyboard-navigable-blocks.spec.js @@ -75,7 +75,7 @@ test.describe( 'Order of block keyboard navigation', () => { ); await page.keyboard.press( 'Tab' ); - await KeyboardNavigableBlocks.expectLabelToHaveFocus( 'Post' ); + await KeyboardNavigableBlocks.expectLabelToHaveFocus( 'Block' ); } ); test( 'allows tabbing in navigation mode if no block is selected (reverse)', async ( { From db954f9f240e2c0ff043542106437aa1161eeb2e Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Wed, 31 Jan 2024 15:09:10 -0500 Subject: [PATCH 10/14] update changelog --- packages/components/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 4637326a604051..1a5fe64aafe3e5 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -17,6 +17,7 @@ - `Guide`, `Modal`: Restore accent color themability ([#58098](https://github.com/WordPress/gutenberg/pull/58098)). - `DropdownMenuV2`: Restore accent color themability ([#58130](https://github.com/WordPress/gutenberg/pull/58130)). +- `Tabs`: improve controlled mode focus handling and keyboard navigation ([#57696](https://github.com/WordPress/gutenberg/pull/57696)). - Expand theming support in the `COLORS` variable object ([#58097](https://github.com/WordPress/gutenberg/pull/58097)). ## 25.16.0 (2024-01-24) @@ -45,7 +46,6 @@ - `BoxControl`: Update design ([#56665](https://github.com/WordPress/gutenberg/pull/56665)). - `CustomSelect`: adjust `renderSelectedValue` to fix sizing ([#57865](https://github.com/WordPress/gutenberg/pull/57865)). - `Theme`: Set `color` on wrapper div ([#58095](https://github.com/WordPress/gutenberg/pull/58095)). -- `Tabs`: improve controlled mode focus handling and keyboard navigation ([#57696](https://github.com/WordPress/gutenberg/pull/57696)). ## 25.15.0 (2024-01-10) From cb5f3824f361d8b25fbdad18c6d00c2f1d36d878 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 1 Feb 2024 16:40:51 +0100 Subject: [PATCH 11/14] Rewrite hook to update the active element, this time using ariakit's state to track the active ID --- packages/components/src/tabs/index.tsx | 41 ++++++++++---------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx index 7497043f2a5167..4f7d547454cc37 100644 --- a/packages/components/src/tabs/index.tsx +++ b/packages/components/src/tabs/index.tsx @@ -7,7 +7,7 @@ import * as Ariakit from '@ariakit/react'; /** * WordPress dependencies */ -import { useInstanceId, usePrevious } from '@wordpress/compose'; +import { useInstanceId } from '@wordpress/compose'; import { useEffect, useLayoutEffect, @@ -49,8 +49,8 @@ function Tabs( { const isControlled = selectedTabId !== undefined; - const { items, selectedId } = store.useState(); - const { setSelectedId, setActiveId, move } = store; + const { items, selectedId, activeId } = store.useState(); + const { setSelectedId, setActiveId } = store; // Keep track of whether tabs have been populated. This is used to prevent // certain effects from firing too early while tab data and relevant @@ -159,41 +159,30 @@ function Tabs( { setSelectedId, ] ); - const previousSelectedId = usePrevious( selectedId ); - useEffect( () => { if ( ! isControlled ) { return; } + const currentItem = items.find( ( item ) => item.id === selectedId ); - const activeElement = currentItem?.element?.ownerDocument.activeElement; + const focusedElement = + currentItem?.element?.ownerDocument.activeElement; if ( - ! activeElement || - ! items.some( ( item ) => activeElement === item.element ) + ! focusedElement || + ! items.some( ( item ) => focusedElement === item.element ) ) { return; // Return early if no tabs are focused. } - const previousSelectedTabHadFocus = - typeof previousSelectedId === 'string' && - previousSelectedId === activeElement?.id; - - // If a tab other than the one previously selected had focus when the - // selection changed, update the activeId to the currently focused tab. - // The activeId controls how arrow key navigation behaves. Keeping them - // in sync avoids confusion when navigating tabs with the keyboard. - if ( ! previousSelectedTabHadFocus ) { - setActiveId( activeElement.id ); + // If, after ariakit re-computes the active tab, that tab doesn't match + // the currently focused tab, then we force an update to ariakit to avoid + // any mismatches, especially when navigating to previous/next tab with + // arrow keys. + if ( activeId !== focusedElement.id ) { + setActiveId( focusedElement.id ); } - }, [ - isControlled, - items, - move, - previousSelectedId, - selectedId, - setActiveId, - ] ); + }, [ activeId, isControlled, items, selectedId, setActiveId ] ); const contextValue = useMemo( () => ( { From 9fa31f13fe3d80e9a9872275789d5cb9a6665837 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 1 Feb 2024 16:41:21 +0100 Subject: [PATCH 12/14] Make sure that keyboard focus goes to the selected tab when tabbing back into the tablist --- packages/components/src/tabs/tablist.tsx | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/components/src/tabs/tablist.tsx b/packages/components/src/tabs/tablist.tsx index 7a53115910796c..afa2d8283e6d6c 100644 --- a/packages/components/src/tabs/tablist.tsx +++ b/packages/components/src/tabs/tablist.tsx @@ -28,11 +28,30 @@ export const TabList = forwardRef< return null; } const { store } = context; + + const { selectedId, activeId, selectOnMove } = store.useState(); + const { setActiveId } = store; + + const onBlur = () => { + if ( ! selectOnMove ) { + return; + } + + // When automatic tab selection is on, make sure that the active tab is up + // to date with the selected tab when leaving the tablist. This makes sure + // that the selected tab will receive keyboard focus when tabbing back into + // the tablist. + if ( selectedId !== activeId ) { + setActiveId( selectedId ); + } + }; + return ( } + onBlur={ onBlur } { ...otherProps } > { children } From 80715ca1b116f4f6e8b547961c9ec61cda514901 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 1 Feb 2024 16:41:28 +0100 Subject: [PATCH 13/14] Update tests --- packages/components/src/tabs/test/index.tsx | 129 ++++++++++++++++---- 1 file changed, 103 insertions(+), 26 deletions(-) diff --git a/packages/components/src/tabs/test/index.tsx b/packages/components/src/tabs/test/index.tsx index 3c006cf6c96786..ca9a01928e599c 100644 --- a/packages/components/src/tabs/test/index.tsx +++ b/packages/components/src/tabs/test/index.tsx @@ -1173,32 +1173,109 @@ describe( 'Tabs', () => { } ); } ); describe( 'When `selectedId` is changed by the controlling component', () => { - it( 'should continue to handle arrow key navigation properly', async () => { - const { rerender } = render( - - ); - - // Tab key should focus the currently selected tab, which is Beta. - await press.Tab(); - expect( await getSelectedTab() ).toHaveTextContent( 'Beta' ); - expect( await getSelectedTab() ).toHaveFocus(); - - rerender( - - ); - - // When the selected tab is changed, it should not automatically receive focus. - expect( await getSelectedTab() ).toHaveTextContent( 'Gamma' ); - expect( - screen.getByRole( 'tab', { name: 'Beta' } ) - ).toHaveFocus(); - - // Arrow keys should move focus to the next tab, which is Gamma - await press.ArrowRight(); - expect( - screen.getByRole( 'tab', { name: 'Gamma' } ) - ).toHaveFocus(); - } ); + describe.each( [ true, false ] )( + 'and `selectOnMove` is %s', + ( selectOnMove ) => { + it( 'should continue to handle arrow key navigation properly', async () => { + const { rerender } = render( + + ); + + // Tab key should focus the currently selected tab, which is Beta. + await press.Tab(); + expect( await getSelectedTab() ).toHaveTextContent( + 'Beta' + ); + expect( await getSelectedTab() ).toHaveFocus(); + + rerender( + + ); + + // When the selected tab is changed, it should not automatically receive focus. + expect( await getSelectedTab() ).toHaveTextContent( + 'Gamma' + ); + expect( + screen.getByRole( 'tab', { name: 'Beta' } ) + ).toHaveFocus(); + + // Arrow keys should move focus to the next tab, which is Gamma + await press.ArrowRight(); + expect( + screen.getByRole( 'tab', { name: 'Gamma' } ) + ).toHaveFocus(); + } ); + + it( 'should focus the correct tab when tabbing out and back into the tablist', async () => { + const { rerender } = render( + <> + + + + ); + + // Tab key should focus the currently selected tab, which is Beta. + await press.Tab(); + await press.Tab(); + expect( await getSelectedTab() ).toHaveTextContent( + 'Beta' + ); + expect( await getSelectedTab() ).toHaveFocus(); + + rerender( + <> + + + + ); + + // When the selected tab is changed, it should not automatically receive focus. + expect( await getSelectedTab() ).toHaveTextContent( + 'Gamma' + ); + expect( + screen.getByRole( 'tab', { name: 'Beta' } ) + ).toHaveFocus(); + + // Press shift+tab, move focus to the button before Tabs + await press.ShiftTab(); + expect( + screen.getByRole( 'button', { name: 'Focus me' } ) + ).toHaveFocus(); + + // Press tab, move focus back to the tablist + await press.Tab(); + + const betaTab = screen.getByRole( 'tab', { + name: 'Beta', + } ); + const gammaTab = screen.getByRole( 'tab', { + name: 'Gamma', + } ); + + expect( + selectOnMove ? gammaTab : betaTab + ).toHaveFocus(); + } ); + } + ); } ); describe( 'When `selectOnMove` is `true`', () => { From 0db55f28fff22fdd470adb1785b5a48c51271bbe Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 1 Feb 2024 17:28:28 +0100 Subject: [PATCH 14/14] Optimize hook dependencies, no need to rely on `selectedId` to get the active element --- packages/components/src/tabs/index.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/components/src/tabs/index.tsx b/packages/components/src/tabs/index.tsx index 4f7d547454cc37..e8e9bff76b3e92 100644 --- a/packages/components/src/tabs/index.tsx +++ b/packages/components/src/tabs/index.tsx @@ -164,9 +164,8 @@ function Tabs( { return; } - const currentItem = items.find( ( item ) => item.id === selectedId ); const focusedElement = - currentItem?.element?.ownerDocument.activeElement; + items?.[ 0 ]?.element?.ownerDocument.activeElement; if ( ! focusedElement || @@ -182,7 +181,7 @@ function Tabs( { if ( activeId !== focusedElement.id ) { setActiveId( focusedElement.id ); } - }, [ activeId, isControlled, items, selectedId, setActiveId ] ); + }, [ activeId, isControlled, items, setActiveId ] ); const contextValue = useMemo( () => ( {