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

Fix aria-checked attribute not set for plugin settings buttons in Options dropdown #65667

Merged
merged 3 commits into from
Sep 26, 2024
Merged

Fix aria-checked attribute not set for plugin settings buttons in Options dropdown #65667

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a78e53
Fix aria-checked attribute not set for plugin settings buttons in Opt…
manzoorwanijk Sep 26, 2024
de3992c
Handle all the possible roles for checked state
manzoorwanijk Sep 26, 2024
33c142c
Use better type for role
manzoorwanijk Sep 26, 2024
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
7 changes: 7 additions & 0 deletions packages/interface/src/components/complementary-area-toggle/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ function ComplementaryAreaToggle( {
<ComponentToUse
icon={ selectedIcon && isSelected ? selectedIcon : icon }
aria-controls={ identifier.replace( '/', ':' ) }
// Make sure aria-checked matches spec https://www.w3.org/TR/wai-aria-1.1/#aria-checked
aria-checked={
props.role === 'menuitemcheckbox' ||
props.role === 'menuitemradio'
? isSelected
: undefined
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same approach used in

gutenberg/packages/components/src/menu-item/index.tsx

Lines 60 to 65 in 8c5a0b6

// Make sure aria-checked matches spec https://www.w3.org/TR/wai-aria-1.1/#aria-checked
aria-checked={
role === 'menuitemcheckbox' || role === 'menuitemradio'
? isSelected
: undefined
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this component is more generic than MenuItem, should we check for all of the roles are compatible with aria-checked ? Ie. checkbox, option, radio, switch, menuitemcheckbox, menuitemradio, treeitem ? (source)

Copy link
Member

Choose a reason for hiding this comment

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

I think the component rendered by plugins will always be MenuItem. See:

gutenberg/packages/interface/src/components/complementary-area-more-menu-item/index.js

Lines 13 to 50 in 69efbca

const PluginsMenuItem = ( {
// Menu item is marked with unstable prop for backward compatibility.
// They are removed so they don't leak to DOM elements.
// @see https://github.com/WordPress/gutenberg/issues/14457
__unstableExplicitMenuItem,
__unstableTarget,
...restProps
} ) => <MenuItem { ...restProps } />;
export default function ComplementaryAreaMoreMenuItem( {
scope,
target,
__unstableExplicitMenuItem,
...props
} ) {
return (
<ComplementaryAreaToggle
as={ ( toggleProps ) => {
return (
<ActionItem
__unstableExplicitMenuItem={
__unstableExplicitMenuItem
}
__unstableTarget={ `${ scope }/${ target }` }
as={ PluginsMenuItem }
name={ `${ scope }/plugin-more-menu` }
{ ...toggleProps }
/>
);
} }
role="menuitemcheckbox"
selectedIcon={ check }
name={ target }
scope={ scope }
{ ...props }
/>
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the component rendered by plugins will always be MenuItem

That's true, but <ComplementaryAreaToggle /> is a generic component that technically can be rendered as anything, and therefore I don't think that checking for more roles can do any harm in this case

onClick={ () => {
if ( isSelected ) {
disableComplementaryArea( scope );
Expand Down
Loading