Tooltip: avoid unnecessary re-renders of <select> child elements #42483
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +14 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
t-hamano
changed the title
t-hamano
added
[Type] Bug
ciampo
added
the
[Feature] UI Components
ciampo
reviewed
Jul 19, 2022
There was a problem hiding this comment.
I tried on my local machine, and I can still replicate the bug — maybe I'm doing something wrong in my testing?
Also, if the problem is caused by the select component, should we try to add a fix in select-related components directly? Maybe we could look at calling stopPropagation or something like that?
|
I have created a video to test the effectiveness of this PR. (I have tested it on Windows, but since #41845 is reported on MacOS, so I think it isn't an OS issue.) 2d0ee6aa21c7e3163e22fa93f9b1a64a.mp4
|
There was a problem hiding this comment.
What a fascinating problem, thanks for digging in @t-hamano! It looks like it might not just be us struggling with odd behaviour with select elements in Firefox, I found this bug report too, that seems tangentially related: https://bugzilla.mozilla.org/show_bug.cgi?id=1430892
Similar to @ciampo, in testing this PR, I found it a bit unreliable as to when it seemed to fix the issue and when it didn't. When I moved the mouse even slightly while interacting with the select component, it would still seem to be behave strangely sometimes resulting in the wrong unit being selected.
If I removed all of the eventHandlers from the Tooltip component that seemed to fare a little bit better 🤔
- I think @ciampo makes a good point at seeing what we can do with the components in this repo that use
selectand see if there are fixes we need to implement there. But also: - The Tooltip component appears to add event handlers in order to emit events to its children — this behaviour seems to have been around a long time, but it also seems like it could be error-prone if it results in firing more events than it really should? Rather than just focusing on a single handler and element, do we need to revisit this behaviour across the
eventHandlersin the component? - This might not be at all related, but I noticed that the SelectControl component provides a
noopfunction for some of itsonmethods, e.g. here — could the presence ofnoopprops be confusing theTooltipcomponent into emitting events when it shouldn't? (This is probably a long shot, but just thought I'd mention it, because I find the logic of the Tooltip component a little challenging to follow)
Not sure if any of that is helpful, but happy to do further testing of this PR tomorrow!
|
Thanks for working on a fix for this @t-hamano 👍 While investigating #41845 earlier this afternoon, I found that the border radius control also has the same problem in Firefox that this PR attempts to resolve. A git bisect pointed to the addition of tooltips around the inner As @andrewserong and @ciampo mention, preventing the issue as close to the source as possible sounds good. I'm not sure how reliably we can count on that though if a simple After checking out this PR I didn't have much success testing. The bug is still present for me. Screen.Recording.2022-07-20.at.7.21.40.pm.mp4 |
|
I have tested on Storybook to clarify the problems we are currently experiencing with I believe the issue is a problem that occurs when a select element is wrapped in a Therefore, I think we need to handle 8fb6386eb311714d0f4ae7c584112c56.mp4 |
|
I gave this PR another spin, and I think that the reason why the fix didn't work for me (or for Aaron and Andrew) is because the same early return added to the Suggested changediff --git a/packages/components/src/tooltip/index.js b/packages/components/src/tooltip/index.js
index bbe410f50a..840232c248 100644
--- a/packages/components/src/tooltip/index.js
+++ b/packages/components/src/tooltip/index.js
@@ -133,6 +133,10 @@ function Tooltip( props ) {
};
const createMouseUp = ( event ) => {
+ if ( event.target.tagName === 'OPTION' ) {
+ return;
+ }
+
emitToChild( children, 'onMouseUp', event );
document.removeEventListener( 'mouseup', cancelIsMouseDown );
setIsMouseDown( false );Having said that, I think it still makes sense to investigate a bit further what @andrewserong suggested
With regards to where the fix should be applied:
A fix to |
|
Thank you for the detailed explanation!
May I commit the suggestion and test to see if it has been fully improved in other people's environments? And I also couldn't think of an appropriate PR title to describe this issue. |
Of course! Feel free to commit and test :) If this approach solves the issue, it would be interesting to understand if we can, as a follow-up, rewrite the Tooltip component in a less convoluted way (basically following @andrewserong's advice)
I'll see if I can come up with something! |
ciampo
changed the title
ciampo
changed the title
ciampo
approved these changes
Jul 22, 2022
There was a problem hiding this comment.
I have tested again #37856 / #38113 / #41845 which are the subjects of this resolution. In my environment, they all appear to be resolved by this PR.
Those issues seem to be fixed for me as well — thank you for working on this 🚀
It'd be great if you could work on a follow-up PR with the enhancements suggested in the previous messages.
Is this what you are referring to?
|
Yes. In particular, it looks like |
t-hamano
added
the
Browser Issues
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: #37856 / #38113 / #41845
What?
This PR prevents the
Tooltipcomponent from re-rendering child components when the children's select list is chosen.Why?
Through I was investigating #41845, I found that the behavior of the
onMouseDownevent when a select list is chosen varies from browser to browser.As mentioned in this comment, in Firefox,
onMouseDownis also fired when a select list is selected.This means that in Firefox, the
onMouseDownevent inTooltipis also triggered when the children's select list is selected.I'm not sure, but I would expect that the
Tooltip'sonMousedownfiring would cause the child component to re-render and the select element'sonChangeevent is triggered with the old value.As far as I know, this problem occurs in the following cases in Firefox:
BoxControl(BoxControlhasUnitControlcomponents surrounded by aTooltip.)SelectControlin theTooltipcomponentHow?
I have made it so that the process is canceled only when the select list is selected in the
onMouseDownevent of the Tooltip component.I think there is a smarter way to handle this.
Testing Instructions
BoxControland click "Unlink Sides".