Button: always render the Tooltip component even when a tooltip should not be shown #56490
Conversation
ciampo
added
[Type] Bug
ciampo
requested review from
afercia,
andrewhayward,
brookewp,
fabiankaegy and
tellthemachines
|
Size Change: -44 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
It looks like some |
|
After taking a look, failures seem related to the fact that With the changes from this PR, since I would argue that this scenario has been around on We have two main approaches that we could take:
What do folks think? |
Alternative solution (specific to the save button) in #56502 |
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
This is a fair compromise IMHO 👍
This PR would, unfortunately, introduce a regression to any consumer that is rendering
<Tooltip><Button /></Tooltip>
Is it even possible that there is such an instance existing and it's not broken in the same way as the test breaks?
Right now, a consumer may render
I will also try one more approach using the |
Opened #56527, although I'm not managing to fix the issue. We may have to resort to using the fix proposed in this PR. In order to mitigate the disruption, one potential fix may be to add some logic to the Let me know if this sounds like a good ide. |
This is definitely not great and could potentially break existing usages.
I think this sounds like a good compromise. Do we have any usages in the Gutenberg core that will be affected? |
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
e0b8ee3 to
aa8537b
Compare
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
aa8537b to
b93864f
Compare
| ( ( showTooltip && !! label ) || | ||
| // There's a shortcut or... | ||
| shortcut || | ||
| !! shortcut || |
There was a problem hiding this comment.
Making sure that shouldShowTooltip is an actual boolean
There was a problem hiding this comment.
Changes in this file mostly undo the temporary fix from #56502
|
Update:
Until #57202 (or any alternative solution) is merged, this PR will stay on hold. |
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
1e657eb to
95824d5
Compare
|
Rebased to include changes from #57202. Will look into unit test failures — there may be some extra unintended consequences when nesting tooltips in |
There was indeed an issue with how props were forwarded / merged / passed to the I will rebase this PR on top of |
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
e66abe6 to
ad3b6f5
Compare
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
6c052b7 to
250350c
Compare
|
With #57878, #57975 and #58125 merged, we should be finally ready for a final round of reviews. I've also added a unit test which should fail on |
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
250350c to
03404d4
Compare
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Code looks good, and is testing well 🚀
| let computedPlacement; | ||
| // if `tooltipPosition` is defined, compute value to `placement` | ||
| if ( tooltipPosition !== undefined ) { | ||
| computedPlacement = positionToPlacement( tooltipPosition ); | ||
| } |
…uld not be shown
ciampo
force-pushed
the
fix/button-tooltip-re-render
branch
from
03404d4 to
36fea4d
Compare
|
@ciampo Why would a button switch from having a tooltip to not having a tooltip while it's rendered in the DOM? What's the use case? Edit: I read the testing instructions, so the save button :) |
|
This is causing so much work to be done for buttons that never even show a tooltip. More often than not, buttons never have a tooltip. Why is this there even a tooltip option for a button, why is this not an optional wrapper component? |
FWIW, I would definitely not include a tooltip (and its floating-ui dependencies) if we were to design a Button component from scratch. Maybe in a v2! |
@ellatrix These are all correct questions that we also asked ourselves. Unfortunately the decision to include the I agree with your observations and, as also Lena said, if we were to build a |
Fixes #56522
Requires #57202, #57878, #57975, #58125
What?
This PR tweaks the
Buttoncomponent so that it always renders theTooltipcomponent, even when a tooltip shouldn't be shown.It also un-does the temporary fix from #56502
Why?
This change is necessary to fix the issue described in #44735, where basically the element rendered by
Buttonwould be removed and re-added to the DOM any time theButtoncomponent switches from having to show a tooltip to not having to show it, and vice-versa.The fact that a new HTML element is re-created every time triggers a focus loss that can be very annoying, especially to assistive technology users.
How?
After some debugging, the issue seems related to the fact that React couldn't reconciliate the "with tooltip" and "without tooltip" version of the
Buttoncomponent without re-creating the whole button every time.To solve this, I decided to tweak
Buttonso that it always renders theTooltipcomponent.Note: this doesn't mean that the tooltip will always be shown, though — the original user-facing behaviour should have been retained by only passing props to the
Tooltipcomponent when needed.Testing Instructions
Testing Instructions for Keyboard
From #44735 (comment):
A similar approach can also be used directly in Storybook.
Also, make sure that in general, tooltips show on
Buttonwhen they are supposed to, and don't show when they are not supposed to.