Fix visual jitter in Combobox component when using native scrollbar
#3190
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
danthomps
commented
May 9, 2024
| @@ -1675,6 +1683,7 @@ function OptionsFn<TTag extends ElementType = typeof DEFAULT_OPTIONS_TAG>( | |||
| '--button-width': useElementSize(data.buttonRef, true).width, | |||
| } as CSSProperties, | |||
| onWheel: handleWheel, | |||
| onScroll: handleScroll, | |||
There was a problem hiding this comment.
Chose to use an onScroll event as none of the other events would fire if the first thing we move our mouse into is the native scrollbar.
danthomps
commented
May 9, 2024
If you use the scrollbar, a `mousedown` and `pointerdown` event is fired before you start interacting with the scrollbar. If you use the `scroll` event, then the callback is fired while scrolling. To improve performance a bit more, we will set the activation trigger in the `mousedown` event because we only need to set it once. If you are done scrolling, we don't reset it (we didn't do that before either), but instead we rely on other events to override the trigger (e.g.: you start using the keyboard). The big benefit is that this now only calls the `setActivationTrigger` once, instead of `n` times with the same value.
This is a similar performance trick as before. We only need to set the activationTrigger once, so there is no need to keep calling this function for no reason.
RobinMalfait
force-pushed
the
fix/combobox-scrollbar-jitter
branch
from
86c8a82
to
4e34771
Compare
RobinMalfait
changed the title
Combobox when using native scrollbarCombobox component when using native scrollbar
|
Hey! Thank you for this PR, I really appreciate it. I made some changes, here is a quick breakdown:
Thanks again for your initial work! |
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.
Fixes #3189
Bug
The dropdown will jitter when using the native scrollbar before entering the dropdown list with your cursor or using the mouse wheel to scroll.
This is because the
ActivationTriggerhas not been set to pointer. As a result, scrollToIndexis called while you are attempting to scroll, leading to the rubberband effect.How to Reproduce
If you are on macOS, go to System Settings and set Show Scroll Bars to Always.
It can be difficult to reproduce without this, as you must hover over the native scroll bar first avoid triggering any React events.
The bug on the Tailwind blog demo
tw-blog-scroll.mp4
Occasionally, React can render the scrolling quickly enough to make the rubberbanding non-visible, making it somewhat challenging to consistently reproduce on the blog post.
Reproducing on the Playground
Adding a 250ms artificial delay on the scrolling makes it much more obvious.
If you scroll with your mouse wheel or move your pointer inside the dropdown, the issue will resolve itself. The bug only occurs when you enter native scrollbar first and interact with it directly.
native-scroll-delay.mp4
A Fix
Adding a
onScrollhandler that ignores mobile and sets theActivationTriggerto pointer.native-scroll-fix.mp4
Feel free to discard any or all of this fix or improve it further. If nothing else, this demonstrates how to reproduce it.