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

Components: Avoid displaying tooltip on focus from mousedown #16800

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 29, 2019

This pull request seeks to resolve an issue where a button with an associated tooltip will briefly flash the tooltip when clicked. The underlying issue here is that a tooltip is expected to be shown (immediately) when a button is focused. Previously, this was not disambiguated from focus which results from clicking to interact with the button, where the tooltip is not expected to be shown.

Before After
tooltip-before tooltip

Testing instructions:

Verify there are no regressions in the display of the tooltips being shown upon focusing the button.

Verify that the tooltip is not shown when clicking on a button with an associated tooltip (e.g. inserter, block movers, etc).

Needs Accessibility Feedback: I'd appreciate any advice on whether I've overlooked any use cases with the assumption that focus-via-click should be ignored.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components Needs Accessibility Feedback Need input from accessibility labels Jul 29, 2019
@aduth aduth requested a review from mtias July 29, 2019 20:50
@gziolo
Copy link
Member

gziolo commented Jul 30, 2019

It looks like this change invalidated several snapshots saved for unit tests:
https://travis-ci.com/WordPress/gutenberg/jobs/220649539

I guess npm run test-unit -- -u is something that will solve it.

@@ -35,13 +35,29 @@ class Tooltip extends Component {
TOOLTIP_DELAY
);

/**
Copy link
Member

Choose a reason for hiding this comment

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

Should this be just a multiline comment with //?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be just a multiline comment with //?

It's meant to be JSDoc describing the instance variable. I guess including @type (like in the subsequent one) could help make this clearer).

@aduth
Copy link
Member Author

aduth commented Jul 30, 2019

It looks like this change invalidated several snapshots saved for unit tests:
https://travis-ci.com/WordPress/gutenberg/jobs/220649539

I guess npm run test-unit -- -u is something that will solve it.

Thanks. Updated in 3363cb6.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code looks good and it works as advertised in my testing 👍

I didn't discover any regressions when using keyboard only and tabbing between icon-only buttons.

@aduth aduth merged commit 208ff09 into master Jul 30, 2019
@aduth aduth deleted the fix/button-tooltip-click branch July 30, 2019 20:07
@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 6, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Components: Avoid displaying tooltip on focus from mousedown

* Testing: Update Tooltip snapshots

* Components: Tooltip: Add JSDoc type for assigned cancelIsMouseDown
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Components: Avoid displaying tooltip on focus from mousedown

* Testing: Update Tooltip snapshots

* Components: Tooltip: Add JSDoc type for assigned cancelIsMouseDown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Needs Accessibility Feedback Need input from accessibility [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants