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.

Sign up for GitHub

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

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

Closed
wants to merge 22 commits into from
+18 −1
Closed

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

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
ae832dc
Accessibility: dismiss BlockMover tooltips on escape key press.
nicolad May 11, 2019
dc86b93
Accessibility: Make tooltips dismissable onMouseDown.
nicolad May 11, 2019
9a61a03
Merge branch 'master' into accessibility/improve-tooltips-behavior
nicolad May 12, 2019
861351c
Merge branch 'master' into accessibility/improve-tooltips-behavior
nicolad May 13, 2019
ceae20d
Merge branch 'master' into accessibility/improve-tooltips-behavior
nicolad May 14, 2019
62b163d
Merge branch 'master' into accessibility/improve-tooltips-behavior
nicolad May 14, 2019
60bbc6c
Imported KeyboardShortcuts into Tooltip.
nicolad May 20, 2019
8ae4d23
Fixed KeyboardShortcuts import and wrapped children with this.
nicolad May 20, 2019
4379cf3
Updated the import and wrapped popover content.
nicolad May 20, 2019
919831f
Wrapped correct elements.
nicolad May 25, 2019
f0962b3
test-unit:update summary --- 7 snapshots updated from 2 test suites.
nicolad May 25, 2019
9741f77
unit-tests temp fix
nicolad May 25, 2019
ee9b1a5
Conflicts fixed.
nicolad Jun 18, 2019
0331775
Merge branch 'master' of github.com:WordPress/gutenberg into accessib…
nicolad Jun 30, 2019
67cfdf7
Merge branch 'master' of github.com:WordPress/gutenberg into accessib…
nicolad Jul 24, 2019
b23a066
Removed KeyboardShortcuts and checked escape key press.
nicolad Jul 24, 2019
2dfaf5b
Updated snapshots.
nicolad Jul 24, 2019
10e7a32
Merge branch 'master' into accessibility/improve-tooltips-behavior
nicolad Sep 14, 2019
a6f7be2
Moved destructuring back.
nicolad Sep 14, 2019
22ef4e2
Merge branch 'master' of github.com:WordPress/gutenberg into accessib…
nicolad Oct 8, 2019
68dd831
Removed obsolete onMouseEnter and onMouseLeave handlers.
nicolad Oct 8, 2019
87321bd
Conflicts fixed.
nicolad Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/block-mover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export class BlockMover extends Component {
aria-disabled={ isFirst }
onFocus={ this.onFocus }
onBlur={ this.onBlur }
onMouseEnter={ this.onFocus }
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

onMouseLeave={ this.onBlur }
/>
<IconDragHandle
className="editor-block-mover__control block-editor-block-mover__control"
Expand All @@ -85,6 +87,8 @@ export class BlockMover extends Component {
aria-disabled={ isLast }
onFocus={ this.onFocus }
onBlur={ this.onBlur }
onMouseEnter={ this.onFocus }
onMouseLeave={ this.onBlur }
/>
<span id={ `block-editor-block-mover__up-description-${ instanceId }` } className="editor-block-mover__description block-editor-block-mover__description">
{
Expand Down
12 changes: 12 additions & 0 deletions packages/components/src/color-picker/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ exports[`ColorPicker should commit changes to all views on blur 1`] = `
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down Expand Up @@ -321,6 +323,8 @@ exports[`ColorPicker should commit changes to all views on keyDown = DOWN 1`] =
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down Expand Up @@ -494,6 +498,8 @@ exports[`ColorPicker should commit changes to all views on keyDown = ENTER 1`] =
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down Expand Up @@ -667,6 +673,8 @@ exports[`ColorPicker should commit changes to all views on keyDown = UP 1`] = `
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down Expand Up @@ -840,6 +848,8 @@ exports[`ColorPicker should only update input view for draft changes 1`] = `
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down Expand Up @@ -1013,6 +1023,8 @@ exports[`ColorPicker should render color picker 1`] = `
onBlur={[Function]}
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

Readability improvement.

Copy link
Member

Choose a reason for hiding this comment

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

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.

)
);

Expand Down
9 changes: 8 additions & 1 deletion packages/components/src/tooltip/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ class Tooltip extends Component {
return;
}

// If pressed key is escape, no further actions are needed.
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 ) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @aduth, updated this.

return;
}

// Needed in case unsetting is over while delayed set pending, i.e.
// quickly blur/mouseleave before delayedSetIsOver is called
this.delayedSetIsOver.cancel();
Expand All @@ -90,6 +95,7 @@ class Tooltip extends Component {

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if ( Children.count( children ) !== 1 ) {
if ( 'development' === process.env.NODE_ENV ) {
// eslint-disable-next-line no-console
Expand All @@ -100,11 +106,12 @@ class Tooltip extends Component {
}

const child = Children.only( children );
const { isOver } = this.state;
return cloneElement( child, {
onMouseEnter: this.createToggleIsOver( 'onMouseEnter', true ),
onMouseLeave: this.createToggleIsOver( 'onMouseLeave' ),
onMouseDown: this.createToggleIsOver( 'onMouseDown' ),
onClick: this.createToggleIsOver( 'onClick' ),
onKeyDown: this.createToggleIsOver( 'onKeyDown' ),
onFocus: this.createToggleIsOver( 'onFocus' ),
onBlur: this.createToggleIsOver( 'onBlur' ),
children: concatChildren(
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/tooltip/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.components-tooltip.components-popover {
z-index: z-index(".components-tooltip");
cursor: default;

&::before {
border-color: transparent;
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-post/src/components/header/more-menu/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ exports[`MoreMenu should match snapshot 1`] = `
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
>
Expand All @@ -56,6 +57,7 @@ exports[`MoreMenu should match snapshot 1`] = `
onClick={[Function]}
onFocus={[Function]}
onKeyDown={[Function]}
onMouseDown={[Function]}
onMouseEnter={[Function]}
onMouseLeave={[Function]}
type="button"
Expand Down