Improve keyboard interaction of the DropdownMenu by afercia · Pull Request #1975 · WordPress/gutenberg

afercia · 2017-07-21T18:08:35Z

This PR tries to improve the accessibility and keyboard interaction of the DropdownMenu following the model described in the ARIA Authoring Practices.

The DropdownMenu is, in ARIA terms, a composite widget made of a Menu button and a Menu. To get how the expected keyboard interaction should work I'd recommend to jump directly to the W3C examples for the standalone Menu:
https://www.w3.org/TR/wai-aria-practices/examples/menu-button/menu-button-actions.html
and the same example in the context of a Toolbar:
https://www.w3.org/TR/wai-aria-practices/examples/toolbar/toolbar.html

Navigation through the menu items should work with arrow keys only, while tabbing should close the menu (same for Escape). Optionally, items navigation should "loop" continuously.

Worth reminding ARIA widgets replicate common, established patterns already used in our operating systems and keyboard users are used to them. For example, the macOS Finder menu works this way: it uses arrow keys to navigate the drop-down items:

The mixed use of the Tab key and arrow keys is a great way to reduce the amount of Tab key presses necessary to navigate through content, as it allows to open a menu just if that's really needed.

As mentioned in the issue, it would be great to consider to extend this behavior to other components too.

Fixes #1880

afercia · 2017-07-21T18:09:36Z

Fixes #1880

aduth · 2017-07-26T20:50:21Z

components/dropdown-menu/index.js

