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

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

Merged
merged 13 commits into from
Aug 24, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Aug 23, 2021

Closes #5013 by reverting #3335 and applied the onFocus and onBlur event handlers to the cloned child which basically means that while the wrapped item has focus the tooltip will never close.

Before

Screen.Recording.2021-08-23.at.12.03.17.PM.mp4

Just reverting

Screen.Recording.2021-08-23.at.12.07.00.PM.mp4

Forces visibility while in focus

Screen.Recording.2021-08-23.at.12.19.20.PM.mp4

What would be even better

Is if we could also keep the tooltip shown when a user tries to interact with the contents. I know we've mostly said that interactive content should not be placed within tooltips, but use popovers instead. I still think this is correct, but I do think it would be helpful for when users might want to copy text.

Example of it not working right now:

Screen.Recording.2021-08-23.at.12.30.56.PM.mp4

Thought process:

So I've been thinking about solutions to this issue, remembering there was a "resolved" issue around closing tooltips on every mouse out/move, even when focused (#2560 & #3335). Looking at that specific issue, it seems the behavior being circumvented was when there were two or more elements in close proximity with tooltips and one is focused. I feel like most recently I've been more frustrated with tooltips closing when I don't want them to and seen less tooltips this close together.

@chandlerprall Since you were the one who originally opened the issue, was there more than just the overlapping tooltip issue?

My inclination, honestly, is just to revert #3335 or at least remove the mouse out/move listener when the element is focused.

Originally posted by @cchaos on #5013 (comment)

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Props have proper autodocs and playground toggles
  • Added documentation
    - [ ] Checked Code Sandbox works for the any docs 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

cchaos added 2 commits August 19, 2021 18:44
This reverts commit fd3dd84.

# Conflicts:
#	CHANGELOG.md
#	src/components/tool_tip/tool_tip.tsx
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Aug 23, 2021

FYI: I've decided to update the docs too, working on that now. Reviews can hold until I've pushed the updates.

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor Author

cchaos commented Aug 23, 2021

Ok, this is reviewable again 😸

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Tested in the deployed docs & verified some changes locally

@cchaos cchaos enabled auto-merge (squash) August 24, 2021 19:15
@kibanamachine
Copy link

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

@cchaos cchaos merged commit ecaaa0a into elastic:master Aug 24, 2021
@diegozubieta95 diegozubieta95 mentioned this pull request Mar 2, 2022
2 tasks
@cchaos cchaos deleted the revert/3335_tooltip_focus branch March 4, 2022 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiTooltip] Add an "alwaysVisible" prop
3 participants