Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mActivelyScrolling expected to be true so long as events are being fired. #15146

Closed
wants to merge 1 commit into from

Conversation

tomasreimers
Copy link
Contributor

@tomasreimers tomasreimers commented Jul 22, 2017

Problem:

It was observed that in this code path (i.e. horizontal, paging-enabled scroll view) if you tried to programmatically call the scrollTo method within ~1s of the onMomentumScrollEnd event (which should only be called after all scrolling has ended), the scrollView would scroll to the new location, and then scroll BACK to the original location.

For example, assume you had released the scrollView at location B, and the nearest page boundary is A. Then, 1000ms later, you call scrollTo position C. The order of operations would be:

  1. Begin scrolling to A from position B (as it is the nearest page boundary)
  2. Reach position A
  3. Fire onMomentumScrollEnd
  4. 1000ms later call scrollTo C
  5. scrollView scrolls to C
  6. scrollView scrolls BACK to position A (for no apparent reason).

Reason:

I suspect this is because the smoothlyScrollTo will continue to animate towards A, but the scrollEvents will not fire as they are too close to each other. So the true order of events is:

  1. Begin scrolling to A from position B (as it is the nearest page boundary)

[begin smoothlyScrollTo]
[scroll towards position A]
[mActivelyScrolling is true]

  1. Reach position A

[mActivelyScrolling is true]
[scroll towards position A]
[mActivelyScrolling is false, as there is another scrollEvent, but because it is close enough to the same location it is ignored]

  1. Fire onMomentumScrollEnd
  2. 1000ms later call scrollTo C

[scroll towards position C]

  1. scrollView scrolls to C

[scroll towards position A as the original smoothlyScrollTo animation was never completed]

  1. scrollView scrolls BACK to position A.

This is an untested hypothesis, but seems to explain the behavior, and the solution is more semantically correct anyway. If there is an easy way to rebuild the android binaries happy to test it myself! Just let me know!

Solution:

Move the mActivelyAnimating outside the mOnScrollDispatchHelper.onScrollChanged helper, because the HorizontalScrollView event should be considered to be animating so long as onScrollChanged events are being fired.

@facebook-github-bot 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 Jul 22, 2017
@pull-bot
Copy link

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

Generated by 🚫 dangerJS

@tomasreimers
Copy link
Contributor Author

Error visible here: https://snack.expo.io/SJyS7GuwZ

You can see how on iOS you get the expected behavior (once the scroll is over it jumps to the center, 320):

screen shot 2017-08-09 at 12 44 57 am

While on Android it jumps to the center, and then resumes the previous lerp:

screen shot 2017-08-09 at 12 46 33 am

@shergin shergin self-requested a review August 15, 2017 21:02
@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 8, 2017
@facebook-github-bot
Copy link
Contributor

@tomasreimers has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants