iOS ScrollView: Add scrollBy method by rigdern · Pull Request #15609 · facebook/react-native

rigdern · 2017-08-22T22:04:06Z

Android PR: #15610

General Motivation

Currently, React Native allows you to scroll a ScrollView to a particular position using scrollTo. If instead you want to scroll by a delta, you could try to do this using scrollTo (add the delta to the current scroll position) but this doesn't always work out as intended due to the async nature of React Native (what you think is the current scroll position might be a stale value). This change introduces a scrollBy API to solve this problem. scrollBy takes a delta to scroll by and native takes care of doing the arithmetic because it knows the latest scroll position.

Example

We have a vertical virtualized list view where all of the items are absolutely positioned. The item positions are controlled by AnimatedValues because we want to avoid having to go through the diffing process to move them.

A scenario where scrollBy is useful is when we load items above the user's viewport which causes the y position of items in the viewport to change. If we didn't do anything additional, this would cause the items the user is looking at to fly off of the screen. To avoid this, we update the scroll position (by the amount the y coordinate of the viewport items changed) and update the y coordinates at the same time.

We initially tried to do this with scrollTo by adding the current scroll position to the y coordinate delta. However, sometimes the current scroll position value was stale by the time scrollTo ran which caused us to scroll to the wrong position and for the user to perceive a jumpiness. Consequently, we've introduced a native scrollBy implementation so we don't have to worry about using a stale value for current scroll position.

Test Plan

In a test app, verified that scrollBy works properly with and without animation and for both horizontal and vertical ScrollViews. Also, my team is using this change in our app.

Adam Comella
Microsoft Corp.

Currently, React Native allows you to scroll a ScrollView to a particular position using `scrollTo`. If instead you want to scroll by a delta, you could try to do this using `scrollTo` (add the delta to the current scroll position) but this doesn't always work out as intended due to the async nature of React Native (what you think is the current scroll position might be a stale value). This change introduces a `scrollBy` API to solve this problem. `scrollBy` takes a delta to scroll by and native takes care of doing the arithmetic because it knows the latest scroll position.

pull-bot · 2017-08-22T22:12:20Z

Attention: @shergin, @javache

Generated by 🚫 dangerJS

eslint-bot · 2017-08-22T22:13:34Z

  },

