Post visibility panel better keyboard interaction. #1886

afercia · 2017-07-13T17:01:19Z

This PR is a POC to try to address keyboard interaction on the Post Visibility panel:

To my understanding, PostVisibility is mounted when the sidebar opens so I'm not sure withFocusReturn can be used here. If only something similar to react-click-outside existed also for focusOut, things would be simpler. Any feedback for a better approach very welcome.

adds a keydown event to close the panel when pressing Escape
adds an blur event to close the panel when tabbing away (uses event.relatedTarget)
moves focus back to the toggle when closing the panel

The code here is mainly to share and discuss better approaches. I haven't found better ways to do this, so any help would be greatly appreciated 🙂

Also to note: seems it doesn't work very well when using Safari+VoiceOver, while it works nicely with Firefox+NVDA.

I also get a warning A component is changing an uncontrolled input of type text to be controlled... but this seems to happen also on master.

Fixes #1885

aduth

If we moved the dialog portion of the PostVisibility into a separate component, could we then apply withFocusReturn to the new component?

Thinking the Popover component could help manage these behaviors. Need to revisit #1204.

aduth · 2017-07-14T18:26:18Z

editor/sidebar/post-visibility/index.js

+
+ componentDidMount() {
+ const node = findDOMNode( this );
+ node.addEventListener( 'keydown', this.handleKeyDown, false );


Was there a reason we couldn't apply an onKeyDown prop to the rendered <div /> ?

Was wondering about this, I've used the same pattern used in the DropdownMenu. Should then be changed to an onKeyDown prop there too?

Should then be changed to an onKeyDown prop there too?

Probably.

aduth · 2017-07-14T18:27:20Z

editor/sidebar/post-visibility/index.js

@@ -71,6 +74,45 @@ class PostVisibility extends Component {
 this.setState( { opened: false } );
 }

+ handleBlur( event ) {
+ if ( this.state.opened && event.relatedTarget !== null ) {
+ const wrapper = findDOMNode( this );


This is a case where we could avoid findDOMNode by instead assigning a ref to the rendered element.

https://facebook.github.io/react/docs/refs-and-the-dom.html

If you have absolutely no control over the child component implementation, your last option is to use findDOMNode(), but it is discouraged.

afercia · 2017-07-16T16:00:33Z

If we moved the dialog portion of the PostVisibility into a separate component, could we then apply withFocusReturn to the new component?
Thinking the Popover component could help manage these behaviors. Need to revisit #1204.

Standardizing and simplifying makes a lot of sense. We should then always make the Popover behave like a modal: constraining tabbing within the component, add a control to close it, etc. There's some discussion on the issue about this. While moving focus back on the toggle makes sense when pressing Escape, we're unsure about what should happen on blur. Wondering if just closing the Popover and let tabbing order work natively would be a best option.

Note: looks like withFocusReturn must be used with care, see #1917 and #1918

afercia · 2017-07-17T21:12:14Z

OK so testing with Safari + VoiceOver, event.relatedTarget returns null, only when using VoiceOver 😞 No idea why. This makes closing the popover on blur fail. I've tested also with Safari Technology Preview + VoiceOver and it works perfectly. Any suggestions for alternative methods to check if the element that receives focus is within the popover very welcome.

afercia · 2017-07-17T21:20:28Z

Refactored and simplified to use an onKeyDown prop on the popover dialog and refs to get DOM elements. Also, changed the behavior on blur: focus is not moved back to the toggle now; instead, the natural tabbing order is preserved.

The issue with event.relatedTarget on Safari+VoiceOver stands though.

aduth · 2017-07-18T19:42:23Z

editor/sidebar/post-visibility/index.js

@@ -10,7 +10,8 @@ import { withInstanceId } from 'components';
 * WordPress dependencies
 */
 import { __ } from 'i18n';
-import { Component } from 'element';
+import { Component, findDOMNode } from 'element';


Tests fail here because this findDOMNode is unused.

aduth · 2017-07-18T19:43:01Z

editor/sidebar/post-visibility/index.js

 >
 { getVisibilityLabel( visibility ) }
 </button>

 { this.state.opened &&
- <div className="editor-post-visibility__dialog">
+ <div className="editor-post-visibility__dialog"
+ onKeyDown={ this.handleKeyDown }


Warning:

138:6 warning Visible, non-interactive elements should not have mouse or keyboard event listeners

Might explain why this was avoided for DropdownMenu . Is it fair to disable the rule here?

I think this rule should be improved a bit. While the intent is good, it should be allowed to use keyboard events this way when one knows what he's doing. Also, why doesn't catch onBlur then? Also onBlur can indicate keyboard interaction.
https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/no-static-element-interactions.md

Also, some of the "solutions" mentioned in the docs, like adding role="presentation" to elements that already have no semantic meaning, don't look right to me and are against the ARIA spec.

Will disable the rule (also remove the previous disabling and comment for htmlFor).

If the rule causes more harm or confusion than it helps, I'd also be okay with disabling it globally from the root .eslintrc.json (i.e. "jsx-a11y/no-static-element-interactions": "off").

Hm I think the rule makes perfectly sense for example to avoid to use <div> or <span> elements as UI controls instead of buttons. Re: the warning: it should probably be evaluated on a case by case basis. Our onKeyDown here is actually used for the radio buttons and input field. Maybe the rule should be improved to check if a <div> is focusable and actually able to receive a keyboard event. Anyways, it's good to have it.

aduth · 2017-07-18T19:44:36Z

editor/sidebar/post-visibility/index.js

@@ -71,6 +74,21 @@ class PostVisibility extends Component {
 this.setState( { opened: false } );
 }

+ handleBlur( event ) {
+ if ( this.state.opened && event.relatedTarget !== null && ! this.dialog.contains( event.relatedTarget ) ) {


While explicitness could be handy, I don't know that there's ever a case that event.relatedTarget is null and this.dialog.contains( null ) would ever be true. In other words, we could safely drop && event.relatedTarget !== null.

This needs to be changed anyways, since relatedTarget doesn't work on Safari 10 when using VoiceOver (see comment on the issue). We need to find an alternative solution.

Apparently Firefox has some issues with relatedTarget as well? There's an interesting idea here for determining the related target via a setTimeout( () => { const relatedTarget = document.activeElement } , 0 ) when the blur occurs.

facebook/react#2011 (comment)

Yes the setTimeout 0 trick is the only possible solution I've found so far, not so spectacularly happy to have to use setTimeout. I guess other solutions would be fragile though. Re: Firefox, I think at the time that comments were posted, Firefox was not supporting relatedTarget. It does now, since version 48 https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget
Instead, IE 11 could be a problem but I can't test in IE 11 at the moment 🙂 see #1397 and #1863

aduth · 2017-08-03T13:14:43Z

editor/sidebar/post-visibility/index.js

@@ -11,6 +11,7 @@ import { withInstanceId } from 'components';
 */
 import { __ } from 'i18n';
 import { Component } from 'element';
+import { ESCAPE } from 'utils/keycodes';


Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin git rebase origin/master

Further, we should not import from nested paths. See #941. Instead, import { keycodes } from '@wordpress/utils'; and destructure from the keycodes object:

const { ESCAPE } = keycodes;

afercia · 2017-08-03T17:03:26Z

Rebased and restored (more or less...) the focus/blur behavior. However, this didn't work so well also before the latest changes. I'm wondering if the whole thing should work instead on the Popover component, since now it's focusable (it uses tabindex="0" by default). Maybe also clickOutside should work on the Popover? /cc @aduth

codecov · 2017-08-03T17:05:52Z

Codecov Report

Merging #1886 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
- Coverage    22.5%   22.43%   -0.08%     
==========================================
  Files         139      139              
  Lines        4332     4391      +59     
  Branches      729      736       +7     
==========================================
+ Hits          975      985      +10     
- Misses       2833     2878      +45     
- Partials      524      528       +4

Impacted Files	Coverage Δ
editor/sidebar/post-visibility/index.js	`0% <0%> (ø)`	⬆️
editor/header/tools/preview-button.js	`65.38% <0%> (-29.06%)`	⬇️
blocks/library/button/index.js	`26.66% <0%> (ø)`	⬆️
editor/sidebar/table-of-contents/index.js	`0% <0%> (ø)`	⬆️
blocks/library/cover-text/index.js	`32.43% <0%> (+0.85%)`	⬆️

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 b9deeb8...84d7587. Read the comment docs.

aduth · 2017-08-07T17:09:01Z

Some ongoing effort in #2160 to move "click outside" behavior into the popover. I think Escape-handling might be well suited there as well, though maybe as a subsequent enhancement.

afercia · 2017-08-09T16:23:16Z

With the relevant changes in #2160 I think this PR can be closed. The related issue should stay open though, as keyboard interaction is still a big problem. See also #2306

afercia · 2017-08-29T14:30:34Z

Closing as the PopOver component has seen big changes and the pending issues should be addressed with a new approach, see #2306

afercia requested a review from aduth July 13, 2017 17:01

aduth added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels. labels Jul 14, 2017

aduth reviewed Jul 14, 2017

View reviewed changes

afercia mentioned this pull request Jul 16, 2017

Sidebar: withFocusReturn doesn't work when the Sidebar is already open #1918

Closed

afercia mentioned this pull request Jul 17, 2017

Post visibility panel better keyboard interaction #1885

Closed

aduth reviewed Jul 18, 2017

View reviewed changes

aduth reviewed Aug 3, 2017

View reviewed changes

afercia added 4 commits August 3, 2017 18:03

Post visibility panel better keyboard interaction.

5047c00

Use onKeyDown prop and refs.

91d9e90

Fix uncontrolled controlled input warning.

d2d3765

Restore keyboard interaction after Popover introduction.

84d7587

afercia force-pushed the update/post-visibility-keyboard-interaction branch from 63cd684 to 84d7587 Compare August 3, 2017 17:00

afercia closed this Aug 29, 2017

afercia deleted the update/post-visibility-keyboard-interaction branch August 29, 2017 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post visibility panel better keyboard interaction. #1886

Post visibility panel better keyboard interaction. #1886

afercia commented Jul 13, 2017

aduth left a comment

aduth Jul 14, 2017

afercia Jul 16, 2017

aduth Jul 18, 2017

aduth Jul 14, 2017

afercia Jul 16, 2017

afercia commented Jul 16, 2017

afercia commented Jul 17, 2017

afercia commented Jul 17, 2017

aduth Jul 18, 2017

aduth Jul 18, 2017

afercia Jul 18, 2017

aduth Jul 20, 2017

afercia Jul 21, 2017

aduth Jul 18, 2017

afercia Jul 18, 2017

aduth Jul 20, 2017

afercia Jul 21, 2017

aduth Aug 3, 2017

afercia commented Aug 3, 2017

codecov bot commented Aug 3, 2017 •

edited

Loading

aduth commented Aug 7, 2017

afercia commented Aug 9, 2017

afercia commented Aug 29, 2017

Post visibility panel better keyboard interaction. #1886

Post visibility panel better keyboard interaction. #1886

Conversation

afercia commented Jul 13, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Jul 16, 2017

afercia commented Jul 17, 2017

afercia commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Aug 3, 2017

codecov bot commented Aug 3, 2017 • edited Loading

Codecov Report

aduth commented Aug 7, 2017

afercia commented Aug 9, 2017

afercia commented Aug 29, 2017

codecov bot commented Aug 3, 2017 •

edited

Loading