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

Fix issue with top bar menu having unique ARIA roles #13059

Closed
wants to merge 22 commits into from
Closed

Fix issue with top bar menu having unique ARIA roles #13059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
001a3f3
Merge pull request #1 from WordPress/master
Firestorm980 Nov 9, 2018
a42e5d6
Merge pull request #2 from WordPress/master
Firestorm980 Nov 20, 2018
75fc100
Merge pull request #3 from WordPress/master
Firestorm980 Nov 29, 2018
7811b8f
Merge pull request #4 from WordPress/master
Firestorm980 Nov 30, 2018
7f9ea08
Merge pull request #5 from WordPress/master
Firestorm980 Dec 12, 2018
874ab92
Updates roles for appropriate a11y in the header more menu
Firestorm980 Dec 13, 2018
92e4ef4
linting updates for header menu components
Firestorm980 Dec 13, 2018
ac8eb39
Updated snapshots for menu items
Firestorm980 Dec 13, 2018
7bf8a34
Merge pull request #6 from WordPress/master
Firestorm980 Dec 20, 2018
c58588b
Merge branch 'master' into fix/issue-12505
Firestorm980 Dec 20, 2018
13120e6
Merge pull request #7 from WordPress/master
Firestorm980 Feb 1, 2019
2517c36
Merge branch 'master' into fix/issue-12505
Firestorm980 Feb 1, 2019
a47ea27
Fix extra spacing in class attribute and eslint errors
Firestorm980 Feb 1, 2019
d99dbb7
Add a label for the tools and options menu
Firestorm980 Feb 1, 2019
ae906f4
Add a check for eventToOffset to prevent errors
Firestorm980 Feb 1, 2019
c52db84
Updated test snapshot
Firestorm980 Feb 1, 2019
be2b57c
Revert "Updated test snapshot"
Firestorm980 Feb 2, 2019
0ea3031
Add proper conditional for eventToOffset
Firestorm980 Feb 3, 2019
a123632
Merge remote-tracking branch 'upstream/master'
Firestorm980 Mar 22, 2019
8eabcbf
Merge branch 'master' into fix/issue-12505
Firestorm980 Mar 22, 2019
264510e
fix roles as props, update options to be menu item, fix a11y tree
Firestorm980 Mar 22, 2019
b23da67
update tests
Firestorm980 Mar 22, 2019
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
15 changes: 13 additions & 2 deletions packages/components/src/menu-group/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ export function MenuGroup( {
className = '',
instanceId,
label,
role = 'menu',
Copy link
Member

Choose a reason for hiding this comment

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

It feels wrong to see the default role set menu for the component which is called MenuGroup and then see it overridden back to group in other places. It seems like we need to introduce another component called Menu which will have role set to menu, its class name which will hide the label for the menu, and will always have useEventToOffset set to true. This will make it possible to leave MenuGroup usage unchanged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gziolo should we spin up a new issue for that?

Copy link
Member

Choose a reason for hiding this comment

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

I would feel comfortable doing it as part of this PR because in the current form you might see unexpected role="menu" in all plugins using MenuGroup.

useEventToOffset = true,
isLabelHidden = false,
} ) {
if ( ! Children.count( children ) ) {
return null;
Expand All @@ -33,9 +36,17 @@ export function MenuGroup( {
return (
<div className={ classNames }>
{ label &&
<div className="components-menu-group__label" id={ labelId }>{ label }</div>
<div
className={ classnames( 'components-menu-group__label', {
hidden: isLabelHidden,
} ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

see above

id={ labelId }
aria-hidden={ ( role === 'group' ) }
>
{ label }
</div>
}
<NavigableMenu orientation="vertical" aria-labelledby={ label ? labelId : null }>
<NavigableMenu orientation="vertical" aria-labelledby={ label ? labelId : null } role={ role } useEventToOffset={ useEventToOffset }>
{ children }
</NavigableMenu>
</div>
Expand Down
3 changes: 3 additions & 0 deletions packages/components/src/menu-group/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ exports[`MenuGroup should match snapshot 1`] = `
className="components-menu-group"
>
<div
aria-hidden={false}
className="components-menu-group__label"
id="components-menu-group-label-1"
>
Expand All @@ -13,6 +14,8 @@ exports[`MenuGroup should match snapshot 1`] = `
<ForwardRef(NavigableMenu)
aria-labelledby="components-menu-group-label-1"
orientation="vertical"
role="menu"
useEventToOffset={true}
>
<p>
My item
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/navigable-container/container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ class NavigableContainer extends Component {
const { getFocusableContext } = this;
const { cycle = true, eventToOffset, onNavigate = noop, stopNavigationEvents } = this.props;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { cycle = true, eventToOffset, onNavigate = noop, stopNavigationEvents } = this.props;
const { cycle = true, eventToOffset = noop, onNavigate = noop, stopNavigationEvents } = this.props;

This should be enough to make this work instead of the check:

// This avoids a console error when eventToOffset is null


// This avoids a console error when eventToOffset is null
if ( ! eventToOffset ) {
return;
}

const offset = eventToOffset( event );

// eventToOffset returns undefined if the event is not handled by the component
Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/navigable-container/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import NavigableContainer from './container';
export function NavigableMenu( {
role = 'menu',
orientation = 'vertical',
useEventToOffset = true,
...rest
}, ref ) {
const eventToOffset = ( evt ) => {
Expand Down Expand Up @@ -49,7 +50,7 @@ export function NavigableMenu( {
onlyBrowserTabstops={ false }
role={ role }
aria-orientation={ role === 'presentation' ? null : orientation }
eventToOffset={ eventToOffset }
eventToOffset={ useEventToOffset ? eventToOffset : null }
{ ...rest }
/>
);
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-post/src/components/header/mode-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ function ModeSwitcher( { onSwitch, mode } ) {

return (
<MenuGroup
role="group"
useEventToOffset={ false }
label={ __( 'Editor' ) }
>
<MenuItemsChoice
Expand Down
12 changes: 7 additions & 5 deletions packages/edit-post/src/components/header/more-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import WritingMenu from '../writing-menu';

const ariaClosed = __( 'Show more tools & options' );
const ariaOpen = __( 'Hide more tools & options' );
const menuLabel = __( 'Tools & Options' );

const MoreMenu = () => (
<Dropdown
Expand All @@ -33,11 +34,12 @@ const MoreMenu = () => (
) }
renderContent={ ( { onClose } ) => (
<Fragment>
<WritingMenu onClose={ onClose } />
<ModeSwitcher onSelect={ onClose } />
<PluginMoreMenuGroup.Slot fillProps={ { onClose } } />
<ToolsMoreMenuGroup.Slot fillProps={ { onClose } } />
<MenuGroup>
<MenuGroup label={ menuLabel } isLabelHidden={ true }>
Copy link
Member

Choose a reason for hiding this comment

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

Fragment is no longer necessary to wrap components.

<WritingMenu onClose={ onClose } />
<ModeSwitcher onSelect={ onClose } />
<PluginMoreMenuGroup.Slot fillProps={ { onClose } } />
<ToolsMoreMenuGroup.Slot fillProps={ { onClose } } />

<OptionsMenuItem onSelect={ onClose } />
</MenuGroup>
</Fragment>
Expand Down
3 changes: 2 additions & 1 deletion ...es/edit-post/src/components/header/plugin-more-menu-item/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ exports[`PluginMoreMenuItem renders menu item as button properly 1`] = `
className="components-menu-group"
>
<div
aria-hidden={true}
className="components-menu-group__label"
id="components-menu-group-label-0"
>
Expand All @@ -14,7 +15,7 @@ exports[`PluginMoreMenuItem renders menu item as button properly 1`] = `
aria-labelledby="components-menu-group-label-0"
aria-orientation="vertical"
onKeyDown={[Function]}
role="menu"
role="group"
>
<button
className="components-button components-icon-button components-menu-item__button has-icon has-text"
Expand Down
5 changes: 4 additions & 1 deletion packages/edit-post/src/components/header/plugins-more-menu-group/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ const { Fill: PluginsMoreMenuGroup, Slot } = createSlotFill( 'PluginsMoreMenuGro
PluginsMoreMenuGroup.Slot = ( { fillProps } ) => (
<Slot fillProps={ fillProps }>
{ ( fills ) => ! isEmpty( fills ) && (
<MenuGroup label={ __( 'Plugins' ) }>
<MenuGroup
role="group"
useEventToOffset={ false }
label={ __( 'Plugins' ) }>
{ fills }
</MenuGroup>
) }
Expand Down
5 changes: 4 additions & 1 deletion packages/edit-post/src/components/header/tools-more-menu-group/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ const { Fill: ToolsMoreMenuGroup, Slot } = createSlotFill( 'ToolsMoreMenuGroup'
ToolsMoreMenuGroup.Slot = ( { fillProps } ) => (
<Slot fillProps={ fillProps }>
{ ( fills ) => ! isEmpty( fills ) && (
<MenuGroup label={ __( 'Tools' ) }>
<MenuGroup
role="group"
useEventToOffset={ false }
label={ __( 'Tools' ) }>
{ fills }
</MenuGroup>
) }
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-post/src/components/header/writing-menu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import FeatureToggle from '../feature-toggle';
function WritingMenu( { onClose } ) {
return (
<MenuGroup
role="group"
useEventToOffset={ false }
label={ _x( 'View', 'noun' ) }
>
<FeatureToggle
Expand Down