+  /**
+   * A helper function to scroll by a specific offset in the ScrollView.


no-trailing-spaces: Trailing spaces not allowed.

eslint-bot · 2017-08-22T22:13:34Z

+   *
+   * `scrollBy(options: {deltaX: number = 0; deltaY: number = 0; animated: boolean = true})`
+   */
+  scrollBy: function(options: { deltaX?: number, deltaY?: number, animated?: boolean } ) {


no-trailing-spaces: Trailing spaces not allowed.

The change enables developers to ignore scroll events that occur between the time they set the scroll position in JavaScript and the time that the native code applies the scroll position to the view. This is important for apps that care about carefully tracking and managing the ScrollView's scroll position. For example, when a virtualized ScrollView sets the scroll position to X, it wants to begin preparing to render the content as though it's scrolled to X. Currently, if it receives a scroll event for Y, it doesn't know whether the ScrollView reached Y before or after X. Consequently, it doesn't know whether it should start rendering content for Y because Y is the latest scroll position or if it should ignore Y and continue rendering content for X. The way this is implemented is each method for changing the scroll position (e.g. scrollTo, scrollToEnd) returns an id. When the native code sets the scroll position on the view, it fires an `onScrollPositionSet` which contains the id of the related scroll request. Apps can solve the problem described above by ignoring all scroll events that occur between the time that the app requests the scroll position change and the time the corresponding `onScrollPositionSet` event fires. **Test Plan** Built a test app that uses the above technique to ignore scroll events that occur between the scroll request and the firing of the corresponding `onScrollPositionSet` event. Logged whether each `onScroll` event should be ignored and verified that the log matched my expectations. Tested this with `scrollTo`, `scrollToEnd`, and `scrollBy`. **Dependency Notes** I built this change on top of my other PR (facebook#15609). If I didn't do this, I'd have to resolve merge conflicts after facebook#15609 was merged. Adam Comella Microsoft Corp.

shergin · 2017-08-23T05:03:49Z

@rigdern Thank you for the is great PRs!

Honestly, I am a bit conserned about should we have this or not; I see a great advantage in having the smallest possible set of APIs.

Because of asynchronous nature of RN, this method is already asynchronous. So, if we simply implement reliable getter for contentOffset value (which we also need, right?), we will be able easily implement scrollBy in pure JavaScript.

rigdern · 2017-08-23T06:04:20Z

@shergin As you point out, you can implement scrollBy in terms of scrollTo as long as you know the current scroll position and the delta you want to scroll by. If you have the current scroll position in JavaScript, you could implement scrollBy in terms of scrollTo. However, because native and JavaScript communicate asynchronously, doing this calculation in JavaScript always runs the risk of it being done with a stale scroll position. This is why we moved the calculation to native.

Before implementing scrollBy, my team tried doing the calculation in JavaScript using the latest position given by onScroll. However, we ran into some scrolling issues that were resolved by introducing this scrollBy API.

shergin · 2017-08-25T00:42:49Z

@rigdern Yes, implementing this natively makes this faster but it does not makes it synchronous. It helps a bit and probably fixes your case, but conceptually it is pretty same as my proposed alternative solution.
UIBlock will be executed on UIManager "tick" anyway (~60hz). Native contentOffset implementation though should be much more perfromant (you can use simple distach_async to retrieve value from main thread), so you will be able get result before the next tick. So, I bet my proposed solution will be as fast as this one. (But it is much simpler and we end up with two method instead on one.)

rigdern · 2017-08-25T00:54:17Z

@shergin I'm not sure I understand your proposal. Can you elaborate?

shergin · 2017-08-25T02:13:48Z

@rigdern
Sure!
Step 1. Implementing a prop/method currentContentOffset which return promise with offset value.
Step 2. Implementing scrollBy as a combination of currentContentOffset and scrollTo.

rigdern · 2017-08-25T04:19:28Z

@shergin let's say you want to scroll by 50 pixels. What happens if by the time your scrollTo is processed, the scroll position has changed such that your currentContentOffset value is off by 10 pixels? Then your scrollTo request will only move you by 40 pixels instead of the 50 that you wanted.

Isn't this a problem that your proposal has but my proposal does not?

shergin · 2017-08-25T04:53:15Z

@rigdern I believe, your proposal has (almost) same issue, unfortunately. Even if the shift itself will be performed at exact amount of pixels, because of async nature of RN, your app will still have no idea what's going on with scrollview. So, your contr-case looks pretty limited to me.
Beside that, afaik, getting this value should be very very fast, comparable to generic dispatch_async, so you will get wrong/obsolete result almost only if some animation is performing.

rigdern · 2017-08-25T07:25:07Z

@shergin regarding the app not knowing what the ScrollView is doing, there's another PR I'll be sharing which causes the ScrollView to fire an event after it applies the scrollTo/scrollBy operation. The combination of the event and the scrollBy method have given us a good experience for manipulating the ScrollView.

Does this address your concern about this proposal having almost the same problem as yours?

shergin · 2017-08-25T16:04:57Z

@rigdern Sure, firing onScroll event right after scrollTo is a good idea! But I don't think that it can help here.

rigdern · 2017-08-28T22:55:39Z

@shergin To be clear, we're not reusing the onScroll event. We have a a separate event called onScrollPositionSet which native fires after applying the scrollTo/scrollBy request. An app can keep track of what the ScrollView is doing by ignoring all onScroll events that occur after JavaScript has called scrollTo/scrollBy but before it receives the corresponding onScrollPositionSet event.

Can you elaborate on your concern with this solution?

shergin · 2017-09-11T18:36:30Z

@sahrens Spencer, we need you second opinion (or blessing) here!
So, the main question: Should we have scrollBy implemented natively or should we have just currectContentOffset implemented natively?

sahrens · 2017-09-11T19:02:21Z

Let me guess - are you using this for an inverted scroll list to do an animated reveal of new items at the bottom? If so, have you tried layout animation and do things work well when the scroll position is in the middle of the content somewhere?

If not, giving us more context on your scenario would help.

I agree with @shergin in general that minimizing the API is generally good, but I can see some cases where scrollBy might be really helpful.

rigdern · 2017-09-11T19:46:24Z

@sahrens

We have a vertical virtualized list view where all of the items are absolutely positioned. The item positions are controlled by AnimatedValues because we want to avoid having to go through the diffing process to move them.

A scenario where scrollBy is useful is when we load items above the user's viewport which causes the y position of items in the viewport to change. If we didn't do anything additional, this would cause the items the user is looking at to fly off of the screen. To avoid this, we update the scroll position (by the amount the y coordinate of the viewport items changed) and update the y coordinates at the same time.

We initially tried to do this with scrollTo by adding the current scroll position to the y coordinate delta. However, sometimes the current scroll position value was stale by the time scrollTo ran which caused us to scroll to the wrong position and for the user to perceive a jumpiness. Consequently, we've introduced a native scrollBy implementation so we don't have to worry about using a stale value for current scroll position.

I added this example to the PR description.

sahrens · 2017-09-11T20:01:49Z

Ah yes, I have proposed a different solution to this problem - I would posit that the current behavior (content jumping when offscreen layout changes) is rarely what should happen and we should change the bahevior of ScrollView to prevent this, so you can append to the top or bottom with no movement of content in the view port. We'll probably need to hate the behavior somehow so we don't break legacy cases, but I think we should change the behavior long term.

@shergin: I think you might have a task for this. Thoughts?

rigdern · 2017-09-11T20:11:25Z

@sahrens Can you elaborate on the details of how this ScrollView change will work? Can you clarify what you meant by "We'll probably need to hate the behavior...". I think you meant to use a different word than "hate".

To be clear, we are using a custom virtualized list view implementation. We aren't using React Native's ListView or VirtualizedList.

shergin · 2017-09-11T20:27:08Z

Speaking about current scrollBy proposal, here is what I dislike with this in short:

Complex (and eventually unnecessary) native implementation;
Having concept: "I am business logic; I have no idea of current/actual ScrollView state; but, anyways, please scroll it to 50 pixels down.".

So, I believe we can solve both problems having trivial currectContentOffset implementation and trivial scrollBy implementation (in JS!). We have not tried it yet. 😞 I feel I have to implement it by myself.

Speaking about "content-offset aware relayout": Yes, we discussed it (just created t21772624), and that is great idea, but I am not sure that I know how to implement it. Yet. And looks like we have to have a way to specify exact node which have to define "stable position".

sahrens · 2017-09-11T20:32:23Z

Ha yeah meant "gate" the behavior.

Basically, by default, if you add items off the top of the screen, then we should automatically do the compensation you're trying to do manually.

sahrens · 2017-09-11T20:33:41Z

But at the very least we could make it a new prop, like maintainVisibleContentPosition or something.

rigdern · 2017-09-11T21:29:15Z

@sahrens supporting a maintainVisibleContentPosition prop in the ScrollView might make the scrollBy method unnecessary because that's the kind of behavior we're trying to achieve through scrollBy.

However, I suspect implementing this logic in ScrollView will be harder than us implementing it in our virtualized list view because our virtualized list view knows a lot more about its content. It knows the content consists of a sequence of items, it knows the size and position of each item, and it knows when the content is changing. Whereas the ScrollView makes fewer assumptions about its content which makes it a more general problem which is harder to solve. Also, I imagine the maintainVisibleContentPosition implementation would need to query for the position of the "locked" element so we'd have to make sure that wouldn't hurt performance.

@shergin you mentioned these criticisms of scrollBy:

Complex (and eventually unnecessary) native implementation;

Having concept: "I am business logic; I have no idea of current/actual ScrollView state; but, anyways, please scroll it to 50 pixels down.".

I agree with the desire to keep the surface area small by only adding APIs that are necessary.

I'm not sure why you characterize the native implementation as "complex". It looks like a small amount of straightforward code.

You mentioned "I am business logic; I have no idea of current/actual ScrollView state;" I wouldn't say it "has no idea". It knows what the ScrollView is doing but its knowledge of the details might be slightly out-of-date. Consequently, if it were to use its slightly out-of-date information in the calculation, the result would be slightly off which would be noticeable to the user.

I feel I have to implement it by myself.

Feel free to do that. We experimented with various solutions on Android and ended up with a native scrollBy in order to avoid the problems mentioned earlier in this thread. You can see if your experiments yield the same conclusion.

What are the next steps for working towards a conclusion on this PR?

sahrens · 2017-09-12T04:52:40Z

I think scrollBy is pretty simple so pretty low risk and low cost in terms of API bloat, so if we're not sure how long it would take to get a proper solution in place, I think it would be ok to do this in the mean time. Maybe we shouldn't document it to discourage usage though?

Are you ok with moving forward on this, @shergin?

sahrens · 2017-09-12T04:58:20Z

Also, are you really not seeing any flicker with this? Seems like you would run the risk of flickering depending on if the layout takes more than a frame to compute or not. Or are you doing something else to try and synchronize, like waiting for onLayout or something?

rigdern · 2017-09-13T00:04:54Z

@sahrens

Also, are you really not seeing any flicker with this?

I asked my team about this and you are correct. It turns out we ended up seeing flickering on Android. To fix this, we wrote a custom native module which repositions the items and updates the scroll position in the same frame. (It's not that general of a solution. The method takes a list of views, a list of coordinates to move the views to, and a delta to scroll by and then applies all of that information in the same frame.) So we're no longer using this scrollBy patch on Android.

On iOS, we're still using this scrollBy patch and we're not seeing any flickering.

sahrens · 2017-09-13T15:01:37Z

Oh another issue - does scrollBy (or your custom native module on Android) maintain scroll momentum nicely?

Here's another approach - what about rendering an "infinite" container on top of your content with a big contentInset to compensate for it, then render new items into that container stacking up from the bottom (flex-end) and reduce the contentInset at the same time so they can be scrolled to? Then you don't have to muck with scroll position at all. If that approach works, maybe we can bake it into FlatList...

rigdern · 2017-09-13T21:20:04Z

@sahrens

Regarding scroll momentum, we designed our virtualized list view to avoid interfering with scroll momentum. It does this by waiting for the scroll position to stop changing (so the ScrollView is probably idle) before setting the scroll position.

I know that contentInset is an API on iOS. Is something similar supported on Android?

By the way, I think it's unlikely that my team will be able to switch to a generalized virtualized list. We wrote 2 custom virtualized lists. The first one was intended to handle all virtualized list scenarios in our app (we open sourced it: https://microsoft.github.io/reactxp/docs/extensions/virtuallistview.html). However, one of the virtualized list scenarios in our app was more complicated than the others so we ended up writing a second customized virtualized list just for that scenario. This second more complicated one is the one that uses scrollBy and the one we've been talking about here.

shergin · 2017-09-14T00:53:19Z

... Flickering on Android supports my theory (where the app does not really know what's going on and ask ScrollView to scroll by some amount of pixels). So, we are lucky, this solution works on iOS for this very specific case, but it does not work on Android because... we are not lucky on Android.

This make me feel uncomfortable with this approach.

sahrens · 2017-09-14T01:06:03Z

If it waits for scrolling to stop to adjust scroll position, it must also wait to insert the new item, right? Otherwise it would jump. In that case, what happens if you scroll continuously until the ScrollView stops at the edge - would the new content pop in at that point?

Looks like your VirtualListView is very similar to VirtualizedList. That's cool you keep the DOM order constant and change the offsets. I thought about doing that, but it made it hard to leverage the power of native layout and LayoutAnimation for cell height changes and animated drag-and-drop.

What does recycling actually do? Looks like all it does is recycle the info objects which are very lightweight? There is quite a bit of code though so I probably missed something. I found recycling actually had little benefit and in some cases was actually worse because the react diffing of totally different items was more expensive than just rendering new, and the bottleneck is the js, not the native view creation. The JS VM GC is also very good at short lifecycle object management, unlike say dalvik, so I saw little benefit from recycling js objects.

In any case, have you tried a side-by-side perf test with VirtualizedList? I'm curious how they compare.

rigdern · 2017-09-14T01:47:31Z

@shergin

(where the app does not really know what's going on and ask ScrollView to scroll by some amount of pixels)

I'm not sure what you mean by this. If you mean that the flickering happens because the scrollBy and the repositioning of items happen in different frames, then yes that's right. I'm not sure why they happen in the same frame on iOS but get split across frames and cause flickering on Android. As you say, we are lucky on iOS.

facebook-github-bot · 2017-10-16T17:06:00Z

@rigdern I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

stale · 2017-12-15T17:40:23Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

rigdern · 2017-12-15T19:17:56Z

Thanks for the discussion. We decided to go with an alternative solution. We built a native module which combines a number of operations our virtual list view implementation requires (e.g. scrolling & repositioning items) into a single method so all of the operations are completed in the same frame.

paramaggarwal · 2018-04-11T18:03:55Z

maintainVisibleContentPosition is now in RN 0.54 for iOS.

rigdern requested review from javache and shergin as code owners August 22, 2017 22:04

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 22, 2017

eslint-bot reviewed Aug 22, 2017

View reviewed changes

Remove trailing whitespace

b27beda

rigdern mentioned this pull request Aug 22, 2017

Android ScrollView: Add scrollBy method #15610

Closed

shergin added the Platform: iOS iOS applications. label Aug 23, 2017

edencakir mentioned this pull request Sep 17, 2017

New Registration Screen hummingbird-me/kitsu-mobile#53

Merged

birkir mentioned this pull request Nov 17, 2017

Support iOS 11 prefersLargeTitles #1643 wix/react-native-navigation#2090

Merged

stale Bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 15, 2017

rigdern closed this Dec 15, 2017

Conversation

rigdern commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-bot commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eslint-bot Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

eslint-bot Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

shergin commented Aug 23, 2017

Uh oh!

rigdern commented Aug 23, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

rigdern commented Aug 25, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

rigdern commented Aug 25, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

rigdern commented Aug 25, 2017

Uh oh!

shergin commented Aug 25, 2017

Uh oh!

rigdern commented Aug 28, 2017

Uh oh!

shergin commented Sep 11, 2017

Uh oh!

sahrens commented Sep 11, 2017

Uh oh!

rigdern commented Sep 11, 2017

Uh oh!

sahrens commented Sep 11, 2017

Uh oh!

rigdern commented Sep 11, 2017

Uh oh!

shergin commented Sep 11, 2017

Uh oh!

sahrens commented Sep 11, 2017

Uh oh!

sahrens commented Sep 11, 2017

Uh oh!

rigdern commented Sep 11, 2017

Uh oh!

sahrens commented Sep 12, 2017

Uh oh!

sahrens commented Sep 12, 2017

Uh oh!

rigdern commented Sep 13, 2017

Uh oh!

sahrens commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rigdern commented Sep 13, 2017

Uh oh!

shergin commented Sep 14, 2017

Uh oh!

sahrens commented Sep 14, 2017

Uh oh!

rigdern commented Sep 14, 2017

Uh oh!

facebook-github-bot commented Oct 16, 2017

Uh oh!

stale Bot commented Dec 15, 2017

Uh oh!

rigdern commented Dec 15, 2017

Uh oh!

paramaggarwal commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

rigdern commented Aug 22, 2017 •

edited

Loading

pull-bot commented Aug 22, 2017 •

edited

Loading

sahrens commented Sep 13, 2017 •

edited

Loading

paramaggarwal commented Apr 11, 2018 •

edited

Loading