Accessibility: Dismiss BlockMover tooltips on escape key press. #15578

nicolad · 2019-05-11T08:34:43Z

Description

Fixes: #15145
Contextual PR: https://github.com/WordPress/gutenberg/pull/8147/files

How has this been tested?

Screenshots

Types of changes

Passed onKeyDown to createToggleIsOver method of Tooltip component.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

nicolad · 2019-05-11T08:40:21Z

packages/components/src/icon-button/index.js

@@ -45,7 +45,7 @@ function IconButton( props, ref ) {
 			// the children are empty and...
 			( ! children || ( isArray( children ) && ! children.length ) ) &&
 			// the tooltip is not explicitly disabled.
-			false !== tooltip
+			tooltip !== false


Readability improvement.

Noting that this will probably conflict with changes currently proposed in #19193 . Regardless if it's an improvement, I might suggest to remove it here, since it's not entirely relevant (or close in proximity) to the intended changes.

youknowriad · 2019-05-13T08:18:51Z

Can we make the dismissable tooltip behavior something built-in into the Tooltip component so it can apply holistically to all tooltips?

aduth · 2019-05-13T15:08:07Z

Can we make the dismissable tooltip behavior something built-in into the Tooltip component so it can apply holistically to all tooltips?

Yes. There's some implementation notes at the earlier (since-closed) #8147 (review)

nicolad · 2019-05-20T14:01:34Z

packages/components/src/tooltip/index.js

@@ -12,6 +12,7 @@ import {
 	cloneElement,
 	concatChildren,
 } from '@wordpress/element';
+import { KeyboardShortcuts } from '@wordpress/components';


@aduth, this is a little bit strange, just by importing KeyboardShortcuts I do get that nasty error:
Fatal error: Allowed memory size of 268435456 bytes exhausted ...
Am I doing something wrong here?

you can't import something from the same package using the @wordpress/* syntax, just use relative paths.

Discussed in Slack thread (link requires registration):

https://wordpress.slack.com/archives/C02QB2JS7/p1557863367231300

aduth

In this branch, while I do see that the tooltip is dismissed when pressing Escape, I also see that focus is lost entirely. I don't see this in master, and I wouldn't think it should be expected.

nicolad · 2019-05-21T16:45:02Z

In this branch, while I do see that the tooltip is dismissed when pressing Escape, I also see that focus is lost entirely. I don't see this in master, and I wouldn't think it should be expected.

I recall that there was an issue related to handling focus after modal is closed or smth similar. Will post here if I will find that.

aduth · 2019-05-21T18:12:21Z

packages/components/src/tooltip/index.js

@@ -117,7 +120,13 @@ class Tooltip extends Component {
 						aria-hidden="true"
 						animate={ false }
 					>
-						{ text }
+						<KeyboardShortcuts


I'm surprised this works, if it does. text is the text of the tooltip itself, which I wouldn't expect should ever have focus such that the KeyboardShortcuts could capture keyboard events. Instead, I would expect the KeyboardShortcuts should wrap the child.props.children (the original children).

@aduth, please let me know if the implementation is ok, so I can start adjusting failing tests. Thank you.

aduth · 2019-06-25T18:12:23Z

The general approach with KeyboardShortcuts seems like it would be a good approach.

There are a few concerns, however:

Why do we need any changes around onMouseOver, onKeyDown? To me, it seems like there should be almost no other changes necessary in this pull request aside from adding the KeyboardShortcuts wrapping element.
In the escape handler for KeyboardShortcuts, we can probably avoid createToggleIsOver (I doubt it works anyways) and instead bind a callback which calls setState directly.
A more difficult issue is the design regression seen in this screenshot:
- It seems that adding a new div wrapper (from KeyboardShortcuts) may affect styles of icon buttons. I'm not sure if there's an easy way to account for this universally. The div wrapper would be nice to avoid if we could, but we still need some way to capture events.

In further investigation, I wonder if we avoid KeyboardShortcuts and implement the onKeyDown as a property of the cloneElement props. You already have this. But as mentioned above, maybe we want to avoid createToggleIsOver, or at least check that the key being pressed is Escape. I don't recall what the earlier changes looked like, but with onKeyDown only, did it not achieve the desired effect?

…ility/improve-tooltips-behavior

nicolad · 2019-07-24T23:41:25Z

Removed KeyboardShortcuts, it works as expected:

aduth · 2019-07-29T19:22:23Z

packages/components/src/tooltip/index.js

@@ -90,6 +95,7 @@ class Tooltip extends Component {

 	render() {
 		const { children, position, text, shortcut } = this.props;
+		const { isOver } = this.state;


Why was this changed? Technically it's a violation of the no-unused-vars-before-return rule, but there is an explicit exemption for property destructuring since they're more difficult (but foreseeably possible) to handle. It's for this temporary exemption only that this doesn't emit an ESLint error. In fact: Looking at this usage, I'm tempted to remove the exemption when the destructuring only contains a single property.

I might suggest it be put back where it was.

Moved this back.
It was changed to make destructuring first in order to ease readability.
This is a frequent pattern I've seen through various react apps/libs.

aduth · 2019-07-29T19:23:51Z

packages/block-editor/src/components/block-mover/index.js

@@ -66,6 +66,8 @@ export class BlockMover extends Component {
 					aria-disabled={ isFirst }
 					onFocus={ this.onFocus }
 					onBlur={ this.onBlur }
+					onMouseEnter={ this.onFocus }


Could you explain this change?

This PR is based on @afercia 's PR
Maybe @afercia might help here since I've iterated over his initial work?

Honestly I can't remember why I made that change. That was one year ago 😬 Sorry!

@aduth @afercia, FYI - I have reverted these changes.

aduth · 2019-07-29T19:25:57Z

packages/components/src/tooltip/index.js

@@ -71,6 +71,11 @@ class Tooltip extends Component {
 				return;
 			}

+			// If pressed key is escape, no further actions are needed.
+			if ( event.keyCode === 27 ) { // 27 is the keyCode for escape


I would suggest to use the @wordpress/keycodes package to get a constant value, which negates the need for an inline code comment to explain the magic constant. It's already a dependency of @wordpress/components, so you should be able to just import it as such:

import { ESCAPE } from '@wordpress/keycodes'; // ...

Suggested change

if ( event.keyCode === 27 ) { // 27 is the keyCode for escape

if ( event.keyCode === ESCAPE ) {

Thanks @aduth, updated this.

…ility/improve-tooltips-behavior

aduth

Apologies for having not revisited this sooner. I was away from the project for some time between August and November, so I missed the updates.

I could use some clarification about what it is in the event handler which is actually causing the tooltip to be dismissed. At the very least, we should consider to include more detail about this behavior in the existing proposed inline comment.

aduth · 2019-12-18T15:20:59Z

packages/components/src/tooltip/index.js

@@ -89,6 +90,11 @@ class Tooltip extends Component {
 				return;
 			}

+			// If pressed key is escape, no further actions are needed.


I'm a bit confused by this. What actions have been taken at this point in the event handler when pressing Escape?

aduth · 2019-12-18T15:21:52Z

packages/components/src/icon-button/index.js

@@ -45,7 +45,7 @@ function IconButton( props, ref ) {
 			// the children are empty and...
 			( ! children || ( isArray( children ) && ! children.length ) ) &&
 			// the tooltip is not explicitly disabled.
-			false !== tooltip
+			tooltip !== false


Noting that this will probably conflict with changes currently proposed in #19193 . Regardless if it's an improvement, I might suggest to remove it here, since it's not entirely relevant (or close in proximity) to the intended changes.

tellthemachines · 2020-03-16T02:17:48Z

I'm wondering how this is going to play with the escape key now being used to put the editor in select mode. Should we change that interaction so that pressing escape if a tooltip is open only closes the tooltip?

nicolad · 2020-06-22T13:02:45Z

IconButton has been merged into Button here: #19193

nicolad · 2021-02-05T10:48:33Z

@tellthemachines is this PR still relevant?
shall I close it?

tellthemachines · 2021-02-08T00:59:58Z

@nicolad the issue persists, though to fix it we need to take into account potential conflicts with the Escape key also being used to enter Select mode.

Are you still keen and able to work on this? If not, I'd suggest closing the PR.

nicolad · 2021-02-08T09:40:59Z

Are you still keen and able to work on this? If not, I'd suggest closing the PR.

I am not able to work on this ATM.

Thanks for your input, have an awesome week 🚀

Accessibility: dismiss BlockMover tooltips on escape key press.

ae832dc

nicolad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label May 11, 2019

nicolad requested review from afercia and aduth May 11, 2019 08:34

nicolad requested review from ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners May 11, 2019 08:34

nicolad self-assigned this May 11, 2019

nicolad commented May 11, 2019

View reviewed changes

Accessibility: Make tooltips dismissable onMouseDown.

dc86b93

nicolad requested a review from ellatrix as a code owner May 11, 2019 08:51

nicolad changed the title ~~Dismiss BlockMover tooltips on escape key press.~~ Accessibility: Dismiss BlockMover tooltips on escape key press. May 11, 2019

Merge branch 'master' into accessibility/improve-tooltips-behavior

9a61a03

nicolad added 4 commits May 13, 2019 19:48

Merge branch 'master' into accessibility/improve-tooltips-behavior

861351c

Merge branch 'master' into accessibility/improve-tooltips-behavior

ceae20d

Merge branch 'master' into accessibility/improve-tooltips-behavior

62b163d

Imported KeyboardShortcuts into Tooltip.

60bbc6c

nicolad commented May 20, 2019

View reviewed changes

nicolad added 2 commits May 20, 2019 17:22

Fixed KeyboardShortcuts import and wrapped children with this.

8ae4d23

Updated the import and wrapped popover content.

4379cf3

aduth reviewed May 21, 2019

View reviewed changes

nicolad added 2 commits May 26, 2019 00:43

Wrapped correct elements.

919831f

test-unit:update summary --- 7 snapshots updated from 2 test suites.

f0962b3

nicolad requested a review from nerrad as a code owner May 25, 2019 21:52

nicolad added 2 commits May 26, 2019 01:30

unit-tests temp fix

9741f77

Conflicts fixed.

ee9b1a5

nicolad added 3 commits June 30, 2019 13:22

Merge branch 'master' of github.com:WordPress/gutenberg into accessib…

0331775

…ility/improve-tooltips-behavior

Merge branch 'master' of github.com:WordPress/gutenberg into accessib…

67cfdf7

…ility/improve-tooltips-behavior

Removed KeyboardShortcuts and checked escape key press.

b23a066

Updated snapshots.

2dfaf5b

aduth requested changes Jul 29, 2019

View reviewed changes

aduth mentioned this pull request Jul 29, 2019

ESLint Plugin: no-unused-vars-before-return: Exempt destructuring only if to multiple properties #16799

Merged

nicolad added 4 commits September 14, 2019 10:57

Merge branch 'master' into accessibility/improve-tooltips-behavior

10e7a32

Moved destructuring back.

a6f7be2

Merge branch 'master' of github.com:WordPress/gutenberg into accessib…

22ef4e2

…ility/improve-tooltips-behavior

Removed obsolete onMouseEnter and onMouseLeave handlers.

68dd831

nicolad requested a review from aduth December 17, 2019 18:18

aduth reviewed Dec 18, 2019

View reviewed changes

Conflicts fixed.

87321bd

nicolad closed this Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility: Dismiss BlockMover tooltips on escape key press. #15578

Accessibility: Dismiss BlockMover tooltips on escape key press. #15578

nicolad commented May 11, 2019 •

edited

Loading

nicolad May 11, 2019

aduth Dec 18, 2019

youknowriad commented May 13, 2019

aduth commented May 13, 2019

nicolad May 20, 2019

youknowriad May 20, 2019

aduth May 20, 2019

aduth left a comment

nicolad commented May 21, 2019

aduth May 21, 2019

nicolad Jun 18, 2019

aduth commented Jun 25, 2019

nicolad commented Jul 24, 2019

aduth Jul 29, 2019

nicolad Sep 14, 2019

aduth Jul 29, 2019

nicolad Jul 29, 2019

afercia Jul 30, 2019

nicolad Oct 8, 2019

aduth Jul 29, 2019

nicolad Sep 14, 2019

aduth left a comment

aduth Dec 18, 2019

aduth Dec 18, 2019

tellthemachines commented Mar 16, 2020

nicolad commented Jun 22, 2020

nicolad commented Feb 5, 2021 •

edited

Loading

tellthemachines commented Feb 8, 2021

nicolad commented Feb 8, 2021

	if ( event.keyCode === 27 ) { // 27 is the keyCode for escape
	if ( event.keyCode === ESCAPE ) {

Accessibility: Dismiss BlockMover tooltips on escape key press. #15578

Accessibility: Dismiss BlockMover tooltips on escape key press. #15578

Conversation

nicolad commented May 11, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 13, 2019

aduth commented May 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

nicolad commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jun 25, 2019

nicolad commented Jul 24, 2019

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

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

tellthemachines commented Mar 16, 2020

nicolad commented Jun 22, 2020

nicolad commented Feb 5, 2021 • edited Loading

tellthemachines commented Feb 8, 2021

nicolad commented Feb 8, 2021

nicolad commented May 11, 2019 •

edited

Loading

nicolad commented Feb 5, 2021 •

edited

Loading