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

Feat: avoid multiple tooltips showing up simultaneously #6937

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

scomea
Copy link
Collaborator

@scomea scomea commented Apr 9, 2024

📖 Description

The current tooltip behavior can show multiple tooltips at the same time, most commonly a tooltip that shows because an element has been focused remains visible when another element is hovered. Or a hovered tooltip remains visible as the user tabs to other controls.

After this change an active tooltip monitors mouse movement and focus changes as the document level so a hover driven tooltip is dismissed when another element is focused, and a focus driven tooltip is dismissed when the mouse moves.

Also ensured we removed listeners on disconnect.

🎫 Issues

Multiple user focus driven tooltips can show up simultaneously and could easily overlap:

image

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

@scomea scomea force-pushed the users/scomea/tooltip-move branch from 33082b4 to 20d71ab Compare April 9, 2024 23:37
Stephane Comeau added 2 commits April 9, 2024 20:41
@olaf-k
Copy link
Contributor

olaf-k commented Apr 10, 2024

I think the current implementation is worth keeping alongside this new behavior.

as implementation goes, aiming at only having one tooltip displayed at a time could be achieved without resorting to add new listeners in each instance and simply keep one reference at class level.

e.g.,

static currentTooltipInstance: Tooltip | null = null;

then, at the start of showTooltip add

if (Tooltip.currentTooltipInstance !== this && Tooltip.currentTooltipInstance?.hideTooltip)
	Tooltip.currentTooltipInstance.hideTooltip();
Tooltip.currentTooltipInstance = this;

@scomea
Copy link
Collaborator Author

scomea commented Apr 10, 2024

I think the current implementation is worth keeping alongside this new behavior.

as implementation goes, aiming at only having one tooltip displayed at a time could be achieved without resorting to add new listeners in each instance and simply keep one reference at class level.

e.g.,

static currentTooltipInstance: Tooltip | null = null;

then, at the start of showTooltip add

if (Tooltip.currentTooltipInstance !== this && Tooltip.currentTooltipInstance?.hideTooltip)
	Tooltip.currentTooltipInstance.hideTooltip();
Tooltip.currentTooltipInstance = this;

Not convinced about keeping the old behavior as I think it is provably broken with the overlapping cases. The static prop is a good idea and much leaner, will give it a go. (worked!)

@scomea scomea changed the title Feat: improve tooltip show/hide behavior Feat: avoid multiple tooltips showing up simultaneously Apr 11, 2024
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.

None yet

2 participants