Adding constrainTabbing prop to useDialog hook (#57962)

Adding `constrainTabbing` prop to `useDialog` hook

Tabbing constraint is currently tied to the `focusOnMount` prop in `useDialog'; if `focusOnMount` is not `false`, tabbing will be constrained. Sometimes we want the `focusOnMount` behaviour, without constrained tabbing.

This patch adds a separate `constrainTabbing` prop, which implicitly maintains the current behaviour, taking its default value from `focusOnMount`. Otherwise, if set explicitly, tabbing will be constrained based on the value passed.

packages/components/CHANGELOG.md

@@ -31,6 +31,7 @@
 -   `Tooltip` and `Button`: tidy up unit tests ([#57975](https://github.com/WordPress/gutenberg/pull/57975)).
 -   `BorderControl`, `BorderBoxControl`: Replace style picker with ToggleGroupControl ([#57562](https://github.com/WordPress/gutenberg/pull/57562)).
 -	`SlotFill`: fix typo in use-slot-fills return docs ([#57654](https://github.com/WordPress/gutenberg/pull/57654))
+-	`Popover`: Adding `constrainTabbing` prop to `useDialog` hook ([#57962](https://github.com/WordPress/gutenberg/pull/57962))
 
 ### Bug Fix
 

packages/components/src/popover/index.tsx

@@ -126,6 +126,7 @@ const UnconnectedPopover = (
 	const {
 		animate = true,
 		headerTitle,
+		constrainTabbing,
 		onClose,
 		children,
 		className,
@@ -264,6 +265,7 @@ const UnconnectedPopover = (
 	}
 
 	const [ dialogRef, dialogProps ] = useDialog( {
+		constrainTabbing,
 		focusOnMount,
 		__unstableOnClose: onDialogClose,
 		// @ts-expect-error The __unstableOnClose property needs to be deprecated first (see https://github.com/WordPress/gutenberg/pull/27675)

packages/components/src/popover/test/index.tsx

@@ -2,6 +2,7 @@
  * External dependencies
  */
 import { render, screen, waitFor, getByText } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import type { CSSProperties } from 'react';
 
 /**
@@ -36,6 +37,28 @@ type PlacementToInitialTranslationTuple = [
 	CSSProperties[ 'translate' ],
 ];
 
+beforeAll( () => {
+	// This mock is necessary because deep in the weeds, `useConstrained` relies
+	// on `focusable` to return a list of DOM elements that can be focused. Part
+	// of this process involves checking that an element has an intrinsic size,
+	// which will always fail in JSDom.
+	//
+	// https://github.com/WordPress/gutenberg/blob/trunk/packages/dom/src/focusable.js#L55-L61
+	jest.spyOn(
+		HTMLElement.prototype,
+		'offsetHeight',
+		'get'
+	).mockImplementation( function getOffsetHeight( this: HTMLElement ) {
+		// The `1` returned here is somewhat arbitrary – it just needs to be a
+		// non-zero integer.
+		return 1;
+	} );
+} );
+
+afterAll( () => {
+	jest.restoreAllMocks();
+} );
+
 // There's no matching `placement` for 'middle center' positions,
 // fallback to 'bottom' (same as `floating-ui`'s default.)
 const FALLBACK_FOR_MIDDLE_CENTER_POSITIONS = 'bottom';
@@ -188,6 +211,233 @@ describe( 'Popover', () => {
 				expect( document.body ).toHaveFocus();
 			} );
 		} );
+
+		describe( 'tab constraint behavior', () => {
+			// `constrainTabbing` is implicitly controlled by `focusOnMount`.
+			// By default, when `focusOnMount` is false, `constrainTabbing` will
+			// also be false; otherwise, `constrainTabbing` will be true.
+
+			const setup = async (
+				props?: Partial< React.ComponentProps< typeof Popover > >
+			) => {
+				const user = await userEvent.setup();
+				const view = render(
+					<Popover data-testid="popover-element" { ...props }>
+						<button>Button 1</button>
+						<button>Button 2</button>
+						<button>Button 3</button>
+					</Popover>
+				);
+
+				const popover = screen.getByTestId( 'popover-element' );
+				await waitFor( () => expect( popover ).toBeVisible() );
+
+				const [ firstButton, secondButton, thirdButton ] =
+					screen.getAllByRole( 'button' );
+
+				return {
+					...view,
+					popover,
+					firstButton,
+					secondButton,
+					thirdButton,
+					user,
+				};
+			};
+
+			// Note: due to an issue in testing-library/user-event [1], the
+			// tests for constrained tabbing fail.
+			// [1]: https://github.com/testing-library/user-event/issues/1188
+			//
+			// eslint-disable-next-line jest/no-disabled-tests
+			describe.skip( 'constrains tabbing', () => {
+				test( 'by default', async () => {
+					// The default value for `focusOnMount` is 'firstElement',
+					// which means the default value for `constrainTabbing` is
+					// 'true'.
+
+					const { user, firstButton, secondButton, thirdButton } =
+						await setup();
+
+					await waitFor( () => expect( firstButton ).toHaveFocus() );
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( thirdButton ).toHaveFocus();
+				} );
+
+				test( 'when `focusOnMount` is true', async () => {
+					const {
+						user,
+						popover,
+						firstButton,
+						secondButton,
+						thirdButton,
+					} = await setup( { focusOnMount: true } );
+
+					expect( popover ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( thirdButton ).toHaveFocus();
+				} );
+
+				test( 'when `focusOnMount` is "firstElement"', async () => {
+					const { user, firstButton, secondButton, thirdButton } =
+						await setup( { focusOnMount: 'firstElement' } );
+
+					await waitFor( () => expect( firstButton ).toHaveFocus() );
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( thirdButton ).toHaveFocus();
+				} );
+
+				test( 'when `focusOnMount` is false if `constrainTabbing` is true', async () => {
+					const {
+						user,
+						baseElement,
+						firstButton,
+						secondButton,
+						thirdButton,
+					} = await setup( {
+						focusOnMount: false,
+						constrainTabbing: true,
+					} );
+
+					expect( baseElement ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( thirdButton ).toHaveFocus();
+				} );
+			} );
+
+			describe( 'does not constrain tabbing', () => {
+				test( 'when `constrainTabbing` is false', async () => {
+					// The default value for `focusOnMount` is 'firstElement',
+					// which means the default value for `constrainTabbing` is
+					// 'true', but the provided value should override this.
+
+					const {
+						user,
+						baseElement,
+						firstButton,
+						secondButton,
+						thirdButton,
+					} = await setup( { constrainTabbing: false } );
+
+					await waitFor( () => expect( firstButton ).toHaveFocus() );
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( baseElement ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( baseElement ).toHaveFocus();
+				} );
+
+				test( 'when `focusOnMount` is false', async () => {
+					const {
+						user,
+						baseElement,
+						firstButton,
+						secondButton,
+						thirdButton,
+					} = await setup( { focusOnMount: false } );
+
+					expect( baseElement ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( baseElement ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( baseElement ).toHaveFocus();
+				} );
+
+				test( 'when `focusOnMount` is true if `constrainTabbing` is false', async () => {
+					const {
+						user,
+						baseElement,
+						popover,
+						firstButton,
+						secondButton,
+						thirdButton,
+					} = await setup( {
+						focusOnMount: true,
+						constrainTabbing: false,
+					} );
+
+					expect( popover ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( baseElement ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( baseElement ).toHaveFocus();
+				} );
+
+				test( 'when `focusOnMount` is "firstElement" if `constrainTabbing` is false', async () => {
+					const {
+						user,
+						baseElement,
+						firstButton,
+						secondButton,
+						thirdButton,
+					} = await setup( {
+						focusOnMount: 'firstElement',
+						constrainTabbing: false,
+					} );
+
+					await waitFor( () => expect( firstButton ).toHaveFocus() );
+					await user.tab();
+					expect( secondButton ).toHaveFocus();
+					await user.tab();
+					expect( thirdButton ).toHaveFocus();
+					await user.tab();
+					expect( baseElement ).toHaveFocus();
+					await user.tab();
+					expect( firstButton ).toHaveFocus();
+					await user.tab( { shift: true } );
+					expect( baseElement ).toHaveFocus();
+				} );
+			} );
+		} );
 	} );
 
 	describe( 'Slot outside iframe', () => {

packages/components/src/popover/types.ts

@@ -65,6 +65,15 @@ export type PopoverProps = {
 	 * @default true
 	 */
 	flip?: boolean;
+	/**
+	 * Determines whether tabbing is constrained to within the popover,
+	 * preventing keyboard focus from leaving the popover content without
+	 * explicit focus elswhere, or whether the popover remains part of the wider
+	 * tab order. If no value is passed, it will be derived from `focusOnMount`.
+	 *
+	 * @default `focusOnMount` !== false
+	 */
+	constrainTabbing?: boolean;
 	/**
 	 * By default, the _first tabbable element_ in the popover will receive focus
 	 * when it mounts. This is the same as setting this prop to `"firstElement"`.

packages/compose/src/hooks/use-constrained-tabbing/index.js

@@ -1,7 +1,6 @@
 /**
  * WordPress dependencies
  */
-import { TAB } from '@wordpress/keycodes';
 import { focus } from '@wordpress/dom';
 
 /**
@@ -33,9 +32,9 @@ import useRefEffect from '../use-ref-effect';
 function useConstrainedTabbing() {
 	return useRefEffect( ( /** @type {HTMLElement} */ node ) => {
 		function onKeyDown( /** @type {KeyboardEvent} */ event ) {
-			const { keyCode, shiftKey, target } = event;
+			const { key, shiftKey, target } = event;
 
-			if ( keyCode !== TAB ) {
+			if ( key !== 'Tab' ) {
 				return;
 			}
 

packages/compose/src/hooks/use-dialog/index.ts

@@ -19,7 +19,26 @@ import useFocusOutside from '../use-focus-outside';
 import useMergeRefs from '../use-merge-refs';
 
 type DialogOptions = {
+	/**
+	 * Determines whether focus should be automatically moved to the popover
+	 * when it mounts. `false` causes no focus shift, `true` causes the popover
+	 * itself to gain focus, and `firstElement` focuses the first focusable
+	 * element within the popover.
+	 *
+	 * @default 'firstElement'
+	 */
 	focusOnMount?: Parameters< typeof useFocusOnMount >[ 0 ];
+	/**
+	 * Determines whether tabbing is constrained to within the popover,
+	 * preventing keyboard focus from leaving the popover content without
+	 * explicit focus elsewhere, or whether the popover remains part of the
+	 * wider tab order.
+	 * If no value is passed, it will be derived from `focusOnMount`.
+	 *
+	 * @see focusOnMount
+	 * @default `focusOnMount` !== false
+	 */
+	constrainTabbing?: boolean;
 	onClose?: () => void;
 	/**
 	 * Use the `onClose` prop instead.
@@ -48,6 +67,7 @@ type useDialogReturn = [
  */
 function useDialog( options: DialogOptions ): useDialogReturn {
 	const currentOptions = useRef< DialogOptions | undefined >();
+	const { constrainTabbing = options.focusOnMount !== false } = options;
 	useEffect( () => {
 		currentOptions.current = options;
 	}, Object.values( options ) );
@@ -83,7 +103,7 @@ function useDialog( options: DialogOptions ): useDialogReturn {
 
 	return [
 		useMergeRefs( [
-			options.focusOnMount !== false ? constrainedTabbingRef : null,
+			constrainTabbing ? constrainedTabbingRef : null,
 			options.focusOnMount !== false ? focusReturnRef : null,
 			options.focusOnMount !== false ? focusOnMountRef : null,
 			closeOnEscapeRef,