Remove tooltip focus on mousemove by adfaris · Pull Request #3335 · elastic/eui

adfaris · 2020-04-16T16:37:12Z

Summary

Resolves #2560

When a tooltip is opened via tab focus, any mouse movement will now close it. There will not be two tooltips opened at the same time.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation examples
- [ ] Added or updated jest tests
- [ ] Checked for breaking changes and labeled appropriately
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

kibanamachine · 2020-04-16T16:37:15Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cla-checker-service · 2020-04-16T16:37:17Z

💚 CLA has been signed

kibanamachine · 2020-04-16T16:37:26Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cchaos · 2020-04-16T18:13:55Z

👋 Welcome, @adfaris! Can you please sign the CLA? Be sure to sign it with the same email address as your github account.

Be sure to also checkout our Contributing guide.

adfaris · 2020-04-16T19:42:44Z

👋 Welcome, @adfaris! Can you please sign the CLA? Be sure to sign it with the same email address as your github account.

Be sure to also checkout our Contributing guide.

Hi @cchaos,

Thank you for the warm welcome.

I just verified my email settings on GitHub and resigned the form. No updates yet.

adfaris · 2020-04-16T19:43:54Z

👋 Welcome, @adfaris! Can you please sign the CLA? Be sure to sign it with the same email address as your github account.
Be sure to also checkout our Contributing guide.

Hi @cchaos,

Thank you for the warm welcome.

I just verified my email settings on GitHub and resigned the form. No updates yet.

Nevermind, it worked. Thanks

cchaos · 2020-04-16T19:46:10Z

Jenkins, test this

kibanamachine · 2020-04-16T20:06:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3335/

adfaris · 2020-04-16T20:28:15Z

Test passed locally and should pass in CI now.

cchaos · 2020-04-16T20:39:41Z

src/components/tool_tip/tool_tip.tsx

      hasFocus: true,
    });
    this.showToolTip();
+    window.addEventListener('mousemove', this.hasFocusMouseMoveListener);


Hmm, my main concern is adding this handler to hide the tooltip on any mouse movement. Example:

It means that if the element that has a tooltip is interact-able (can have a focus state) and is focused, the tooltip disappears almost immediately when clicked on because of any slight movement with the mouse. It can just be a bit funky to interact with.

I offer this worry, but without a solid solution. Like a global tooltip service that manages the number of tooltips visible, which sounds like a headache from a system point of view. Or if #2560 is truly an issue that arises often or an issue at all.

The behavior for mousing over is not changed. It only listens for and closes the tooltip on any mousemove when the tooltip is opened via tabbing onto it on the keyboard.

When I view this in the build for this PR: https://eui.elastic.co/pr_3335/#/display/badge, the tooltip only closes on mouseout, like before, not on any mousemove.

I figured that this was the only practical solution, because switching from using a keyboard to a mouse seems appropriate to close a tooltip. I also think that a global registry may be more headache than it's worth.

This event listener is added in the onFocus method which is only called when it is entered via keyboard tab.

Yep, totally understand. However, you can enter the focus state with a mouse as well. For instance, if the anchoring element is just a toggle or a switch and doesn't actually navigate the user anywhere. Once the user clicks on the anchor (switch), any subsequent movement will close the tooltip.

Ah, I see. Thanks.

Maybe we can be more specific on the event handler by applying the event listener only if it was a focus event with the tab key index as well. Thoughts?

adfaris · 2020-04-21T18:18:20Z

@cchaos. Please re-review when you get a chance. I have made the necessary changes and everything is working.

cchaos · 2020-04-23T13:54:17Z

Jenkins, test this

adfaris · 2020-04-23T15:37:19Z

Build is failing because it can't install yarn.

cchaos · 2020-04-23T15:39:37Z

retest

kibanamachine · 2020-04-23T15:59:49Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3335/

cchaos · 2020-05-13T20:51:20Z

Hi again @adfaris. Sorry for the extremely late response on this one. I've been going back and forth in my head whether the combination of keyboard tabbing and mousing around would be normal practice or if it would be odd to see the tooltip disappear when doing so.

Eventually I've landed on, no, I don't think this is a common case nor would it look unintended. So in that regard, I think this is a good final solution 💯

I think all it needs now is a Changelog entry since this does effect the shipped components.

adfaris · 2020-05-15T14:36:42Z

The CHANGELOG has been updated.

cchaos · 2020-05-15T14:55:58Z

Thanks @adfaris, looks like you'll need to rebase with master since there's been several releases and the changelog is out of date.

cchaos · 2020-05-15T18:11:50Z

Hmm, that rebase didn't go so well, I'll get it cleaned up for you.

cchaos · 2020-05-15T18:52:04Z

retest

src/components/tool_tip/tool_tip.tsx

kibanamachine · 2020-05-15T19:11:57Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3335/

src/components/tool_tip/tool_tip.tsx

chandlerprall · 2020-05-21T19:56:12Z

jenkins test this

kibanamachine · 2020-05-21T20:17:01Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3335/

chandlerprall

Gave this another run through on the docs to double check and found that the tooltip is disabled for keyboard users after it is triggered by the mouse

…mouse

adfaris · 2020-05-22T18:08:52Z

