From 075e40519697eea35e8f2b5a8b0c0afc0c54d968 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Sep 2023 11:30:11 +0100 Subject: [PATCH 01/26] Allow focus to be selected from list of tabbables via callback --- packages/compose/README.md | 2 +- .../src/hooks/use-focus-on-mount/index.js | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/compose/README.md b/packages/compose/README.md index 62ebdef6d798ed..50343458165794 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -315,7 +315,7 @@ const WithFocusOnMount = () => { _Parameters_ -- _focusOnMount_ `boolean | 'firstElement'`: Focus on mount mode. +- _focusOnMount_ `boolean | 'firstElement' | Function`: Focus on mount mode. _Returns_ diff --git a/packages/compose/src/hooks/use-focus-on-mount/index.js b/packages/compose/src/hooks/use-focus-on-mount/index.js index c5176b17e9ceab..3c4413dca9c489 100644 --- a/packages/compose/src/hooks/use-focus-on-mount/index.js +++ b/packages/compose/src/hooks/use-focus-on-mount/index.js @@ -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' | Function} focusOnMount Focus on mount mode. * @return {import('react').RefCallback} Ref callback. * * @example @@ -79,6 +79,20 @@ export default function useFocusOnMount( focusOnMount = 'firstElement' ) { return; } + if ( typeof focusOnMountRef?.current === 'function' ) { + timerId.current = setTimeout( () => { + const tabbables = focus.tabbable.find( node ); + + const elementToFocus = focusOnMountRef.current( tabbables ); + + if ( elementToFocus ) { + setFocus( /** @type {HTMLElement} */ ( elementToFocus ) ); + } + }, 0 ); + + return; + } + setFocus( node ); }, [] ); } From 4e13c67997136f1e8127bbea70d1657f66fcf82e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Sep 2023 11:31:09 +0100 Subject: [PATCH 02/26] Make all modals ignore Close button when placing focus --- packages/components/src/modal/index.tsx | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index d21a3f9ae3535e..40e6eee04d5628 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -39,6 +39,14 @@ import type { ModalProps } from './types'; // Used to count the number of open modals. let openModalCount = 0; +function focusFirstNonCloseButtonElement( tabbables: HTMLElement[] ) { + return tabbables.find( + // Ignore the `Close` button. + // See: https://github.com/WordPress/gutenberg/issues/54106. + ( tabbableNode ) => tabbableNode?.ariaLabel !== 'Close' + ); +} + function UnforwardedModal( props: ModalProps, forwardedRef: ForwardedRef< HTMLDivElement > @@ -47,7 +55,7 @@ function UnforwardedModal( bodyOpenClassName = 'modal-open', role = 'dialog', title = null, - focusOnMount = true, + focusOnMount = focusFirstNonCloseButtonElement, shouldCloseOnEsc = true, shouldCloseOnClickOutside = true, isDismissible = true, From 79d1ad1aa493318f34d5863f740517f0c5bf0f6d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 8 Sep 2023 11:49:44 +0100 Subject: [PATCH 03/26] Remap existing true value to ignore close button --- packages/components/src/modal/index.tsx | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index 40e6eee04d5628..72e7581338c3ae 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -39,10 +39,14 @@ import type { ModalProps } from './types'; // Used to count the number of open modals. let openModalCount = 0; +/** + * Returns the first tabbable element that is not a close button. + * See: https://github.com/WordPress/gutenberg/issues/54106. + * @param tabbables HTMLElement[] an array of tabbable elements. + * @return HTMLElement the first tabbable element that is not a close button. + */ function focusFirstNonCloseButtonElement( tabbables: HTMLElement[] ) { return tabbables.find( - // Ignore the `Close` button. - // See: https://github.com/WordPress/gutenberg/issues/54106. ( tabbableNode ) => tabbableNode?.ariaLabel !== 'Close' ); } @@ -55,7 +59,7 @@ function UnforwardedModal( bodyOpenClassName = 'modal-open', role = 'dialog', title = null, - focusOnMount = focusFirstNonCloseButtonElement, + focusOnMount = true, shouldCloseOnEsc = true, shouldCloseOnClickOutside = true, isDismissible = true, @@ -83,7 +87,13 @@ function UnforwardedModal( const headingId = title ? `components-modal-header-${ instanceId }` : aria.labelledby; - const focusOnMountRef = useFocusOnMount( focusOnMount ); + + // Modals should ignore the `Close` button which is the first focusable element. + // Remap `true` to select the next focusable element instead. + const focusOnMountRef = useFocusOnMount( + focusOnMount === true ? focusFirstNonCloseButtonElement : focusOnMount + ); + const constrainedTabbingRef = useConstrainedTabbing(); const focusReturnRef = useFocusReturn(); const focusOutsideProps = useFocusOutside( onRequestClose ); From ac8c8d482df0d5299ecb470e7448749351618f65 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 11 Sep 2023 11:51:48 +0100 Subject: [PATCH 04/26] Optimise firstElement to be biased towards first content element --- .../block-editor/src/hooks/block-rename-ui.js | 1 + packages/components/src/modal/README.md | 6 +++- packages/components/src/modal/index.tsx | 28 +++++++++++++++---- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/packages/block-editor/src/hooks/block-rename-ui.js b/packages/block-editor/src/hooks/block-rename-ui.js index 835a09556aed5e..ec13198f4dd701 100644 --- a/packages/block-editor/src/hooks/block-rename-ui.js +++ b/packages/block-editor/src/hooks/block-rename-ui.js @@ -69,6 +69,7 @@ function RenameModal( { blockName, originalBlockName, onClose, onSave } ) { aria={ { describedby: dialogDescription, } } + focusOnMount="firstElement" >

{ __( 'Enter a custom name for this block.' ) } diff --git a/packages/components/src/modal/README.md b/packages/components/src/modal/README.md index 01cad7d6ff2e0f..7fa937461b8e97 100644 --- a/packages/components/src/modal/README.md +++ b/packages/components/src/modal/README.md @@ -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**, ignoring 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` diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index 72e7581338c3ae..7fe04402dfde1a 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -40,15 +40,31 @@ import type { ModalProps } from './types'; let openModalCount = 0; /** - * Returns the first tabbable element that is not a close button. + * When `firstElement` is passed to `focusOnMount`, this function is optimised 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 optimising + * for the best a11y experience. + * * See: https://github.com/WordPress/gutenberg/issues/54106. * @param tabbables HTMLElement[] an array of tabbable elements. * @return HTMLElement the first tabbable element that is not a close button. */ -function focusFirstNonCloseButtonElement( tabbables: HTMLElement[] ) { - return tabbables.find( - ( tabbableNode ) => tabbableNode?.ariaLabel !== 'Close' - ); +function getFirstTabbableElement( tabbables: HTMLElement[] ) { + // 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 ]; } function UnforwardedModal( @@ -91,7 +107,7 @@ function UnforwardedModal( // Modals should ignore the `Close` button which is the first focusable element. // Remap `true` to select the next focusable element instead. const focusOnMountRef = useFocusOnMount( - focusOnMount === true ? focusFirstNonCloseButtonElement : focusOnMount + focusOnMount === 'firstElement' ? getFirstTabbableElement : focusOnMount ); const constrainedTabbingRef = useConstrainedTabbing(); From 12d1e33155f6d0fcd07b2ad1325ce1e74724fba9 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 11 Sep 2023 12:01:09 +0100 Subject: [PATCH 05/26] Optimise firstElement to be biased towards first content element --- packages/compose/src/hooks/use-focus-on-mount/index.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/compose/src/hooks/use-focus-on-mount/index.js b/packages/compose/src/hooks/use-focus-on-mount/index.js index 3c4413dca9c489..c23379d939bd57 100644 --- a/packages/compose/src/hooks/use-focus-on-mount/index.js +++ b/packages/compose/src/hooks/use-focus-on-mount/index.js @@ -83,6 +83,9 @@ export default function useFocusOnMount( focusOnMount = 'firstElement' ) { timerId.current = setTimeout( () => { const tabbables = focus.tabbable.find( node ); + // Ignoring next line because the type is actually callable, + // but the linter cannot recognise the outer typeof check. + // @ts-ignore const elementToFocus = focusOnMountRef.current( tabbables ); if ( elementToFocus ) { From 3d51a34be69e2b4cbb24e75470ff0bd102013850 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 11 Sep 2023 12:03:26 +0100 Subject: [PATCH 06/26] Remove redundant comment --- packages/components/src/modal/index.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index 7fe04402dfde1a..67b78a63c26a67 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -104,8 +104,6 @@ function UnforwardedModal( ? `components-modal-header-${ instanceId }` : aria.labelledby; - // Modals should ignore the `Close` button which is the first focusable element. - // Remap `true` to select the next focusable element instead. const focusOnMountRef = useFocusOnMount( focusOnMount === 'firstElement' ? getFirstTabbableElement : focusOnMount ); From ae180f360a2182fdc6cfaf493a890e64d083772d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 11 Sep 2023 12:05:31 +0100 Subject: [PATCH 07/26] Make classname stable between usages --- packages/components/src/modal/index.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index 67b78a63c26a67..a19d4b0f98d362 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -39,6 +39,8 @@ import type { ModalProps } from './types'; // Used to count the number of open modals. let openModalCount = 0; +const MODAL_HEADER_CLASSNAME = 'components-modal__header'; + /** * When `firstElement` is passed to `focusOnMount`, this function is optimised to * avoid focusing on the `Close` button (or other "header" elements of the Modal @@ -55,7 +57,7 @@ let openModalCount = 0; function getFirstTabbableElement( tabbables: HTMLElement[] ) { // Attempt to locate tabbable outside of the header portion of the Modal. const firstContentTabbable = tabbables.find( ( tabbable ) => { - return tabbable.closest( '.components-modal__header' ) === null; + return tabbable.closest( `.${ MODAL_HEADER_CLASSNAME }` ) === null; } ); if ( firstContentTabbable ) { @@ -284,7 +286,7 @@ function UnforwardedModal( tabIndex={ hasScrollableContent ? 0 : undefined } > { ! __experimentalHideHeader && ( -

+
{ icon && ( Date: Mon, 11 Sep 2023 12:13:13 +0100 Subject: [PATCH 08/26] Update CHANGELOG --- packages/components/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index d0e758045f4817..89cd1e7f0212ba 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Breaking changes + +- 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)). From c23d2840f43e1e4db6a3d839ebbe82853c154210 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 12 Sep 2023 10:45:52 +0100 Subject: [PATCH 09/26] Fix lint by storing function as reference --- packages/compose/src/hooks/use-focus-on-mount/index.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/compose/src/hooks/use-focus-on-mount/index.js b/packages/compose/src/hooks/use-focus-on-mount/index.js index c23379d939bd57..0ef54c308fd82c 100644 --- a/packages/compose/src/hooks/use-focus-on-mount/index.js +++ b/packages/compose/src/hooks/use-focus-on-mount/index.js @@ -80,13 +80,15 @@ export default function useFocusOnMount( focusOnMount = 'firstElement' ) { } if ( typeof focusOnMountRef?.current === 'function' ) { + // 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( () => { const tabbables = focus.tabbable.find( node ); - // Ignoring next line because the type is actually callable, - // but the linter cannot recognise the outer typeof check. - // @ts-ignore - const elementToFocus = focusOnMountRef.current( tabbables ); + const elementToFocus = focusOnMountFunc( tabbables ); if ( elementToFocus ) { setFocus( /** @type {HTMLElement} */ ( elementToFocus ) ); From fe83a2ebb22a65ff929fcfc2dbb16ab347a306ce Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 12 Sep 2023 10:48:30 +0100 Subject: [PATCH 10/26] Use more accurate type --- packages/compose/README.md | 2 +- packages/compose/src/hooks/use-focus-on-mount/index.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/compose/README.md b/packages/compose/README.md index 50343458165794..4d3c78baea41e5 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -315,7 +315,7 @@ const WithFocusOnMount = () => { _Parameters_ -- _focusOnMount_ `boolean | 'firstElement' | Function`: 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_ diff --git a/packages/compose/src/hooks/use-focus-on-mount/index.js b/packages/compose/src/hooks/use-focus-on-mount/index.js index 0ef54c308fd82c..3829616562af0a 100644 --- a/packages/compose/src/hooks/use-focus-on-mount/index.js +++ b/packages/compose/src/hooks/use-focus-on-mount/index.js @@ -7,7 +7,7 @@ import { focus } from '@wordpress/dom'; /** * Hook used to focus the first tabbable element on mount. * - * @param {boolean | 'firstElement' | Function} 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} Ref callback. * * @example From 559cc793e1c201678d26290584e53e5e85534aea Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 12 Sep 2023 10:51:20 +0100 Subject: [PATCH 11/26] Add CHANGELOG entry for Compose package --- packages/compose/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/compose/CHANGELOG.md b/packages/compose/CHANGELOG.md index ab2f98424481d5..ce41a813f65ac2 100644 --- a/packages/compose/CHANGELOG.md +++ b/packages/compose/CHANGELOG.md @@ -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) From e83fa17fe5a810703a2f147d6741b314309dbfe9 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 12 Sep 2023 10:53:12 +0100 Subject: [PATCH 12/26] Improve Modal README Co-authored-by: Marco Ciampini --- packages/components/src/modal/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/modal/README.md b/packages/components/src/modal/README.md index 7fa937461b8e97..944fc6c384db8e 100644 --- a/packages/components/src/modal/README.md +++ b/packages/components/src/modal/README.md @@ -192,7 +192,7 @@ Titles are required for accessibility reasons, see `aria.labelledby` and `title` 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**, ignoring focusable nodes within the Modal's header. This is useful +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 From 23234d637dbed2d5978970b692385b05da53e924 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Mon, 11 Sep 2023 11:51:48 +0100 Subject: [PATCH 13/26] Optimise firstElement to be biased towards first content element --- packages/components/src/modal/README.md | 2 +- packages/components/src/modal/index.tsx | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/components/src/modal/README.md b/packages/components/src/modal/README.md index 944fc6c384db8e..7fa937461b8e97 100644 --- a/packages/components/src/modal/README.md +++ b/packages/components/src/modal/README.md @@ -192,7 +192,7 @@ Titles are required for accessibility reasons, see `aria.labelledby` and `title` 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 +within the Modal's **contents**, ignoring focusable nodes within the Modal's header. This is useful for Modal's which contain immediately focusable elements such as form fields. - Required: No diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index a19d4b0f98d362..7fe04402dfde1a 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -39,8 +39,6 @@ import type { ModalProps } from './types'; // Used to count the number of open modals. let openModalCount = 0; -const MODAL_HEADER_CLASSNAME = 'components-modal__header'; - /** * When `firstElement` is passed to `focusOnMount`, this function is optimised to * avoid focusing on the `Close` button (or other "header" elements of the Modal @@ -57,7 +55,7 @@ const MODAL_HEADER_CLASSNAME = 'components-modal__header'; function getFirstTabbableElement( tabbables: HTMLElement[] ) { // Attempt to locate tabbable outside of the header portion of the Modal. const firstContentTabbable = tabbables.find( ( tabbable ) => { - return tabbable.closest( `.${ MODAL_HEADER_CLASSNAME }` ) === null; + return tabbable.closest( '.components-modal__header' ) === null; } ); if ( firstContentTabbable ) { @@ -106,6 +104,8 @@ function UnforwardedModal( ? `components-modal-header-${ instanceId }` : aria.labelledby; + // Modals should ignore the `Close` button which is the first focusable element. + // Remap `true` to select the next focusable element instead. const focusOnMountRef = useFocusOnMount( focusOnMount === 'firstElement' ? getFirstTabbableElement : focusOnMount ); @@ -286,7 +286,7 @@ function UnforwardedModal( tabIndex={ hasScrollableContent ? 0 : undefined } > { ! __experimentalHideHeader && ( -
+
{ icon && ( Date: Tue, 12 Sep 2023 11:05:47 +0100 Subject: [PATCH 14/26] Callback should handle non HTMLElement type --- packages/components/src/modal/index.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index 7fe04402dfde1a..d71bdea33364a7 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -49,10 +49,10 @@ let openModalCount = 0; * for the best a11y experience. * * See: https://github.com/WordPress/gutenberg/issues/54106. - * @param tabbables HTMLElement[] an array of tabbable elements. - * @return HTMLElement the first tabbable element that is not a close button. + * @param tabbables Element[] an array of tabbable elements. + * @return Element the first tabbable element that is not a close button. */ -function getFirstTabbableElement( tabbables: HTMLElement[] ) { +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; From cbbed8dbbad55615216022ec521d7c54717953d4 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 12 Sep 2023 11:19:37 +0100 Subject: [PATCH 15/26] Improve Modal README --- packages/components/src/modal/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/modal/README.md b/packages/components/src/modal/README.md index 7fa937461b8e97..944fc6c384db8e 100644 --- a/packages/components/src/modal/README.md +++ b/packages/components/src/modal/README.md @@ -192,7 +192,7 @@ Titles are required for accessibility reasons, see `aria.labelledby` and `title` 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**, ignoring focusable nodes within the Modal's header. This is useful +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 From fe0f2f1bedeba8d8f502d0328aaa95cf6646afa6 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:10:44 +0100 Subject: [PATCH 16/26] Update Storybook controls --- packages/components/src/modal/stories/index.story.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/components/src/modal/stories/index.story.tsx b/packages/components/src/modal/stories/index.story.tsx index 8405a6eb0113e3..fe43db1ad691db 100644 --- a/packages/components/src/modal/stories/index.story.tsx +++ b/packages/components/src/modal/stories/index.story.tsx @@ -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' }, From ad5ec44cd96ae89a2af36dc2bd411ab132555da0 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:13:47 +0100 Subject: [PATCH 17/26] Add focus unit test --- packages/components/src/modal/test/index.tsx | 35 ++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/components/src/modal/test/index.tsx b/packages/components/src/modal/test/index.tsx index c2ab277f721570..f57f2aab870663 100644 --- a/packages/components/src/modal/test/index.tsx +++ b/packages/components/src/modal/test/index.tsx @@ -129,4 +129,39 @@ describe( 'Modal', () => { screen.getByText( 'A sweet button', { selector: 'button' } ) ).toBeInTheDocument(); } ); + + describe( 'Focus handling', () => { + it( 'should focus the first focusable element in the contents when `firstElement` passed as value for `focusOnMount` prop', async () => { + const user = userEvent.setup(); + const FocusMountDemo = () => { + const [ isShown, setIsShown ] = useState( false ); + return ( + <> + + { isShown && ( + setIsShown( false ) } + > +

Modal content

+ + Button + + + ) } + + ); + }; + render( ); + + const opener = screen.getByRole( 'button' ); + + await user.click( opener ); + + expect( screen.getByTestId( 'button-with-focus' ) ).toHaveFocus(); + } ); + } ); } ); From 7844085a865d50ebf74952235c0c03c3fbe377fb Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:26:28 +0100 Subject: [PATCH 18/26] Mock layout engine to enable focus tests --- packages/components/src/modal/test/index.tsx | 54 ++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/packages/components/src/modal/test/index.tsx b/packages/components/src/modal/test/index.tsx index f57f2aab870663..8d67c424089dd8 100644 --- a/packages/components/src/modal/test/index.tsx +++ b/packages/components/src/modal/test/index.tsx @@ -132,6 +132,44 @@ describe( 'Modal', () => { describe( 'Focus handling', () => { it( 'should focus the first focusable element in the contents when `firstElement` passed as value for `focusOnMount` prop', async () => { + const originalOffsetWidth = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + 'offsetWidth' + ); + + const originalOffsetHeight = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + 'offsetHeight' + ); + + const originalGetClientRects = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + 'getClientRects' + ); + + /** + * The test environment does not have a layout engine, so we need to mock + * the offsetWidth, offsetHeight and getClientRects methods to return a + * value that is not 0. 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. + */ + Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { + configurable: true, + value: 100, + } ); + + Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: 100, + } ); + + Object.defineProperty( HTMLElement.prototype, 'getClientRects', { + configurable: true, + value: () => [ 1, 2, 3 ], + } ); + const user = userEvent.setup(); const FocusMountDemo = () => { const [ isShown, setIsShown ] = useState( false ); @@ -162,6 +200,22 @@ describe( 'Modal', () => { await user.click( opener ); expect( screen.getByTestId( 'button-with-focus' ) ).toHaveFocus(); + + // Restore original HTMLElement prototype + Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { + configurable: true, + value: originalOffsetWidth, + } ); + + Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: originalOffsetHeight, + } ); + + Object.defineProperty( HTMLElement.prototype, 'getClientRects', { + configurable: true, + value: originalGetClientRects, + } ); } ); } ); } ); From 254d71ef10b30604e3d61b397bee837a0e7f7f56 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:29:09 +0100 Subject: [PATCH 19/26] Include another focusable element to make the test more robust --- packages/components/src/modal/test/index.tsx | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/components/src/modal/test/index.tsx b/packages/components/src/modal/test/index.tsx index 8d67c424089dd8..bc13b7a58868d4 100644 --- a/packages/components/src/modal/test/index.tsx +++ b/packages/components/src/modal/test/index.tsx @@ -184,9 +184,13 @@ describe( 'Modal', () => {

Modal content

- Button + First Focusable Element + + + + Another Focusable Element ) } @@ -199,7 +203,9 @@ describe( 'Modal', () => { await user.click( opener ); - expect( screen.getByTestId( 'button-with-focus' ) ).toHaveFocus(); + expect( + screen.getByTestId( 'first-focusable-element' ) + ).toHaveFocus(); // Restore original HTMLElement prototype Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { From 65d39bbdc9866b992195d12e87616a4bfb80a9e3 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:39:17 +0100 Subject: [PATCH 20/26] Add additional focus management tests --- packages/components/src/modal/test/index.tsx | 166 ++++++++++++++++++- 1 file changed, 165 insertions(+), 1 deletion(-) diff --git a/packages/components/src/modal/test/index.tsx b/packages/components/src/modal/test/index.tsx index bc13b7a58868d4..80b9ad54caa8a9 100644 --- a/packages/components/src/modal/test/index.tsx +++ b/packages/components/src/modal/test/index.tsx @@ -131,7 +131,7 @@ describe( 'Modal', () => { } ); describe( 'Focus handling', () => { - it( 'should focus the first focusable element in the contents when `firstElement` passed as value for `focusOnMount` prop', async () => { + it( 'should focus the first focusable element in the contents (if found) when `firstElement` passed as value for `focusOnMount` prop', async () => { const originalOffsetWidth = Object.getOwnPropertyDescriptor( HTMLElement.prototype, 'offsetWidth' @@ -171,6 +171,7 @@ describe( 'Modal', () => { } ); const user = userEvent.setup(); + const FocusMountDemo = () => { const [ isShown, setIsShown ] = useState( false ); return ( @@ -197,6 +198,7 @@ describe( 'Modal', () => { ); }; + render( ); const opener = screen.getByRole( 'button' ); @@ -223,5 +225,167 @@ describe( 'Modal', () => { value: originalGetClientRects, } ); } ); + + 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 originalOffsetWidth = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + 'offsetWidth' + ); + + const originalOffsetHeight = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + 'offsetHeight' + ); + + const originalGetClientRects = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + 'getClientRects' + ); + + /** + * The test environment does not have a layout engine, so we need to mock + * the offsetWidth, offsetHeight and getClientRects methods to return a + * value that is not 0. 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. + */ + Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { + configurable: true, + value: 100, + } ); + + Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: 100, + } ); + + Object.defineProperty( HTMLElement.prototype, 'getClientRects', { + configurable: true, + value: () => [ 1, 2, 3 ], + } ); + + const user = userEvent.setup(); + + const FocusMountDemo = () => { + const [ isShown, setIsShown ] = useState( false ); + return ( + <> + + { isShown && ( + setIsShown( false ) } + > +

Modal content with no focusable elements.

+ + ) } + + ); + }; + + render( ); + + 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(); + + // Restore original HTMLElement prototype + Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { + configurable: true, + value: originalOffsetWidth, + } ); + + Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: originalOffsetHeight, + } ); + + Object.defineProperty( HTMLElement.prototype, 'getClientRects', { + configurable: true, + value: originalGetClientRects, + } ); + } ); + + 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 ( + <> + + { isShown && ( + setIsShown( false ) } + > +

Modal content

+ + First Focusable Element + + + + Another Focusable Element + + + ) } + + ); + }; + render( ); + + 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 ( + <> + + { isShown && ( + setIsShown( false ) } + > +

Modal content

+ + First Focusable Element + + + + Another Focusable Element + + + ) } + + ); + }; + render( ); + + const opener = screen.getByRole( 'button' ); + + await user.click( opener ); + + expect( opener ).toHaveFocus(); + } ); } ); } ); From bf58309e64edfa1a6c10287e3df86b02ea37c944 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:52:00 +0100 Subject: [PATCH 21/26] Mock once in describe block --- packages/components/src/modal/test/index.tsx | 119 ++++++------------- 1 file changed, 37 insertions(+), 82 deletions(-) diff --git a/packages/components/src/modal/test/index.tsx b/packages/components/src/modal/test/index.tsx index 80b9ad54caa8a9..af8bd86edcf8f1 100644 --- a/packages/components/src/modal/test/index.tsx +++ b/packages/components/src/modal/test/index.tsx @@ -131,30 +131,34 @@ describe( 'Modal', () => { } ); describe( 'Focus handling', () => { - it( 'should focus the first focusable element in the contents (if found) when `firstElement` passed as value for `focusOnMount` prop', async () => { - const originalOffsetWidth = Object.getOwnPropertyDescriptor( + let originalOffsetWidth: PropertyDescriptor | undefined; + let originalOffsetHeight: PropertyDescriptor | undefined; + let originalGetClientRects: PropertyDescriptor | undefined; + + beforeEach( () => { + /** + * The test environment does not have a layout engine, so we need to mock + * the offsetWidth, offsetHeight and getClientRects methods to return a + * value that is not 0. 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. + */ + originalOffsetWidth = Object.getOwnPropertyDescriptor( HTMLElement.prototype, 'offsetWidth' ); - const originalOffsetHeight = Object.getOwnPropertyDescriptor( + originalOffsetHeight = Object.getOwnPropertyDescriptor( HTMLElement.prototype, 'offsetHeight' ); - const originalGetClientRects = Object.getOwnPropertyDescriptor( + originalGetClientRects = Object.getOwnPropertyDescriptor( HTMLElement.prototype, 'getClientRects' ); - /** - * The test environment does not have a layout engine, so we need to mock - * the offsetWidth, offsetHeight and getClientRects methods to return a - * value that is not 0. 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. - */ Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { configurable: true, value: 100, @@ -169,7 +173,28 @@ describe( 'Modal', () => { configurable: true, value: () => [ 1, 2, 3 ], } ); + } ); + afterEach( () => { + // Restore original HTMLElement prototype. + // See beforeEach for details. + Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { + configurable: true, + value: originalOffsetWidth, + } ); + + Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: originalOffsetHeight, + } ); + + Object.defineProperty( HTMLElement.prototype, 'getClientRects', { + configurable: true, + value: 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 = () => { @@ -208,63 +233,9 @@ describe( 'Modal', () => { expect( screen.getByTestId( 'first-focusable-element' ) ).toHaveFocus(); - - // Restore original HTMLElement prototype - Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { - configurable: true, - value: originalOffsetWidth, - } ); - - Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: originalOffsetHeight, - } ); - - Object.defineProperty( HTMLElement.prototype, 'getClientRects', { - configurable: true, - value: originalGetClientRects, - } ); } ); 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 originalOffsetWidth = Object.getOwnPropertyDescriptor( - HTMLElement.prototype, - 'offsetWidth' - ); - - const originalOffsetHeight = Object.getOwnPropertyDescriptor( - HTMLElement.prototype, - 'offsetHeight' - ); - - const originalGetClientRects = Object.getOwnPropertyDescriptor( - HTMLElement.prototype, - 'getClientRects' - ); - - /** - * The test environment does not have a layout engine, so we need to mock - * the offsetWidth, offsetHeight and getClientRects methods to return a - * value that is not 0. 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. - */ - Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { - configurable: true, - value: 100, - } ); - - Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: 100, - } ); - - Object.defineProperty( HTMLElement.prototype, 'getClientRects', { - configurable: true, - value: () => [ 1, 2, 3 ], - } ); - const user = userEvent.setup(); const FocusMountDemo = () => { @@ -296,22 +267,6 @@ describe( 'Modal', () => { name: 'Close', } ) ).toHaveFocus(); - - // Restore original HTMLElement prototype - Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { - configurable: true, - value: originalOffsetWidth, - } ); - - Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: originalOffsetHeight, - } ); - - Object.defineProperty( HTMLElement.prototype, 'getClientRects', { - configurable: true, - value: originalGetClientRects, - } ); } ); it( 'should focus the Modal dialog when `true` passed as value for `focusOnMount` prop', async () => { From 9fec0cf4304f37e8c776dd78e3568ffad01c67d6 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 10:58:58 +0100 Subject: [PATCH 22/26] Simplify mocking --- packages/components/src/modal/test/index.tsx | 56 +++----------------- 1 file changed, 8 insertions(+), 48 deletions(-) diff --git a/packages/components/src/modal/test/index.tsx b/packages/components/src/modal/test/index.tsx index af8bd86edcf8f1..93027af900c1ae 100644 --- a/packages/components/src/modal/test/index.tsx +++ b/packages/components/src/modal/test/index.tsx @@ -131,67 +131,27 @@ describe( 'Modal', () => { } ); describe( 'Focus handling', () => { - let originalOffsetWidth: PropertyDescriptor | undefined; - let originalOffsetHeight: PropertyDescriptor | undefined; - let originalGetClientRects: PropertyDescriptor | undefined; + let originalGetClientRects: () => DOMRectList; beforeEach( () => { /** * The test environment does not have a layout engine, so we need to mock - * the offsetWidth, offsetHeight and getClientRects methods to return a - * value that is not 0. This ensures that the focusable elements can be + * 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. */ - originalOffsetWidth = Object.getOwnPropertyDescriptor( - HTMLElement.prototype, - 'offsetWidth' - ); - - originalOffsetHeight = Object.getOwnPropertyDescriptor( - HTMLElement.prototype, - 'offsetHeight' - ); - - originalGetClientRects = Object.getOwnPropertyDescriptor( - HTMLElement.prototype, - 'getClientRects' - ); - - Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { - configurable: true, - value: 100, - } ); - - Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: 100, - } ); - - Object.defineProperty( HTMLElement.prototype, 'getClientRects', { - configurable: true, - value: () => [ 1, 2, 3 ], - } ); + // @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. - Object.defineProperty( HTMLElement.prototype, 'offsetWidth', { - configurable: true, - value: originalOffsetWidth, - } ); - - Object.defineProperty( HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: originalOffsetHeight, - } ); - - Object.defineProperty( HTMLElement.prototype, 'getClientRects', { - configurable: true, - value: originalGetClientRects, - } ); + 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 () => { From e56f4a371cf151ef953cae2d533cec5e3d0a8271 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 11:28:01 +0100 Subject: [PATCH 23/26] Update e2e test --- test/e2e/specs/editor/various/block-renaming.spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e/specs/editor/various/block-renaming.spec.js b/test/e2e/specs/editor/various/block-renaming.spec.js index 8568258aaa4fda..c0f056e7b6e3d7 100644 --- a/test/e2e/specs/editor/various/block-renaming.spec.js +++ b/test/e2e/specs/editor/various/block-renaming.spec.js @@ -58,12 +58,14 @@ 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', @@ -71,8 +73,6 @@ test.describe( 'Block Renaming', () => { await expect( saveButton ).toBeDisabled(); - const nameInput = renameModal.getByLabel( 'Block name' ); - await expect( nameInput ).toHaveAttribute( 'placeholder', 'Group' ); await nameInput.fill( 'My new name' ); From f52d19386ee7de6ef4a8cddd41ec42dc55115fc8 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 13:25:30 +0100 Subject: [PATCH 24/26] Improve comment wording Co-authored-by: Ben Dwyer --- packages/components/src/modal/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index d71bdea33364a7..5d7429571b5347 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -104,7 +104,7 @@ function UnforwardedModal( ? `components-modal-header-${ instanceId }` : aria.labelledby; - // Modals should ignore the `Close` button which is the first focusable element. + // 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( focusOnMount === 'firstElement' ? getFirstTabbableElement : focusOnMount From 47f461ea059c43cf0d5dd2d60ff702895f7eb332 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 13 Sep 2023 13:27:32 +0100 Subject: [PATCH 25/26] Improve another comment --- packages/components/src/modal/index.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index 5d7429571b5347..f28c649af74356 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -40,17 +40,17 @@ import type { ModalProps } from './types'; let openModalCount = 0; /** - * When `firstElement` is passed to `focusOnMount`, this function is optimised to + * 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 optimising + * 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 that is not a close button. + * @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. From 3cf68b556e6b95d13fed671d6c15ecedb276f55d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 19 Sep 2023 10:39:16 +0100 Subject: [PATCH 26/26] Simplify Co-authored-by: Marco Ciampini --- packages/components/src/modal/index.tsx | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/components/src/modal/index.tsx b/packages/components/src/modal/index.tsx index f28c649af74356..fafdb41ed49d91 100644 --- a/packages/components/src/modal/index.tsx +++ b/packages/components/src/modal/index.tsx @@ -53,18 +53,16 @@ let openModalCount = 0; * @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 ]; + return ( + // Attempt to locate tabbable outside of the header portion of the Modal. + tabbables.find( + ( tabbable ) => + tabbable.closest( `.${ MODAL_HEADER_CLASSNAME }` ) === null + ) ?? + // Fallback to the first tabbable element anywhere within the Modal. + // Likely the `Close` button. + tabbables[ 0 ] + ); } function UnforwardedModal(