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

Allow Modals to optionally ignore the Close button when determining where to place focus #54296

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
075e405
Allow focus to be selected from list of tabbables via callback
getdave Sep 8, 2023
4e13c67
Make all modals ignore Close button when placing focus
getdave Sep 8, 2023
79d1ad1
Remap existing true value to ignore close button
getdave Sep 8, 2023
ac8c8d4
Optimise firstElement to be biased towards first content element
getdave Sep 11, 2023
12d1e33
Optimise firstElement to be biased towards first content element
getdave Sep 11, 2023
3d51a34
Remove redundant comment
getdave Sep 11, 2023
ae180f3
Make classname stable between usages
getdave Sep 11, 2023
784ea80
Update CHANGELOG
getdave Sep 11, 2023
c23d284
Fix lint by storing function as reference
getdave Sep 12, 2023
fe83a2e
Use more accurate type
getdave Sep 12, 2023
559cc79
Add CHANGELOG entry for Compose package
getdave Sep 12, 2023
e83fa17
Improve Modal README
getdave Sep 12, 2023
23234d6
Optimise firstElement to be biased towards first content element
getdave Sep 11, 2023
22e1d8c
Callback should handle non HTMLElement type
getdave Sep 12, 2023
cbbed8d
Improve Modal README
getdave Sep 12, 2023
fe0f2f1
Update Storybook controls
getdave Sep 13, 2023
ad5ec44
Add focus unit test
getdave Sep 13, 2023
7844085
Mock layout engine to enable focus tests
getdave Sep 13, 2023
254d71e
Include another focusable element to make the test more robust
getdave Sep 13, 2023
65d39bb
Add additional focus management tests
getdave Sep 13, 2023
bf58309
Mock once in describe block
getdave Sep 13, 2023
9fec0cf
Simplify mocking
getdave Sep 13, 2023
e56f4a3
Update e2e test
getdave Sep 13, 2023
f52d193
Improve comment wording
getdave Sep 13, 2023
47f461e
Improve another comment
getdave Sep 13, 2023
3cf68b5
Simplify
getdave Sep 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/block-rename-ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ function RenameModal( { blockName, originalBlockName, onClose, onSave } ) {
aria={ {
describedby: dialogDescription,
} }
focusOnMount="firstElement"
>
<p id={ dialogDescription }>
{ __( 'Enter a custom name for this block.' ) }
Expand Down
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Breaking changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure I would call this a breaking change, maybe more of an enhancement?


- Update `Modal` so that when passing `firstElement` as the `focusOnMount` prop it will optimize for focusing first element within the Modal's _contents_ as opposed to the entire component. ([#54296](https://github.com/WordPress/gutenberg/pull/54296)).

### Enhancements

- Making Circular Option Picker a `listbox`. Note that while this changes some public API, new props are optional, and currently have default values; this will change in another patch ([#52255](https://github.com/WordPress/gutenberg/pull/52255)).
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/modal/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,11 @@ Titles are required for accessibility reasons, see `aria.labelledby` and `title`

#### `focusOnMount`: `boolean | 'firstElement'`

If this property is true, it will focus the first tabbable element rendered in the modal.
If this property is true, it will focus the first tabbable element rendered anywhere within the modal.

If the value `firstElement` is used then the component will attempt to place focus
within the Modal's **contents**, initially skipping focusable nodes within the Modal's header. This is useful
for Modal's which contain immediately focusable elements such as form fields.

- Required: No
- Default: `true`
Expand Down
36 changes: 35 additions & 1 deletion packages/components/src/modal/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,34 @@ import type { ModalProps } from './types';
// Used to count the number of open modals.
let openModalCount = 0;

/**
* When `firstElement` is passed to `focusOnMount`, this function is optimized to
* avoid focusing on the `Close` button (or other "header" elements of the Modal
* and instead focus within the Modal's contents.
* However, if no tabbable elements are found within the Modal's contents, the
* first tabbable element (likely the `Close` button) will be focused instead.
* This ensures that at least one element is focused whilst still optimizing
* for the best a11y experience.
*
* See: https://github.com/WordPress/gutenberg/issues/54106.
* @param tabbables Element[] an array of tabbable elements.
* @return Element the first tabbable element in the Modal contents (or any tabbable element if none are found in content).
*/
function getFirstTabbableElement( tabbables: Element[] ) {
// Attempt to locate tabbable outside of the header portion of the Modal.
const firstContentTabbable = tabbables.find( ( tabbable ) => {
return tabbable.closest( '.components-modal__header' ) === null;
} );

if ( firstContentTabbable ) {
return firstContentTabbable;
}

// Fallback to the first tabbable element anywhere within the Modal.
// Likely the `Close` button.
return tabbables[ 0 ];
getdave marked this conversation as resolved.
Show resolved Hide resolved
}

function UnforwardedModal(
props: ModalProps,
forwardedRef: ForwardedRef< HTMLDivElement >
Expand Down Expand Up @@ -75,7 +103,13 @@ function UnforwardedModal(
const headingId = title
? `components-modal-header-${ instanceId }`
: aria.labelledby;
const focusOnMountRef = useFocusOnMount( focusOnMount );

// If focusOnMount is `firstElement`, Modals should ignore the `Close` button which is the first focusable element.
// Remap `true` to select the next focusable element instead.
const focusOnMountRef = useFocusOnMount(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me if it's already mentioned above but it's a long PR and I'm not on my laptop 😅 .

Could we instead just assign the focusOnMountRef to the <div> with childrenContainerRef on L320 when focusOnMount === 'firstElement'? This way we avoid the use-focusOnMount API change and the awkward CSS class name selector etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would also change the focus behaviour when the focusOnMount prop is set to true, because the children container would gain focus instead of the dialog container — in my opinion, that behaviour is not correct as we'd like to focus the dialog component.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when focusOnMount === 'firstElement'

I mean only change the ref callback to be assigned to the children container div if and only if foucsOnMount === 'firstElement'. Is it still going to be incorrect?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when focusOnMount === 'firstElement'

I mean only change the ref callback to be assigned to the children container div if and only if foucsOnMount === 'firstElement'. Is it still going to be incorrect?

I wouldn't know off the top of my head, but I guess we could try the approach keeping the current set of unit tests and see how it goes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this give us the flexibility to find the first focusable node outside of the content wrapper if there's nothing appropriate within it? Feels like we still need the "global" context, and moving the ref would take that away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! FWIW, I would prefer to add a new type like firstContentElement instead of patching firstElement. It's also clearer for folks that don't read the doc/changelog or have false (but fair) assumptions.

That said, I don't really have a strong preference here. I'm just sharing a suggestion, and I'm okay with whatever solution we end up with! ❤️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to add a new type like firstContentElement instead of patching firstElement

That could also be an option, I wouldn't be opposed to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using firstContentElement would avoid regressions. I'm tempted to try that approach next week alongside the suggestion to conditionally move the ref when firstContentElement is set.

I knew this wouldn't be easy 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You all are doing a great job! Happy to help in any way when I have the capacity! 💪

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevin940726 Alternative now available in #54590

focusOnMount === 'firstElement' ? getFirstTabbableElement : focusOnMount
);
Comment on lines +107 to +109
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note with this change consumers of Modal will have to opt into this behaviour by passing firstElement. The default of Modal is true.


const constrainedTabbingRef = useConstrainedTabbing();
const focusReturnRef = useFocusReturn();
const focusOutsideProps = useFocusOutside( onRequestClose );
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/modal/stories/index.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ const meta: Meta< typeof Modal > = {
control: { type: null },
},
focusOnMount: {
control: { type: 'boolean' },
options: [ true, false, 'firstElement' ],
control: { type: 'select' },
},
role: {
control: { type: 'text' },
Expand Down
174 changes: 174 additions & 0 deletions packages/components/src/modal/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,178 @@ describe( 'Modal', () => {
screen.getByText( 'A sweet button', { selector: 'button' } )
).toBeInTheDocument();
} );

describe( 'Focus handling', () => {
let originalGetClientRects: () => DOMRectList;

beforeEach( () => {
/**
* The test environment does not have a layout engine, so we need to mock
* the getClientRects method. This ensures that the focusable elements can be
* found by the `focusOnMount` logic which depends on layout information
* to determine if the element is visible or not.
* See https://github.com/WordPress/gutenberg/blob/trunk/packages/dom/src/focusable.js#L55-L61.
*/
// @ts-expect-error We're not trying to comply to the DOM spec, only mocking
window.HTMLElement.prototype.getClientRects = function () {
return [ 'trick-jsdom-into-having-size-for-element-rect' ];
};
} );

afterEach( () => {
// Restore original HTMLElement prototype.
// See beforeEach for details.
window.HTMLElement.prototype.getClientRects =
originalGetClientRects;
} );

it( 'should focus the first focusable element in the contents (if found) when `firstElement` passed as value for `focusOnMount` prop', async () => {
const user = userEvent.setup();

const FocusMountDemo = () => {
const [ isShown, setIsShown ] = useState( false );
return (
<>
<button onClick={ () => setIsShown( true ) }>📣</button>
{ isShown && (
<Modal
focusOnMount="firstElement"
onRequestClose={ () => setIsShown( false ) }
>
<p>Modal content</p>
<a
href="https://wordpress.org"
data-testid="first-focusable-element"
>
First Focusable Element
</a>

<a href="https://wordpress.org">
Another Focusable Element
</a>
</Modal>
) }
</>
);
};

render( <FocusMountDemo /> );

const opener = screen.getByRole( 'button' );

await user.click( opener );

expect(
screen.getByTestId( 'first-focusable-element' )
).toHaveFocus();
} );

it( 'should focus the first focusable element anywhere within the dialog when `firstElement` passed as value for `focusOnMount` prop but there is no focusable element in the Modal contents', async () => {
const user = userEvent.setup();

const FocusMountDemo = () => {
const [ isShown, setIsShown ] = useState( false );
return (
<>
<button onClick={ () => setIsShown( true ) }>📣</button>
{ isShown && (
<Modal
focusOnMount="firstElement"
onRequestClose={ () => setIsShown( false ) }
>
<p>Modal content with no focusable elements.</p>
</Modal>
) }
</>
);
};

render( <FocusMountDemo /> );

const opener = screen.getByRole( 'button' );

await user.click( opener );

// The close button is the first focusable element in the dialog.
expect(
screen.getByRole( 'button', {
name: 'Close',
} )
).toHaveFocus();
} );

it( 'should focus the Modal dialog when `true` passed as value for `focusOnMount` prop', async () => {
const user = userEvent.setup();
const FocusMountDemo = () => {
const [ isShown, setIsShown ] = useState( false );
return (
<>
<button onClick={ () => setIsShown( true ) }>📣</button>
{ isShown && (
<Modal
focusOnMount
onRequestClose={ () => setIsShown( false ) }
>
<p>Modal content</p>
<a
href="https://wordpress.org"
data-testid="first-focusable-element"
>
First Focusable Element
</a>

<a href="https://wordpress.org">
Another Focusable Element
</a>
</Modal>
) }
</>
);
};
render( <FocusMountDemo /> );

const opener = screen.getByRole( 'button' );

await user.click( opener );

expect( screen.getByRole( 'dialog' ) ).toHaveFocus();
} );

it( 'should not move focus when `false` passed as value for `focusOnMount` prop', async () => {
const user = userEvent.setup();
const FocusMountDemo = () => {
const [ isShown, setIsShown ] = useState( false );
return (
<>
<button onClick={ () => setIsShown( true ) }>📣</button>
{ isShown && (
<Modal
focusOnMount={ false }
onRequestClose={ () => setIsShown( false ) }
>
<p>Modal content</p>
<a
href="https://wordpress.org"
data-testid="first-focusable-element"
>
First Focusable Element
</a>

<a href="https://wordpress.org">
Another Focusable Element
</a>
</Modal>
) }
</>
);
};
render( <FocusMountDemo /> );

const opener = screen.getByRole( 'button' );

await user.click( opener );

expect( opener ).toHaveFocus();
} );
} );
} );
4 changes: 4 additions & 0 deletions packages/compose/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Breaking Changes

- Update `useFocusOnMount` to allow passing a callback as the primary argument. This allows for consumers to optionally implement custom focus handling. ([#54296](https://github.com/WordPress/gutenberg/pull/54296)).

## 6.18.0 (2023-08-31)

## 6.17.0 (2023-08-16)
Expand Down
2 changes: 1 addition & 1 deletion packages/compose/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ const WithFocusOnMount = () => {

_Parameters_

- _focusOnMount_ `boolean | 'firstElement'`: Focus on mount mode.
- _focusOnMount_ `boolean | 'firstElement' | ((tabbables: Element[]) => Element | null | undefined)`: Focus on mount mode. May optionally be a callback that receives an array of tabbable elements and should return the element to focus.

_Returns_

Expand Down
21 changes: 20 additions & 1 deletion packages/compose/src/hooks/use-focus-on-mount/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { focus } from '@wordpress/dom';
/**
* Hook used to focus the first tabbable element on mount.
*
* @param {boolean | 'firstElement'} focusOnMount Focus on mount mode.
* @param {boolean | 'firstElement' | ((tabbables: Element[]) => Element | null | undefined) } focusOnMount Focus on mount mode. May optionally be a callback that receives an array of tabbable elements and should return the element to focus.
* @return {import('react').RefCallback<HTMLElement>} Ref callback.
*
* @example
Expand Down Expand Up @@ -79,6 +79,25 @@ export default function useFocusOnMount( focusOnMount = 'firstElement' ) {
return;
}

if ( typeof focusOnMountRef?.current === 'function' ) {
getdave marked this conversation as resolved.
Show resolved Hide resolved
// Store a reference to the function to ensure that the
// focusOnMountRef will still hold a reference to a function
// when the timeout fires.
const focusOnMountFunc = focusOnMountRef.current;

timerId.current = setTimeout( () => {
getdave marked this conversation as resolved.
Show resolved Hide resolved
const tabbables = focus.tabbable.find( node );

const elementToFocus = focusOnMountFunc( tabbables );

if ( elementToFocus ) {
setFocus( /** @type {HTMLElement} */ ( elementToFocus ) );
}
}, 0 );

return;
}

setFocus( node );
}, [] );
}
10 changes: 5 additions & 5 deletions test/e2e/specs/editor/various/block-renaming.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,21 +58,21 @@ test.describe( 'Block Renaming', () => {
name: 'Rename',
} );

// Check focus is transferred into modal.
await expect( renameModal ).toBeFocused();

// Check the Modal is perceivable.
await expect( renameModal ).toBeVisible();

const nameInput = renameModal.getByLabel( 'Block name' );

// Check focus is transferred into the input within the Modal.
await expect( nameInput ).toBeFocused();

const saveButton = renameModal.getByRole( 'button', {
name: 'Save',
type: 'submit',
} );

await expect( saveButton ).toBeDisabled();

const nameInput = renameModal.getByLabel( 'Block name' );

await expect( nameInput ).toHaveAttribute( 'placeholder', 'Group' );

await nameInput.fill( 'My new name' );
Expand Down