Skip to content

Commit

Permalink
don't throttle below 16ms
Browse files Browse the repository at this point in the history
Summary: For some reason the scroll events are sometimes generated with highly irregular spacing, some coming less than a millisecond apart. For interactions that must track scrolling exactly, this can cause them to glitch. With a scroll throttle of less than 17 ms, the intention is clear that the UI should be updated in sync with the scroll view so we shouldn't drop any events.

Reviewed By: PeteTheHeat

Differential Revision: D15068841

fbshipit-source-id: 730e7cb29cc3ddae66f37cf7392e02e0cc9d7844
  • Loading branch information
sahrens authored and facebook-github-bot committed Apr 25, 2019
1 parent 0a16b53 commit c87de76
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 8 deletions.
4 changes: 3 additions & 1 deletion Libraries/Animated/src/components/AnimatedScrollView.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,6 @@ const ScrollView = require('ScrollView');

const createAnimatedComponent = require('createAnimatedComponent');

module.exports = createAnimatedComponent(ScrollView, {scrollEventThrottle: 16});
module.exports = createAnimatedComponent(ScrollView, {

This comment has been minimized.

Copy link
@janicduplessis

janicduplessis Apr 25, 2019

Contributor

I didn’t know about this default prop thing, I think we should also add it to AnimatedFlat/SectionList

scrollEventThrottle: 0.0001,
});
15 changes: 10 additions & 5 deletions Libraries/Components/ScrollView/ScrollView.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,16 @@ type IOSProps = $ReadOnly<{|
* (as a time interval in ms). A lower number yields better accuracy for code
* that is tracking the scroll position, but can lead to scroll performance
* problems due to the volume of information being send over the bridge.
* You will not notice a difference between values set between 1-16 as the
* JS run loop is synced to the screen refresh rate. If you do not need precise
* scroll position tracking, set this value higher to limit the information
* being sent across the bridge. The default value is zero, which results in
* the scroll event being sent only once each time the view is scrolled.
*
* Values between 0 and 17ms indicate 60fps updates are needed and throttling
* will be disabled.
*
* If you do not need precise scroll position tracking, set this value higher
* to limit the information being sent across the bridge.
*
* The default value is zero, which results in the scroll event being sent only
* once each time the view is scrolled.
*
* @platform ios
*/
scrollEventThrottle?: ?number,
Expand Down
7 changes: 5 additions & 2 deletions React/Views/ScrollView/RCTScrollView.m
Original file line number Diff line number Diff line change
Expand Up @@ -697,16 +697,19 @@ - (void)removeScrollListener:(NSObject<UIScrollViewDelegate> *)scrollListener

- (void)scrollViewDidScroll:(UIScrollView *)scrollView
{
[self updateClippedSubviews];
NSTimeInterval now = CACurrentMediaTime();
[self updateClippedSubviews];
/**
* TODO: this logic looks wrong, and it may be because it is. Currently, if _scrollEventThrottle
* is set to zero (the default), the "didScroll" event is only sent once per scroll, instead of repeatedly
* while scrolling as expected. However, if you "fix" that bug, ScrollView will generate repeated
* warnings, and behave strangely (ListView works fine however), so don't fix it unless you fix that too!
*
* We limit the delta to 17ms so that small throttles intended to enable 60fps updates will not
* inadvertantly filter out any scroll events.
*/
if (_allowNextScrollNoMatterWhat ||
(_scrollEventThrottle > 0 && _scrollEventThrottle < (now - _lastScrollDispatchTime))) {
(_scrollEventThrottle > 0 && _scrollEventThrottle < MAX(17, now - _lastScrollDispatchTime))) {

if (_DEPRECATED_sendUpdatedChildFrames) {
// Calculate changed frames
Expand Down

1 comment on commit c87de76

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I’ve seen an issue related to this recently, a lot of people assume scrollEventThrottle=16 means 60fps / no throttling.

Please sign in to comment.