Remove default 50ms Scroll Event Throttling in VirtualizedList#38648
Closed
NickGerleman wants to merge 1 commit into
Closed
Remove default 50ms Scroll Event Throttling in VirtualizedList#38648NickGerleman wants to merge 1 commit into
NickGerleman wants to merge 1 commit into
Conversation
facebook-github-bot
added
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47823772 |
Base commit: b0a8d45 |
NickGerleman
force-pushed
the
export-D47823772
branch
from
4a23fb4 to
c56ce67
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47823772 |
NickGerleman
force-pushed
the
export-D47823772
branch
from
c56ce67 to
7e7628d
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47823772 |
NickGerleman
force-pushed
the
export-D47823772
branch
from
7e7628d to
402e6c9
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47823772 |
1 similar comment
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47823772 |
NickGerleman
force-pushed
the
export-D47823772
branch
from
402e6c9 to
6636e55
Compare
…ook#38648) Summary: Pull Request resolved: facebook#38648 facebook#38475 made this code no longer no-op on Android, which caused regressions documented in facebook#38470 (comment) due to VirtualizedList having more out-of-date information. We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay to new renders by default when high priority work is not happening (see `updateCellsBatchingPeriod`). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events. We can eventually remove this once VirtualizedList is able to use OffScreen universally. Changelog: [General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList Reviewed By: ryancat Differential Revision: D47823772 fbshipit-source-id: dc6290ad02f9cbd40429270fc878d51cbb9b56b9
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D47823772 |
NickGerleman
force-pushed
the
export-D47823772
branch
from
6636e55 to
2508aa9
Compare
Contributor
|
This pull request has been merged in 3eccc53. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary:
This is specific to the scenario, and device, and time-based sampling as implemented on Android may inherently create stutters because we may not proess events at a consistent cadence (and we may never process the last events).
#38475 made this code no longer no-op on Android, which caused scroll regressions documented in #38470
We are already coalescing scroll events on both Android and iOS, which will ensure we are not flooded with events. VirtualizedList also already inserts an artificial 50ms delay by default when high priority work is not happening (see
updateCellsBatchingPeriod). This limits the heavy work done by VirtualizedList (no new renders or expensive math on scroll events), while letting the list still have the most recent events.We can eventually remove this once VirtualizedList is able to use OffScreen universally.
Changelog:
[General][Changed] - Remove default 50ms Scroll Event Throttling in VirtualizedList
Differential Revision: D47823772