Skip to content

Conversation

@stokesman
Copy link
Contributor

@stokesman stokesman commented Sep 22, 2023

What?

Updates the existing logic for accessibly isolating the Modal component so that it works for nested modals. More specifically, when the nested modal is opened the "opener" modal has its aria-hidden attribute set to "true".

Why?

It seems like how it’s supposed to work and we added a test for it in #54569.

How?

  • Executes the isolation logic every time a Modal mounts/unmounts instead of only for the first/final modal.
  • Revises tracking of hidden elements to use a 2d array to support levels of isolation.

Testing Instructions

  1. Open a post or page.
  2. Paste and execute the following snippet in the javascript console.
( ( {
	components: { Button, Modal },
	editPost: { PluginDocumentSettingPanel },
	element: { createElement: el, Fragment, useState },
	plugins: { registerPlugin }
} ) => {
	registerPlugin( 'modal-nesting-demo', {
		render: () => el( PluginDocumentSettingPanel, {
			name: 'modal-nesting-demo',
			title: 'Modal nesting demo',
			children: el( ModalNesting )
		} ),
	} );

	function ModalNesting() {
		const [ isOpen, setOpen ] = useState( false );
		const [ isInnerOpen, setIsInnerOpen ] = useState( false );
		return (
			el( Fragment, null,
				el( Button, { variant: "primary", onClick: () => setOpen( true ) }, "Open Modal" ),
				isOpen && (
					el( Modal, {
						onRequestClose: () => setOpen( false ),
						style: { maxWidth: '600px' },
						title: 'Nested modals vs. a11y isolation',
					},
						el( 'p', null, 'This modal should be accessibly hidden when the nested modal is open. When the nested modal is closed this modal should again be accessible while the rest of the tree remains inaccessible until this modal is closed.' ),

						isInnerOpen && el(
							Modal,
							{ onRequestClose: () => setIsInnerOpen( false ), title: '🪧 Nested' },
							el( 'p', null, 'Nothing to see here.' ),
						),

						el( Button, { variant: "secondary", onClick: () => setIsInnerOpen( true ) }, 'Open Nested'),
					)
				)
			)
		);
	}
} )( wp );
  1. Press the "Open Modal" button added to the post settings ("Editor Settings" > "Modal nesting demo").
  2. Note the Modal is opened and verify the rest of the page is no longer in the accessibility tree.
  3. Press the "Open Nested" button.
  4. Note the nested modal is opened and verify the outer modal is no longer in the accessibility tree.
  5. Close the nested modal.
  6. Verify the original modal is returned to the accessibility tree while the rest of the page is not.
  7. Close the modal.
  8. Verify the rest of the page is returned to the accessibility tree.

Testing Instructions for Keyboard

I tried to make the instructions above applicable for keyboard use.

Screenshots or screencast

modal-nested-aria.mp4

@stokesman stokesman added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Sep 22, 2023
@github-actions
Copy link

github-actions bot commented Sep 22, 2023

Flaky tests detected in a9ae3fe.
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/6340981048
📝 Reported issues:

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Let's add a CHANGELOG entry (I guess under enhancements?) and then merge!

if ( openModalCount === 0 ) {
document.body.classList.remove( bodyOpenClassName );
ariaHelper.showApp();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the separation of concerns here between modalizing/unmodalizing and adding/removing the body classname

Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

Looks good and tests well for me!

@stokesman stokesman force-pushed the update/modal-nested-aria branch from 204fbb8 to a9ae3fe Compare September 28, 2023 15:24
@stokesman stokesman merged commit 25d7b32 into trunk Sep 28, 2023
@stokesman stokesman deleted the update/modal-nested-aria branch September 28, 2023 16:21
@stokesman
Copy link
Contributor Author

Thanks for reviews and testing!

@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components

Projects

Status: Done 🎉

Development

Successfully merging this pull request may close these issues.

4 participants