Polyfill onScrollEnd Event in Safari #32427
Merged
Conversation
This file contains hidden or 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
github-actions
bot
added
the
React Core Team
|
Comparing: 3607f48...399aa22 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
Collaborator
Author
|
Note that at one point in the past the |
Enabled in native and test-renderer since they shouldn't affect them anyway. Makes it easier to see that it's safe to ship.
sebmarkbage
force-pushed
the
scrollend
branch
from
d255e40 to
dead6ae
Compare
Collaborator
Author
|
There's some movement in Safari. It's part of the roadmap for 2025. https://webkit.org/blog/16458/announcing-interop-2025/#scrollend-event But we still need to polyfill for now and for older versions. |
eps1lon
approved these changes
Mar 3, 2025
|
|
||
| if (listeners.length > 0) { | ||
| // Intentionally create event lazily. | ||
| const event: ReactSyntheticEvent = new SyntheticUIEvent( |
Collaborator
There was a problem hiding this comment.
Spec says it's a generic Event not a UIEvent. I also get a generic Event in Chrome.
Collaborator
Author
There was a problem hiding this comment.
Yes, but so is 'scroll' and we already treat these as SyntheticUIEvent:
github-actions bot
pushed a commit
that referenced
this pull request
Mar 3, 2025
We added support for `onScrollEnd` in #26789 but it only works in Chrome and Firefox. Safari still doesn't support `scrollend` and there's no indication that they will anytime soon so this polyfills it. While I don't particularly love our synthetic event system this tries to stay within the realm of how our other polyfills work. This implements all `onScrollEnd` events as a plugin. The basic principle is to first feature detect the `onscrollend` DOM property to see if there's native support and otherwise just use the native event. Then we listen to `scroll` events and set a timeout. If we don't get any more scroll events before the timeout we fire `onScrollEnd`. Basically debouncing it. If we're currently pressing down on touch or a mouse then we wait until it is lifted such as if you're scrolling with a finger or using the scrollbars on desktop but isn't currently moving. If we do get any native events even though we're in polyfilling mode, we use that as an indication to fire the `onScrollEnd` early. Part of the motivation is that this becomes extra useful pair for #32422. We also probably need these events to coincide with other gesture related internals so you're better off using our polyfill so they're synced. DiffTrain build for [605a880](605a880)
github-actions bot
pushed a commit
that referenced
this pull request
Mar 3, 2025
We added support for `onScrollEnd` in #26789 but it only works in Chrome and Firefox. Safari still doesn't support `scrollend` and there's no indication that they will anytime soon so this polyfills it. While I don't particularly love our synthetic event system this tries to stay within the realm of how our other polyfills work. This implements all `onScrollEnd` events as a plugin. The basic principle is to first feature detect the `onscrollend` DOM property to see if there's native support and otherwise just use the native event. Then we listen to `scroll` events and set a timeout. If we don't get any more scroll events before the timeout we fire `onScrollEnd`. Basically debouncing it. If we're currently pressing down on touch or a mouse then we wait until it is lifted such as if you're scrolling with a finger or using the scrollbars on desktop but isn't currently moving. If we do get any native events even though we're in polyfilling mode, we use that as an indication to fire the `onScrollEnd` early. Part of the motivation is that this becomes extra useful pair for #32422. We also probably need these events to coincide with other gesture related internals so you're better off using our polyfill so they're synced. DiffTrain build for [605a880](605a880)
appden
reviewed
Mar 4, 2025
Comment on lines
+147
to
+157
| case 'touchstart': { | ||
| isTouchStarted = true; | ||
| break; | ||
| } | ||
| case 'touchcancel': | ||
| case 'touchend': { | ||
| // Note we cannot use pointer events for this because they get | ||
| // cancelled when native scrolling takes control. | ||
| isTouchStarted = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Sorry for the post-merge drive-by, but not accounting for multi-touch is an old pet peeve of mine. 😜
Suggested change
| case 'touchstart': { | |
| isTouchStarted = true; | |
| break; | |
| } | |
| case 'touchcancel': | |
| case 'touchend': { | |
| // Note we cannot use pointer events for this because they get | |
| // cancelled when native scrolling takes control. | |
| isTouchStarted = false; | |
| break; | |
| } | |
| case 'touchstart': | |
| case 'touchcancel': | |
| case 'touchend': { | |
| // Note we cannot use pointer events for this because they get | |
| // cancelled when native scrolling takes control. | |
| isTouchStarted = ((nativeEvent: any): TouchEvent).touches.length > 0; | |
| break; | |
| } |
Comment on lines
+158
to
+165
| case 'mousedown': { | ||
| isMouseDown = true; | ||
| break; | ||
| } | ||
| case 'mouseup': { | ||
| isMouseDown = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Also shouldn't assume all mouse events are for left clicks, and mouseup won't fire after a drag starts.
Suggested change
| case 'mousedown': { | |
| isMouseDown = true; | |
| break; | |
| } | |
| case 'mouseup': { | |
| isMouseDown = false; | |
| break; | |
| } | |
| case 'mousedown': { | |
| if (((nativeEvent: any): MouseEvent).button === 0) { | |
| isMouseDown = true; | |
| } | |
| break; | |
| } | |
| case 'mouseup': | |
| case 'dragend': { | |
| if (((nativeEvent: any): MouseEvent).button === 0) { | |
| isMouseDown = false; | |
| } | |
| break; | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
We added support for
onScrollEndin #26789 but it only works in Chrome and Firefox. Safari still doesn't supportscrollendand there's no indication that they will anytime soon so this polyfills it.While I don't particularly love our synthetic event system this tries to stay within the realm of how our other polyfills work. This implements all
onScrollEndevents as a plugin.The basic principle is to first feature detect the
onscrollendDOM property to see if there's native support and otherwise just use the native event.Then we listen to
scrollevents and set a timeout. If we don't get any more scroll events before the timeout we fireonScrollEnd. Basically debouncing it. If we're currently pressing down on touch or a mouse then we wait until it is lifted such as if you're scrolling with a finger or using the scrollbars on desktop but isn't currently moving.If we do get any native events even though we're in polyfilling mode, we use that as an indication to fire the
onScrollEndearly.Part of the motivation is that this becomes extra useful pair for #32422. We also probably need these events to coincide with other gesture related internals so you're better off using our polyfill so they're synced.