TooltipV2: Set the tooltip position when the popover open not when the toggle event is triggered #4327
Conversation
🦋 Changeset detectedLatest commit: 78ff65b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
broccolinisoup
force-pushed
the
tooltip-position-wiggle
branch
from
ab17b4a
to
1697c8b
Compare
broccolinisoup
changed the title
| tooltipElRef.current.showPopover() | ||
| const tooltip = tooltipElRef.current | ||
| const trigger = triggerRef.current | ||
| tooltip.setAttribute('popover', 'auto') |
There was a problem hiding this comment.
This should always be set so that the browser can correctly expose the right AT nodes. We should not set it as the tooltip is being opened.
There was a problem hiding this comment.
That makes sense. Although the tooltip is not exposed to AT when it is not opened but I think it still makes sense to set the initial attributes in the render rather than when it is opened.
| @@ -23,6 +23,7 @@ const StyledTooltip = styled.div` | |||
| /* Overriding the default popover styles */ | |||
| display: none; | |||
| &[popover] { | |||
| position: absolute; | |||
There was a problem hiding this comment.
AIUI this is the main thing that is doing all the heavy lifting. I think you could just apply this line and ignore the rest and it would still work?
There was a problem hiding this comment.
Yeah definitely this line does all the heavy lifting and actually the rest is not directly related to this change but the existing issue* just became more visible when we added this line (no idea why 🤷🏻♀️ ).
*The existing issue (at least it is an issue in my opinion) is where we set the position.
Previously the flow was:
- hover/focus event is triggered
tooltip.showPopover()function is called:popover-openattribute is added to the tooltip div- Adding
:popover-opentriggers thetoggleevent as well as triggers the animation that changes the tooltip's opacity from 0 to 1 - Position is set in the
toggleevent's callback function.
I believe the wiggling issue (video in the PR description) happens because we set the position a little too late (step 5) but we make the tooltip visible in the step 3. So in this PR I am setting the position in Step 3 and that makes the toggle event redundant.
Let me know if you have any concern or suggestions.
| side: directionToPosition[direction].side, | ||
| align: directionToPosition[direction].align, | ||
| } | ||
| const {top, left, anchorAlign, anchorSide} = getAnchoredPosition(tooltip, trigger, settings) |
There was a problem hiding this comment.
This is perfect! We can't calculate boundingRect for an element with display:none, so this is the right timing.
If needed (race condition etc), we can wait for position to be set before animating the tooltip's opacity.
…e toggle event is triggered (#4327) * do not set position if it was set before * console * only position set on tooltip show * remove event listener with the same func signature * just test * calculate anchor position after popover-open class is added * clean up * comment tidy up * fix tests * changeset * move setting popover auto back in the render * update snapshots
Changelog
Tooltip element by default has
position: fixedbut this doesn't work cases where the tooltip element is after the first scroll. So the position needs to beposition: absolute. However with this change, there is another bug occurs where the tooltip element first appears in a wrongleftandrightposition therefore causes screen to wiggle. The recording below shows this behaviour.I think this occurs because we set the position when the
toggleevent is triggered and thetoggleevent is triggered after we set the tooltip element to be visible. I updated the position logic to be called in the same function we set the visibility (tooltip.showPopover()) rather than being called as a callback in thetoggleevent. I ran the tests and seems like the bug is gone.tooltip-flickering.mp4
New
Changed
toggleevent listener but now it is called when the:popover-openpseudo class is added to the tooltip element.Removed
toggleevent listening.I don’t know why we would need to listen to the toggle event, since we know exactly when the tooltip is shown. 🤷🏻♀️ It seems like one is enough if I read the docs correctly
Rollout strategy
Testing & Reviewing
Merge checklist