Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding constrainTabbing prop to useDialog hook #57962

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,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

Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ const UnconnectedPopover = (
const {
animate = true,
headerTitle,
constrainTabbing,
onClose,
children,
className,
Expand Down Expand Up @@ -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)
Expand Down
250 changes: 250 additions & 0 deletions packages/components/src/popover/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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', () => {
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
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', () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/components/src/popover/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"`.
Expand Down
5 changes: 2 additions & 3 deletions packages/compose/src/hooks/use-constrained-tabbing/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* WordPress dependencies
*/
import { TAB } from '@wordpress/keycodes';
import { focus } from '@wordpress/dom';

/**
Expand Down Expand Up @@ -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;
}

Expand Down
22 changes: 21 additions & 1 deletion packages/compose/src/hooks/use-dialog/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
andrewhayward marked this conversation as resolved.
Show resolved Hide resolved
onClose?: () => void;
/**
* Use the `onClose` prop instead.
Expand Down Expand Up @@ -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 ) );
Expand Down Expand Up @@ -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,
Expand Down