Feat: avoid multiple tooltips showing up simultaneously

scomea commented 5 months ago

📖 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:

✅ Checklist

General

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

Component-specific

[ ] I have added a new component
[x] I have modified an existing component
[ ] I have updated the definition file
[ ] I have updated the configuration file

olaf-k commented 5 months ago

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 commented 5 months ago

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

janechu commented 2 months ago

We're avoiding feature work for now due to closing out of #6951, I do think this is a good addition we're just focused on fixes as part of our strategy to close out the work on foundation.

microsoft / fast