-		if ( this.menuRef ) {
-			const menu = findDOMNode( this.menuRef );
+		if ( this.menuNode ) {
 			if ( index < 0 ) {


With changes to focusPrevious, is it ever the case that this condition would pass? Also generally concerned about previousElementSibling potentially not being a focusable element. Might be worth dropping this.

aduth · 2017-07-26T20:50:56Z

components/dropdown-menu/index.js

+
+		this.maybeCloseMenuOnBlur = setTimeout( () => {
+			this.closeMenu();
+		}, 100 );


How did you settle on 100 ?

Completely arbitrary (I know...). There's a brief interval between blur on an element and focus on the following element. '0' didn't work. On the W3C examples, they use '300', see:
https://www.w3.org/TR/wai-aria-practices/examples/menu-button/js/Menubutton.js
https://www.w3.org/TR/wai-aria-practices/examples/menu-button/js/MenuItemAction.js
https://www.w3.org/TR/wai-aria-practices/examples/menu-button/js/PopupMenuAction.js

Open to suggestions...

aduth · 2017-07-26T20:53:44Z

components/dropdown-menu/index.js

-					this.closeMenu();
 					const node = findDOMNode( this );
 					const toggle = node.querySelector( '.components-dropdown-menu__toggle' );
 					toggle.focus();


I'm finding that occasionally pressing escape when navigating buttons will clear focus altogether (document.activeElement === document.body). Not sure if that's specific to your changes here though.

macOS Chrome 59.0.3071.115

Same chrome here and can't reproduce. Updated to 60: same.
Instead, I think handleKeyUp() doesn't work as intended: when I keep Escape pressed for a while, the menu closes immediately so when I release Escape the condition to stop propagation isn't met and the toolbar disappears.

aduth · 2017-07-26T20:55:33Z

components/dropdown-menu/index.js

+						ref={ this.bindMenuReferenceNode }
+						onFocus={ this.handleFocus }
+						onBlur={ this.handleBlur }
+						tabIndex="-1"


Is tabindex necessary on div?

afercia · 2017-07-27T22:21:27Z

Using keyup to close the menu when pressing Escape allows to greatly simplify. No timers, no focus/blur. To recap:

menu items can be navigated with arrows only
tabbing will close the menu and natively focus the next/previous focusable elements (note: there's a pre-existing and unrelated bug when using Firefox, will open a separate issue)
pressing Escape closes the menu and focuses the toggle
one more Escape press makes the whole toolbar disappear, as expected

aduth · 2017-07-28T14:30:12Z

components/dropdown-menu/index.js

+		if ( event.keyCode === ESCAPE && this.state.open ) {
+			event.preventDefault();
+			event.stopPropagation();
+			const node = findDOMNode( this );


We could probably create a separate ref for the root rendered element to avoid the findDOMNode here.

aduth · 2017-07-28T14:31:28Z

components/dropdown-menu/index.js

+			event.preventDefault();
+			event.stopPropagation();
+			const node = findDOMNode( this );
+			const toggle = node.querySelector( '.components-dropdown-menu__toggle' );


Actually, for that matter, we could bind the ref to the toggle to avoid the querySelector as well.

aduth · 2017-07-28T14:32:31Z

Would be good to have some unit tests for this component, but that can be addressed separately I think.

codecov · 2017-07-28T14:50:38Z

Codecov Report

Merging #1975 into master will increase coverage by 0.24%.
The diff coverage is 11.53%.

@@            Coverage Diff             @@
##           master    #1975      +/-   ##
==========================================
+ Coverage   20.25%   20.49%   +0.24%     
==========================================
  Files         131      135       +4     
  Lines        4207     4240      +33     
  Branches      716      728      +12     
==========================================
+ Hits          852      869      +17     
- Misses       2827     2839      +12     
- Partials      528      532       +4

Impacted Files	Coverage Δ
components/dropdown-menu/index.js	`0% <0%> (ø)`	⬆️
components/icon-button/index.js	`100% <100%> (ø)`	⬆️
utils/nodetypes.js	`100% <0%> (ø)`
blocks/api/paste/remove-spans.js	`0% <0%> (ø)`
blocks/api/paste/strip-attributes.js	`0% <0%> (ø)`
blocks/api/paste/convert-tables.js	`0% <0%> (ø)`
blocks/api/paste.js	`62.71% <0%> (+2.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9a619d...f74d28a. Read the comment docs.

aduth · 2017-07-28T14:52:40Z

In e91f3a2, I revised IconButton to be a stateful component class, so that we can apply a ref to target it directly shifting focus to the toggle when the menu closes. Let me know if this looks good to you, otherwise I think this is ready to merge.

When used on a component class, the value of a ref is the component instance, not the DOM node. https://facebook.github.io/react/docs/refs-and-the-dom.html#adding-a-ref-to-a-class-component

afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 21, 2017

afercia requested a review from aduth July 21, 2017 18:08

aduth reviewed Jul 26, 2017

View reviewed changes

afercia mentioned this pull request Jul 27, 2017

Buggy focus behavior with the Table block when using Firefox #2063

Closed

aduth reviewed Jul 28, 2017

View reviewed changes

afercia added 4 commits July 28, 2017 16:36

ARIA menu keyboard interaction first pass.

09022e6

Focus the first item on open and some clean-up.

0e596a4

Reduce usage of findDOMNode and some clean-up.

2a1b53a

Use keyup to make Escape close the menu and simplify all the things.

ed9cd44

afercia force-pushed the update/dropdown-menu-keyboard-interaction branch from ec8272a to ed9cd44 Compare July 28, 2017 14:36

Target toggle focus by ref

e91f3a2

aduth and others added 3 commits July 28, 2017 11:40

Fix function binding for node references

378ad6c

Find toggle node to focus

dac57cb

When used on a component class, the value of a ref is the component instance, not the DOM node. https://facebook.github.io/react/docs/refs-and-the-dom.html#adding-a-ref-to-a-class-component

Use Component instead of wp globals.

f74d28a

aduth approved these changes Jul 31, 2017

View reviewed changes

afercia merged commit 581b585 into master Jul 31, 2017

afercia deleted the update/dropdown-menu-keyboard-interaction branch July 31, 2017 15:45

aduth mentioned this pull request Aug 1, 2017

Dropdown Menu: Document, test, refactor #2141

Merged

Conversation

afercia commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

afercia commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afercia commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 28, 2017

Uh oh!

codecov bot commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aduth commented Jul 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

afercia commented Jul 21, 2017 •

edited

Loading

codecov bot commented Jul 28, 2017 •

edited

Loading