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.

Sign up for GitHub

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

Jump to bottom

Implement accessible version of Navigation overlay preview toggle control #53462

Merged
merged 4 commits into from
Aug 11, 2023
Merged

Implement accessible version of Navigation overlay preview toggle control #53462

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b3c6443
Implement accessible version of overlay preview
getdave Aug 9, 2023
949a3f2
Remove unused id attribute
getdave Aug 9, 2023
8075cec
Use better wording
getdave Aug 10, 2023
7bc9bf2
Fix linting
getdave Aug 10, 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
28 changes: 21 additions & 7 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
import { __, sprintf } from '@wordpress/i18n';
import { speak } from '@wordpress/a11y';
import { close, Icon } from '@wordpress/icons';
import { useInstanceId } from '@wordpress/compose';

/**
* Internal dependencies
Expand Down Expand Up @@ -500,6 +501,11 @@ function Navigation( {
isFirstRender.current = false;
}, [ submenuAccessibilityNotice ] );

const overlayMenuPreviewId = useInstanceId(
OverlayMenuPreview,
`overlay-menu-preview`
);

const colorGradientSettings = useMultipleOriginColorsAndGradients();
const stylingInspectorControls = (
<>
Expand All @@ -515,6 +521,11 @@ function Navigation( {
! overlayMenuPreview
);
} }
aria-label={ __(
'Toggle overlay menu controls'
getdave marked this conversation as resolved.
Show resolved Hide resolved
) }
aria-controls={ overlayMenuPreviewId }
aria-expanded={ overlayMenuPreview }
>
{ hasIcon && (
<>
Expand All @@ -529,13 +540,16 @@ function Navigation( {
</>
) }
</Button>
{ overlayMenuPreview && (
<OverlayMenuPreview
setAttributes={ setAttributes }
hasIcon={ hasIcon }
icon={ icon }
/>
) }
<div id={ overlayMenuPreviewId }>
{ overlayMenuPreview && (
Comment on lines +541 to +542
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accessibility question. I couldn't find any definitive best practice for whether the element being targetted by aria-controls should always remain in the DOM.

Currently the contents are added/removed by React on toggle. But should we instead leave it in the DOM and show/hide using CSS?

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 think so. Here is an example that helps.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls

This uses the tab panel use-case where the tab content is hidden via CSS but not totally removed from the DOM.

Why is this panel being removed from the DOM anyway? Performance reasons?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tested this and could not find any negative effects. No warnings in console after opening/closing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this panel being removed from the DOM anyway? Performance reasons?

Generally it seems to be a common pattern in React. I believe it's because it's easier for the developer to simply not-render something based on state rather than figure out the best way to hide via CSS.

Sometimes I guess rendering a component when it's not needed might trigger performance issues so that's worth bearing in mind.

In this case however we should be ok to leave the items in the DOM.

<OverlayMenuPreview
setAttributes={ setAttributes }
hasIcon={ hasIcon }
icon={ icon }
hidden={ ! overlayMenuPreview }
/>
) }
</div>
</>
) }
<h3>{ __( 'Overlay Menu' ) }</h3>
Expand Down