Skip to content

Commit

Permalink
Modal: fix closing when contained iframe is focused (#51602)
Browse files Browse the repository at this point in the history
* Fix modal closing when a contained iframe is focused

Remove focus outside hook.

* Test: add iframe to Modal component

* Monopolize open modal

* Revise comment

* Remove needless condition

* Avoid running effect due to unstable prop

* Permit nested Modals

* Robustly support nested Modals

* Preserve current behavior for nested modals

* Split effect to separate concerns

* Add unit test for request closing of nested modals

* Call onRequestClose for nested modal when outer modal unmounts

* Comment and rename per feedback

* Add changelog entry

* Revert "Test: add iframe to Modal component"

This reverts commit 6502514.

* Fix changelog

---------

Co-authored-by: Tetsuaki Hamano <[email protected]>
  • Loading branch information
stokesman and t-hamano authored Oct 5, 2023
1 parent bd9a1cf commit 0816b4c
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 22 deletions.
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

### Bug Fix

- `Modal`: fix closing when contained iframe is focused ([#51602](https://github.com/WordPress/gutenberg/pull/51602)).

## 25.9.0 (2023-10-05)

### Enhancements
Expand Down
80 changes: 58 additions & 22 deletions packages/components/src/modal/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
* External dependencies
*/
import classnames from 'classnames';
import type { ForwardedRef, KeyboardEvent, UIEvent } from 'react';
import type {
ForwardedRef,
KeyboardEvent,
MutableRefObject,
UIEvent,
} from 'react';

/**
* WordPress dependencies
Expand All @@ -15,12 +20,13 @@ import {
useState,
forwardRef,
useLayoutEffect,
createContext,
useContext,
} from '@wordpress/element';
import {
useInstanceId,
useFocusReturn,
useFocusOnMount,
__experimentalUseFocusOutside as useFocusOutside,
useConstrainedTabbing,
useMergeRefs,
} from '@wordpress/compose';
Expand All @@ -36,8 +42,13 @@ import Button from '../button';
import StyleProvider from '../style-provider';
import type { ModalProps } from './types';

// Used to count the number of open modals.
let openModalCount = 0;
// Used to track and dismiss the prior modal when another opens unless nested.
const level0Dismissers: MutableRefObject<
ModalProps[ 'onRequestClose' ] | undefined
>[] = [];
const ModalContext = createContext( level0Dismissers );

let isBodyOpenClassActive = false;

function UnforwardedModal(
props: ModalProps,
Expand Down Expand Up @@ -91,7 +102,6 @@ function UnforwardedModal(
);
const constrainedTabbingRef = useConstrainedTabbing();
const focusReturnRef = useFocusReturn();
const focusOutsideProps = useFocusOutside( onRequestClose );
const contentRef = useRef< HTMLDivElement >( null );
const childrenContainerRef = useRef< HTMLDivElement >( null );

Expand Down Expand Up @@ -120,26 +130,52 @@ function UnforwardedModal(
}
}, [ contentRef ] );

// Accessibly isolates/unisolates the modal.
useEffect( () => {
ariaHelper.modalize( ref.current );
return () => ariaHelper.unmodalize();
}, [] );

// Keeps a fresh ref for the subsequent effect.
const refOnRequestClose = useRef< ModalProps[ 'onRequestClose' ] >();
useEffect( () => {
openModalCount++;
refOnRequestClose.current = onRequestClose;
}, [ onRequestClose ] );

if ( openModalCount === 1 ) {
document.body.classList.add( bodyOpenClassName );
}
// The list of `onRequestClose` callbacks of open (non-nested) Modals. Only
// one should remain open at a time and the list enables closing prior ones.
const dismissers = useContext( ModalContext );
// Used for the tracking and dismissing any nested modals.
const nestedDismissers = useRef< typeof level0Dismissers >( [] );

// Updates the stack tracking open modals at this level and calls
// onRequestClose for any prior and/or nested modals as applicable.
useEffect( () => {
dismissers.push( refOnRequestClose );
const [ first, second ] = dismissers;
if ( second ) first?.current?.();

const nested = nestedDismissers.current;
return () => {
openModalCount--;
nested[ 0 ]?.current?.();
dismissers.shift();
};
}, [ dismissers ] );

if ( openModalCount === 0 ) {
const isLevel0 = dismissers === level0Dismissers;
// Adds/removes the value of bodyOpenClassName to body element.
useEffect( () => {
if ( ! isBodyOpenClassActive ) {
isBodyOpenClassActive = true;
document.body.classList.add( bodyOpenClassName );
}
return () => {
if ( isLevel0 && dismissers.length === 0 ) {
document.body.classList.remove( bodyOpenClassName );
isBodyOpenClassActive = false;
}
};
}, [ bodyOpenClassName ] );
}, [ bodyOpenClassName, dismissers, isLevel0 ] );

// Calls the isContentScrollable callback when the Modal children container resizes.
useLayoutEffect( () => {
Expand Down Expand Up @@ -200,12 +236,9 @@ function UnforwardedModal(
onPointerUp: React.PointerEventHandler< HTMLDivElement >;
} = {
onPointerDown: ( event ) => {
if ( event.isPrimary && event.target === event.currentTarget ) {
if ( event.target === event.currentTarget ) {
pressTarget = event.target;
// Avoids loss of focus yet also leaves `useFocusOutside`
// practically useless with its only potential trigger being
// programmatic focus movement. TODO opt for either removing
// the hook or enhancing it such that this isn't needed.
// Avoids focus changing so that focus return works as expected.
event.preventDefault();
}
},
Expand All @@ -222,7 +255,7 @@ function UnforwardedModal(
},
};

return createPortal(
const modal = (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
ref={ useMergeRefs( [ ref, forwardedRef ] ) }
Expand Down Expand Up @@ -253,9 +286,6 @@ function UnforwardedModal(
aria-labelledby={ contentLabel ? undefined : headingId }
aria-describedby={ aria.describedby }
tabIndex={ -1 }
{ ...( shouldCloseOnClickOutside
? focusOutsideProps
: {} ) }
onKeyDown={ onKeyDown }
>
<div
Expand Down Expand Up @@ -320,7 +350,13 @@ function UnforwardedModal(
</div>
</div>
</StyleProvider>
</div>,
</div>
);

return createPortal(
<ModalContext.Provider value={ nestedDismissers.current }>
{ modal }
</ModalContext.Provider>,
document.body
);
}
Expand Down
29 changes: 29 additions & 0 deletions packages/components/src/modal/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,35 @@ describe( 'Modal', () => {
expect( onRequestClose ).not.toHaveBeenCalled();
} );

it( 'should request closing of nested modal when outer modal unmounts', async () => {
const user = userEvent.setup();
const onRequestClose = jest.fn();

const RequestCloseOfNested = () => {
const [ isShown, setIsShown ] = useState( true );
return (
<>
{ isShown && (
<Modal
onKeyDown={ ( { key } ) => {
if ( key === 'o' ) setIsShown( false );
} }
onRequestClose={ noop }
>
<Modal onRequestClose={ onRequestClose }>
<p>Nested modal content</p>
</Modal>
</Modal>
) }
</>
);
};
render( <RequestCloseOfNested /> );

await user.keyboard( 'o' );
expect( onRequestClose ).toHaveBeenCalled();
} );

it( 'should accessibly hide and show siblings including outer modals', async () => {
const user = userEvent.setup();

Expand Down

1 comment on commit 0816b4c

@github-actions
Copy link

Choose a reason for hiding this comment

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

Flaky tests detected in 0816b4c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6424253735
📝 Reported issues:

Please sign in to comment.