From 35f4afda50bddfd2028bb3a76a2b10b2d610e2e5 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 15 Apr 2023 12:37:21 +0200 Subject: [PATCH 1/4] Add before / after container buttons --- .../test/tababble-container.tsx | 56 ++++++++++++------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/packages/components/src/navigable-container/test/tababble-container.tsx b/packages/components/src/navigable-container/test/tababble-container.tsx index 88fc10bd98560..c42ee32192aee 100644 --- a/packages/components/src/navigable-container/test/tababble-container.tsx +++ b/packages/components/src/navigable-container/test/tababble-container.tsx @@ -11,18 +11,22 @@ import { TabbableContainer } from '../tabbable'; import type { TabbableContainerProps } from '../types'; const TabbableContainerTestCase = ( props: TabbableContainerProps ) => ( - - - - Item 2 (not tabbable) - - - Item 3 - -

I can not be tabbed

- - Item 4 - + <> + + + + + Item 2 (not tabbable) + + + Item 3 + +

I can not be tabbed

+ + Item 4 + + + ); const getTabbableContainerTabbables = () => [ @@ -57,7 +61,11 @@ describe( 'TabbableContainer', () => { const tabbables = getTabbableContainerTabbables(); - // Move focus to first item. + await user.tab(); + expect( + screen.getByRole( 'button', { name: 'Before container' } ) + ).toHaveFocus(); + await user.tab(); expect( tabbables[ 0 ] ).toHaveFocus(); @@ -91,7 +99,11 @@ describe( 'TabbableContainer', () => { const lastTabbableIndex = tabbables.length - 1; const lastTabbable = tabbables[ lastTabbableIndex ]; - // Move focus to first item. + await user.tab(); + expect( + screen.getByRole( 'button', { name: 'Before container' } ) + ).toHaveFocus(); + await user.tab(); expect( firstTabbable ).toHaveFocus(); @@ -151,21 +163,27 @@ describe( 'TabbableContainer', () => { const tabbables = getTabbableContainerTabbables(); - // Move focus to first item + await user.tab(); + expect( + screen.getByRole( 'button', { name: 'Before container' } ) + ).toHaveFocus(); + expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 0 ); + await user.tab(); expect( tabbables[ 0 ] ).toHaveFocus(); + expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 1 ); await user.keyboard( '[Space]' ); - expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 1 ); + expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 2 ); await user.tab(); - expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 1 ); + expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 2 ); await user.tab( { shift: true } ); // This extra call is caused by the "shift" key being pressed // on its own before "tab" - expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 2 ); + expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 3 ); await user.keyboard( '[Escape]' ); - expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 3 ); + expect( externalWrapperOnKeyDownSpy ).toHaveBeenCalledTimes( 4 ); } ); } ); From f2f0d84f8a5695adef7ffbe024a6d28d389db380 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 15 Apr 2023 14:52:17 +0200 Subject: [PATCH 2/4] Update text with new expectations --- .../test/tababble-container.tsx | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/components/src/navigable-container/test/tababble-container.tsx b/packages/components/src/navigable-container/test/tababble-container.tsx index c42ee32192aee..eb14842025bec 100644 --- a/packages/components/src/navigable-container/test/tababble-container.tsx +++ b/packages/components/src/navigable-container/test/tababble-container.tsx @@ -128,12 +128,17 @@ describe( 'TabbableContainer', () => { /> ); - // With the `cycle` prop set to `false`, cycling is not allowed. // By default, cycling from first to last and from last to first is allowed. + // With the `cycle` prop set to `false`, cycling is not allowed. + // Therefore, focus will escape the `TabbableContainer` and continue its + // natural path in the page. await user.tab( { shift: true } ); - expect( firstTabbable ).toHaveFocus(); + expect( + screen.getByRole( 'button', { name: 'Before container' } ) + ).toHaveFocus(); expect( onNavigateSpy ).toHaveBeenCalledTimes( 2 ); + await user.tab(); await user.tab(); await user.tab(); expect( lastTabbable ).toHaveFocus(); @@ -143,8 +148,12 @@ describe( 'TabbableContainer', () => { lastTabbable ); + // Focus will move to the next natively focusable elements after + // `TabbableContainer` await user.tab(); - expect( lastTabbable ).toHaveFocus(); + expect( + screen.getByRole( 'button', { name: 'After container' } ) + ).toHaveFocus(); expect( onNavigateSpy ).toHaveBeenCalledTimes( 4 ); } ); From 39ac65b52e24829220aee376bd19f0067fab32b9 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sat, 15 Apr 2023 14:52:54 +0200 Subject: [PATCH 3/4] Move `isTab` prevent default only when the component is actually handling the focus --- .../src/navigable-container/container.tsx | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/components/src/navigable-container/container.tsx b/packages/components/src/navigable-container/container.tsx index f52754e06d61a..c5f92f8effcad 100644 --- a/packages/components/src/navigable-container/container.tsx +++ b/packages/components/src/navigable-container/container.tsx @@ -122,11 +122,7 @@ class NavigableContainer extends Component< NavigableContainerProps > { const targetHasMenuItemRole = !! targetRole && MENU_ITEM_ROLES.includes( targetRole ); - // `preventDefault()` on tab to avoid having the browser move the focus - // after this component has already moved it. - const isTab = event.code === 'Tab'; - - if ( targetHasMenuItemRole || isTab ) { + if ( targetHasMenuItemRole ) { event.preventDefault(); } } @@ -150,9 +146,16 @@ class NavigableContainer extends Component< NavigableContainerProps > { const nextIndex = cycle ? cycleValue( index, focusables.length, offset ) : index + offset; + if ( nextIndex >= 0 && nextIndex < focusables.length ) { focusables[ nextIndex ].focus(); onNavigate( nextIndex, focusables[ nextIndex ] ); + + // `preventDefault()` on tab to avoid having the browser move the focus + // after this component has already moved it. + if ( event.code === 'Tab' ) { + event.preventDefault(); + } } } From a91f30816d1227f46196da1c712be23efa6d2c88 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Thu, 27 Apr 2023 15:15:26 +0200 Subject: [PATCH 4/4] CHANGELOG --- packages/components/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 59160a89f036c..d473c14e6a6c0 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -14,6 +14,9 @@ - `onDragStart` in `` is now a synchronous function to allow setting additional data for `event.dataTransfer` ([#49673](https://github.com/WordPress/gutenberg/pull/49673)). +### Bug Fix + +- `NavigableContainer`: do not trap focus in `TabbableContainer` ([#49846](https://github.com/WordPress/gutenberg/pull/49846)). ### Internal