Gave this another run through on the docs to double check and found that the tooltip is disabled for keyboard users after it is triggered by the mouse

@chandlerprall

Please re-review it when you get a chance. The bug has been fixed.

chandlerprall

Changes LGTM, will merge on green CI. Thanks @adfaris !

chandlerprall · 2020-05-27T20:13:01Z

jenkins test this

kibanamachine · 2020-05-27T20:35:59Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3335/

chandlerprall · 2020-05-27T20:52:06Z

jenkins test this

kibanamachine · 2020-05-27T21:13:22Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3335/

This reverts commit fd3dd84. # Conflicts: # CHANGELOG.md # src/components/tool_tip/tool_tip.tsx

…orce visibility while element is focused (#5066) * Revert "Remove tooltip focus on mousemove (#3335)" * Applying `onFocus` and `onBlur` methods to child clone * Put default value for `display` in `defaultProps` so it populates in the props table * [EuiIconTip] Removing `children` from TS

cchaos reviewed Apr 16, 2020

View reviewed changes

adfaris requested a review from cchaos May 15, 2020 14:37

adfaris added 7 commits May 15, 2020 14:13

Remove tooltip focus on mousemove

135529d

Fix linting error

34cc150

Removed the onFocus and onBlur since it is not used

d71f637

Added onKeyUp

e679af3

Update the CHANGELOG

ebf8ab6

Remove tooltip focus on mousemove

d425bd1

Removed the onFocus and onBlur since it is not used

ea1a6fa

cchaos force-pushed the tooltip-to-close-with-mouse-move branch from 6be9742 to ea1a6fa Compare May 15, 2020 18:49

cchaos added 2 commits May 15, 2020 14:51

cl

3706f08

cl

a6f9fbd

chandlerprall suggested changes May 15, 2020

View reviewed changes

src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved

src/components/tool_tip/tool_tip.tsx Show resolved Hide resolved

src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved

Fixed the tooltip issue based feedback

4992a90

chandlerprall suggested changes May 21, 2020

View reviewed changes

src/components/tool_tip/tool_tip.tsx Outdated Show resolved Hide resolved

Add removeEventListener to componentWillUnmount

8a09ef7

chandlerprall suggested changes May 21, 2020

View reviewed changes

Fixed the bug displaying tooltip from keyboad after triggered by the …

dca4496

…mouse

adfaris force-pushed the tooltip-to-close-with-mouse-move branch from 7fdac01 to dca4496 Compare May 22, 2020 18:05

Merge branch 'master' into tooltip-to-close-with-mouse-move

c83f471

adfaris requested a review from chandlerprall May 22, 2020 18:09

chandlerprall approved these changes May 27, 2020

View reviewed changes

clean up eslint warning

d9f188e

chandlerprall merged commit fd3dd84 into elastic:master May 28, 2020

thompsongl mentioned this pull request Jun 3, 2020

Upgrade EUI to v24.1.0 elastic/kibana#68141

Merged

cchaos mentioned this pull request Aug 11, 2021

[EuiTooltip] Add an "alwaysVisible" prop #5013

Closed

cchaos pushed a commit to cchaos/eui that referenced this pull request Aug 23, 2021

Revert "Remove tooltip focus on mousemove (elastic#3335)"

9090925

This reverts commit fd3dd84. # Conflicts: # CHANGELOG.md # src/components/tool_tip/tool_tip.tsx

cchaos mentioned this pull request Aug 23, 2021

[EuiTooltip] Revert "Remove tooltip focus on mousemove (#3335)" and force visibility while element is focused #5066

Merged

7 tasks

Conversation

adfaris commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Apr 16, 2020

Uh oh!

cla-checker-service bot commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Apr 16, 2020

Uh oh!

cchaos commented Apr 16, 2020

Uh oh!

adfaris commented Apr 16, 2020

Uh oh!

adfaris commented Apr 16, 2020

Uh oh!

cchaos commented Apr 16, 2020

Uh oh!

kibanamachine commented Apr 16, 2020

Uh oh!

adfaris commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

adfaris Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adfaris Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

adfaris Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

adfaris commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos commented Apr 23, 2020

Uh oh!

adfaris commented Apr 23, 2020

Uh oh!

cchaos commented Apr 23, 2020

Uh oh!

kibanamachine commented Apr 23, 2020

Uh oh!

cchaos commented May 13, 2020

Uh oh!

adfaris commented May 15, 2020

Uh oh!

cchaos commented May 15, 2020

Uh oh!

cchaos commented May 15, 2020

Uh oh!

cchaos commented May 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented May 15, 2020

Uh oh!

Uh oh!

chandlerprall commented May 21, 2020

Uh oh!

kibanamachine commented May 21, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

Uh oh!

adfaris commented May 22, 2020

Uh oh!

chandlerprall left a comment

Choose a reason for hiding this comment

adfaris commented Apr 16, 2020 •

edited

Loading

cla-checker-service bot commented Apr 16, 2020 •

edited

Loading

adfaris commented Apr 16, 2020 •

edited

Loading

adfaris Apr 16, 2020 •

edited

Loading

adfaris commented Apr 21, 2020 •

edited

Loading