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.

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 arrows navigation in the block more options menu #7480

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jun 22, 2018

Description

Fixes the arrows navigation in the blocks More Options menu after recent changes in #7199

The new slot/fill implementation of the first menu item broke arrows navigation, for more details please refer to the related issue #7443

This PR tries to fix it passing the deep prop, which makes the menu get also the focusable element that are not direct children of the menu.

Also, to make sure the <div> element used by Slot is ignored by assistive technologies and the accessibility tree is "flattened" and made only by menu > menuitem, it uses a role="presentation" on the div. As with all the ARIA things, this doesn't affect anything for browsers as it's only used by assistive technologies. See the ARIA Authoring Practices, where role="none" (alias of presentation) is used to flatten the tree:

Ensuring to flatten the tree structure is important for the way assistive technologies perceive a menu, for example one of the reasons is they calculate and announce the number of items in the menu and their position:

screenshot 11

How has this been tested?

  • npm test and updated snapshots
  • tested with different browser / screen reader combos

To test:

  • open a block More Option menu either by clicking on it or using the keyboard
  • using the arrow keys, navigate through the menu items and check navigation works correctly

Fixes #7443

@afercia afercia requested a review from aduth June 22, 2018 14:38
@aduth
Copy link
Member

aduth commented Jun 25, 2018

@youknowriad Do you happen to recall the context for why deep was needed as part of its introduction in #3303 ? I think we should just remove the prop and have this be its default behavior.

@youknowriad
Copy link
Contributor

@aduth I don't think there was a strong argument for it. It was just incidental to the fact that we have several levels of containers sometimes. And adding role="representation" is not always the answer because of the fact that the components should be independent from each other and sometimes adding a role="representation" to a DOM element should be a decision made within the component as a standalone unit instead of adding it just to satisfy the fact that the component is wrapped with a menu which can be true now, but since the component is reusable might not be always the case.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Pushed c8f558f to remove the deep prop. LGTM if LGTY.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Jun 26, 2018
@gziolo gziolo added this to the 3.2 milestone Jul 5, 2018
@gziolo gziolo merged commit 9a23989 into master Jul 5, 2018
@gziolo gziolo deleted the fix/block-more-options-menu-arrows-navigation branch July 5, 2018 09:21
@afercia
Copy link
Contributor Author

afercia commented Jul 20, 2018

For reference: the div with role-presentation could go away if #7231 gets merged.

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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block More Options menu: arrows navigation doesn't work any longer
4 participants