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

Make the Blocks navigation a nested list to better communicate the blocks nesting level #11734

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Nov 11, 2018

Description

When there are nested blocks, the Blocks navigation displays them with some indentation. However, this information is only visual. This PR seeks to communicate the nesting level also semantically by using an unordered list. Lists convey important information for free: the number of items in the list, their nesting level, the current item number.

Assistive technologies can use the list semantics to properly inform users, so they can actually get the structure of nested blocks.

I've done my best to avoid visual changes.

Worth noting arrows navigation won't work well with screen readers on Windows because now there isn't an ARIA role to instruct screen readers to stop intercepting keystrokes. I think providing the missing information is more important than arrows navigation. Also, there are proposal to change and extend the functionalities of the Blocks navigation, see #10984, #11408, #11688, and #11711. In all those cases, an ARIA menu wouldn't be the proper UI to use and arrows navigation would need to be removed anyways.

I'm trying to keep the arrows navigation behavior for non-screen reader users for now, but I'd tend to think this component is now more similar to the "Document outline": a simple list with no special interaction other than the native one. Please do let me know if completely removing arrows navigation is desirable.

  • makes the Blocks Navigation an unordered (nested) list
  • uses NavigableMenu to keep arrows navigation (for now)
  • uses a screen-reader-text to communicate the selected state of a list item
  • avoids to render an aria-orientation attribute, as this is not a menu any longer
  • changes the editor-block-icon to a <span> element: <div> elements are invalid HTML within buttons; we'd want this component to be reusable everywhere and also ensure valid HTML and a div doesn't seem appropriate; the editor-block-icon is already a flex element with set width and height so this change shouldn't harm anything
  • "Block Navigation" is not used as target for aria-labelledby any longer, I'm not so worried about it because users get this popover clicking on a button that already announces "Block Navigation"

Fixes #11713

Screenshots to illustrate how the nesting level gets announced thanks to the native semantics of an <ul> element:

screenshot 2018-11-11 at 12 29 19

screenshot 2018-11-11 at 12 29 27

screenshot 2018-11-11 at 12 28 17

Note: other screen readers communicate the nesting info in different ways, see the example screenshots on #11713

@afercia afercia requested a review from tofumatt November 11, 2018 15:13
@designsimply designsimply added the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label Nov 11, 2018
@jwold
Copy link

jwold commented Nov 11, 2018

From a UI perspective this looks great. Seems like you've been able to propose a solution that still keeps the look and feel we've been using.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Nov 11, 2018
@mtias mtias added this to the 4.4 milestone Nov 12, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

This is awesome! UI still looks great and the accessibility improvements are ace. Thanks a bunch, this is great 👍

@@ -25,23 +25,30 @@ function BlockNavigationList( {
showNestedBlocks,
} ) {
return (
<ul className="editor-block-navigation__list" role="presentation">
/*
* Disable reason: The `list` ARIA role is redundant but
Copy link
Member

Choose a reason for hiding this comment

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

❤️ for the explanation.

If there's a WebKit bug we could link to that'd be swell, but no worries if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. It's a Safari specific bug. Chrome is exempt. I wouldn't know where to look for Apple open bugs. We do the same thing in BlockTypesList, for more details see #7058 (comment). Test case: https://codepen.io/afercia/full/WxmJWx/

@@ -55,6 +62,7 @@ function BlockNavigationList( {
);
} ) }
</ul>
/* eslint-enable jsx-a11y/no-redundant-roles */
Copy link
Member

Choose a reason for hiding this comment

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

Probably not a big deal, but can this be moved immediately underneath the <ul>? Just to minimise the number of lines we ignore linting on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried and I'd need to do something like this to make the linter happy:

		/* eslint-disable jsx-a11y/no-redundant-roles */
		<ul className="editor-block-navigation__list" role="list">
			{ /* eslint-enable jsx-a11y/no-redundant-roles */ }
			{ map( blocks, ( block ) => {

not sure it looks so nice 🙂

@gziolo gziolo merged commit 4cc3ac0 into master Nov 13, 2018
@gziolo gziolo deleted the update/blocks-navigation-menu-nested-blocks-a11y branch November 13, 2018 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants