Tooltip: Fix positioning by anchoring to child element #41268
Conversation
andrewserong
added
[Type] Bug
|
Size Change: +377 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
| * @param {string} position A position string such as 'bottom left' | ||
| * @return {string} A placement string such as 'bottom-start' | ||
| */ | ||
| const positionToPlacement = ( position ) => { |
There was a problem hiding this comment.
This transformation is necessary because otherwise we end up with the following issue (tested in Storybook):
| Before | After |
|---|---|
 |
 |
I'm not too sure if I've understood the switched left / right logic in the Popover, but so far in testing this function appears to be working correctly for the ToolTip to me. Very happy for feedback, though, if anyone has ideas for how to improve this 🙂
There was a problem hiding this comment.
This confused me as well.
It's a pity that Popover and Tooltip would have "opposite" behaviour in terms of how left and right work.
andrewserong
requested review from
youknowriad,
ciampo,
aaronrobertshaw and
jorgefilipecosta
|
I've pinged a few folks who reviewed #40740. Apologies for the liberal use of pinging / adding reviewers here — I wasn't too sure if the way I've gone about fixing this is the right approach, so just wanted to make sure y'all could see the PR just in case I missed some details about how best to work with the refactored Popover. Thanks! |
| @@ -194,6 +194,7 @@ const Popover = ( | |||
| shift( { | |||
| crossAxis: true, | |||
| limiter: limitShift(), | |||
| padding: 4, | |||
There was a problem hiding this comment.
Why is this necessary, I fear it would have an impact on the block toolbar...
There was a problem hiding this comment.
Good point, I thought maybe we could use some clearance between the edge of the screen and the Tooltip, but from looking at other web apps (e.g. Gmail) it looks like it's pretty common for Tooltips to go flush up against the viewport. I've removed this line.
There was a problem hiding this comment.
Update: turns out without setting padding the Tooltip jumps around a bit when it hits the edge of the viewport:
I'll look into a fix tomorrow.
| @@ -212,9 +259,12 @@ function Tooltip( props ) { | |||
| ? getDisabledElement | |||
| : getRegularElement; | |||
|
|
|||
| const placement = positionToPlacement( position ); | |||
There was a problem hiding this comment.
Why do you think we need this here? Is the "positionToPlacement" in the "Popover" component not working properly? Should we fix that one instead?
There was a problem hiding this comment.
Left a few inline comments, focusing more on the component in isolation and less on its integration in the editor (which I'm sure other folks can also help with).
I also realised that we also have an experimental Tooltip component — it would be interesting to assess if we should look into using that "new" implementation or discard it.
| * @param {string} position A position string such as 'bottom left' | ||
| * @return {string} A placement string such as 'bottom-start' | ||
| */ | ||
| const positionToPlacement = ( position ) => { |
There was a problem hiding this comment.
This confused me as well.
It's a pity that Popover and Tooltip would have "opposite" behaviour in terms of how left and right work.
| * @param {string} position A position string such as 'bottom left' | ||
| * @return {string} A placement string such as 'bottom-start' | ||
| */ | ||
| const positionToPlacement = ( position ) => { |
There was a problem hiding this comment.
Should we add some more unit tests to make sure that this function works properly?
There was a problem hiding this comment.
Yes, good idea, I'll add some tests tomorrow.
| const childRef = useRef( null ); | ||
| const mergedChildRefs = useMergeRefs( [ |
There was a problem hiding this comment.
Maybe we could add some inline code to explain why we're storing both childRef and mergedChildRefs (which are being used later in code in the getElementWithPopover function)
There was a problem hiding this comment.
Added an inline comment to hopefully clear this one up a bit.
There was a problem hiding this comment.
We need the first child's ref now because cloneElement would previously preserve the ref, correct? Also can you explain a bit why would we need the childRef in getRegularElement? Just trying to understand it fully 😄
There was a problem hiding this comment.
This one was the result of trial and error so I'm not 100% sure how it's working but here's my current understanding:
- We need a stable ref to the first child to be passed to the Popover so that it can calculate the correct position. In order to pass it reliably to the Popover without causing any errors, by using an additional
childRefwe have the one reference that is pointing to the current child element. - Because we can't know whether or not the first child of the Tooltip is already using a
reffor another reason, we need to attempt to merge it in with thechildRefwe're using for theanchorRef, this way we don't accidentally overwrite arefused for other purposes. - In
getRegularElementandgetDisabledElementwe specify the mergedRef so that both refs are attached correctly to the cloned element (any existing ref is preserved, and our additionalchildReffor the Popover can be used for position calculation)
Let me know if you think any changes are needed here, or if we can make it clearer what's going on in the inline comment!
There was a problem hiding this comment.
Thanks for explaining! I'm afraid I can't suggest something right now, since I tried doing what I asked above, that is pass only the first child's ref if exists to cloneElement and only pass childRef var as the anchorRef without success.. It would be great though if we were 100% sure for the solution --cc @ellatrix @diegohaz if they have any insights.
There was a problem hiding this comment.
I'm having a hard time understanding what's going on here, but it's definitely good practice to create new refs for each purpose and not try to reuse refs created elsewhere. So the use of useMergeRefs seems fine here, if that's what you're asking.
Seeing Children.toArray( children )[ 0 ]?.ref makes me frown a little though. Can't you use the child variable that you have as well?
There was a problem hiding this comment.
So the use of useMergeRefs seems fine here, if that's what you're asking.
Thanks for confirming, I think that gets at the main issue?
Seeing Children.toArray( children )[ 0 ]?.ref makes me frown a little though. Can't you use the child variable that you have as well?
This is an interesting one — I had a go at consolidating with the exiting child variable, but it results in a few subtle issues:
- React's
Children.only()function throws an error rather than returningundefinedso we can't use it before the conditional to check how many children there are. - Because we're using a hook here, we have the call to the hook after a conditional.
- If we switch
childto use theChildren.toArray( children )[ 0 ]approach, then an incorrectkeygets copied when the element is duplicated, which causes other components' tests to fail (I found that the hard way in 90f5541)
So, as far as I can tell the best trade-off we have so far is this slightly awkward way of accessing the child in order to retrieve the existing ref (if it exists). I've tidied this up a tiny bit by storing the value in const existingChildRef to see if that helps with clarity.
Let me know if either of you can think of a better way to handle this, I've very rarely had to use React.Children directly so I am a little unfamiliar with this part of the API. I'm happy to keep iterating now or in follow-ups.
| isOver, | ||
| position, | ||
| placement, |
There was a problem hiding this comment.
Should we consider updating the APIs of Tooltip to match the ones of Popover? (i.e. adding the placement prop and deprecating the position prop)
There was a problem hiding this comment.
That's a good question, while digging into this, it seems that we need a little more smarts for the Tooltip positioning. "bottom left" means something different to "bottom-start", and in the case of the Tooltip — I think I'd be keen to retain position as it implies implicit offset logic, but potentially also expose placement. I'll write a separate longer comment with a couple of thoughts.
andrewserong
changed the title
|
Thanks for the feedback @ciampo and @youknowriad! I've gotten a little closer to coming up with a slightly more thorough fix, with honing in some of the underlying issues. I've pushed a change in 8001942, but it needs some tidying up and for me to add some tests which I'll look into tomorrow.
I spent some time digging into this question, and I think we have some subtle issues to tease apart. The main issue appears to be the gap between the idea of the placement point and the expectations of the existing position prop. In floatingUI, we set a placement point, and then the element extends in the opposite direction from that alignment. So, in the original Popover PR, flipping However, Tooltips, being quite small, have a slightly different expectation associated with them. I think for a Tooltip, when we say "bottom left" we probably mean something closer to "center the Tooltip at the bottom left corner of this element". The good news is that we can pass a function to
This has a couple of flow on implications if it seems like an okay approach:
Anyway, those are my thoughts from going down a bit of a floatingUI rabbit hole today. I'll tidy up the approach tomorrow if there aren't any objections, and then get the PR ready for review again. But, do let me know if you think there's a different direction I should be pursuing 🙂 |
It looks like that one uses Reakit for the Tooltip, rather than Popover. It'd probably be good to consolidate on the one approach, so I imagine if/when this PR lands, we might be able to look at a refactor? (But let me know if you think we should do it the other way around) |
Thank you for sharing these thoughts. I think that we shouldn't have both
Makes sense — it'd be great to clean this duplication up |
|
Part of the issue in this PR (minimum width of the popover) will be resolved by #41361, if that PR looks viable. I'll put this PR on hold in lieu of that PR, then can rebase this one to see about a possibly more minimal change just for the Tooltip. |
|
The main issue to resolve with this PR has now been merged separately in #41361 (the Tooltip being cut off at the edge of the viewport). The other idea in this PR (to tweak the position of the Tooltip so that it's anchored correctly to the child element) would still be good to get in, but is lower priority, so I'm going to set this PR aside for the moment while I work on some higher priority tasks, and then will 🤞 get back to this next week. Thanks again for the 👀, all! |
|
Thanks for looking into this. Just wanted to note that this is also an accessibility issue. Right now, on trunk, the tooltip may appear on top of some input fields, making impossible for keyboard users to actually see the value they're entering. For example. see the Margin control in the screenshot below:
On this branch, the tooltip position is OK:
|
|
Thanks for taking a look (and for the reminder about this PR) @afercia 😀 I'll aim to dust this PR off and rebase it this week to see if we can progress it toward being mergable. Cheers! |
andrewserong
force-pushed
the
fix/tooltip-position-relative-to-anchor
branch
from
8001942
to
50b54ce
Compare
andrewserong
force-pushed
the
fix/tooltip-position-relative-to-anchor
branch
from
c69377f
to
b903d19
Compare
|
Thanks for taking this for a spin @ntsekouras! I think this PR is working pretty well at the moment. My one hesitation is about the Tooltip (and Popover) position → placement calculation. I've updated the Storybook example in this PR to highlight the problem, which is that the Popover's logic which translates Here's a Gif of the Storybook example with the Tooltip position set to
The URL for that example (if you're running I've tried hacking around a little bit with not much luck, but I think ideally the Popover would be aware enough to switch the placement to the correct side based on the relative size of the Popover versus its I'm curious if you (or others) think it's worth getting this PR in as-is, even though the positioning is reversed when the Tooltip is narrower than its |
I can't see any difference regarding the width of the ref 🤔 . I can see the issue for your concern though:
Is it something more? Was the position worked like this before the Popover refactor? I don't remember.. IMO this is a big improvement from trunk and I haven't observed any regression. |
| const childRef = useRef( null ); | ||
| const mergedChildRefs = useMergeRefs( [ | ||
| childRef, | ||
| Children.toArray( children )[ 0 ]?.ref, |
There was a problem hiding this comment.
Can you not use child here? That would make things a bit clearer.
There was a problem hiding this comment.
Replied in #41268 (comment)
|
Thanks for the continued reviews @ntsekouras and @ellatrix!
I probably did a bad job at explaining it, it's such a subtle/nuanced issue it's difficult to describe! 😄
Previously the Tooltip (and Popover) position situated the Tooltip at the desired location, rather than defining the direction in which it extends. So The issue technically exists prior to this PR, but is made more visible for the Tooltip now that it's anchored to an element rather than a zero width It's a little subtle to work with when it comes to the Tooltip, so it might very well be something for us to dig into in follow-ups. Personally I think this PR improves things, too, so if everyone agrees, we could always land this PR to fix the immediate positioning of Tooltips issue, and continue exploring positioning/placements issues in follow-ups. |
There was a problem hiding this comment.
I ran into this bug while working on #42352 and can confirm that it miraculously went away after merging this branch into mine.
It's a hell of a lot better than what is in trunk. I say let's merge it sooner than later.
PS nice work @andrewserong!
|
Thanks for reviewing @noisysocks! 🙇 I think merging sooner rather than later, and then seeing if we can tweak positioning (or code quality) in follow-ups sounds like a good way to go, too. @ntsekouras and @ellatrix let me know if there are any objections to merging, otherwise I'll merge this one tomorrow morning my time (just to give folks in other timezones a chance to weigh in, since there was a bit of discussion about the approach of accessing React children). |
priethor
added
the
[Focus] Accessibility (a11y)
What?
Alternative to #41176
This PR updates the ToolTip component to ensure that its position is correctly relative to the first child of the ToolTip.
Why?
Following on from #40740 where the Popover component was refactored to use the floatingUI library, it looks like the following issues were introduced:
It looks like it was also mentioned in this comment: #40740 (review)
How?
min-contentinstead of0to prevent horizontal scrollbars from appearingTesting Instructions
ToolTipcomponent is now positioned relative to its first child.Screenshots or screencast