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

Add ability to control scroll animation duration for Android #17422

Closed
wants to merge 15 commits into from

Conversation

@dannycochran
Copy link
Contributor

commented Jan 3, 2018

Motivation

This is one of the more sought after feature requests for RN:
https://react-native.canny.io/feature-requests/p/add-speed-attribute-to-scrollto

This PR adds the support to add a "duration" whenever using "scrollTo" or "scrollToEnd" with
a scrollView. Currently this only exists for Android as the iOS implementation will be somewhat more involved.

This PR is also backwards compatible and does not yet deprecate the "animated" boolean. It may not make sense to ever deprecate "animated", as it could be the flag that is used when devs want the system default duration (which is 250ms for Android). I'm not sure what it is for iOS. It would simplify things to remove "animated", though.

Test Plan

  1. Create a ScollView component that overflows the bounds of the container, e.g.:
  render() {
    return (
      <View style={styles.container}>
        <Button onPress={this.scrollToPosition} title="Scroll to Position" color="#841584" />
        <ScrollView style={styles.scrollView} ref={scrollView => this.scrollView = scrollView}>
          {items.map(item => {
            return <View key={item.key} style={styles.scrollViewItem}><Text>{item.text}</Text></View>
          })}
        </ScrollView>
      </View>
    );
  }
  1. Using the "scrollView" ref and the Button callback, scroll to a position with a desired duration speed, e.g:
scrollToPosition = () => {
  this.scrollView && this.scrollView.scrollTo({
    y: 500
    duration: 1000
  });
};
  1. Ensure the legacy "animated" boolean works as expected:
scrollToPosition = () => {
  this.scrollView && this.scrollView.scrollTo({
    y: 500
    animated: true
  });
};
  1. Test that you can animate the x position as well (change the ScrollView to be horizontal).
  2. Test that the scroll animation is canceled when touching on the scroll view in the middle of an animation.
  3. Test that repeated calls to animate the scrollView don't queue up (e.g. smashing the in the example above, and then tapping into the scrollView should cancel any animation movement).

Vertical scroll:
https://drive.google.com/file/d/1VhaolHx63GOv0YWTg7ay0GtzW1nIk635/view?usp=sharing

Horizontal scroll:
https://drive.google.com/file/d/1yq5P1k4-KPPu9BhS6B9liL-NRGX1Z4gA/view?usp=sharing

Related PRs

Abandoned PR to bring "duration" to iOS:
#1334

Updated docs on react-native-website:
facebook/react-native-website#115

Release Notes

[ANDROID] [ENHANCEMENT] [ScrollView] - Added support for controlling animation duration speed for methods "scrollTo" and "scrollToEnd"

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 3, 2018

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 3, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 3, 2018

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@janicduplessis

This comment has been minimized.

Copy link
Collaborator

commented Jan 3, 2018

Thanks for working on this!

Got a few questions just to make sure, does the technique you used to animate the scroll position is exactly the same as ScrollView#smoothScrollTo (same animation easing curve, etc) and have you tested cancelling animations (either tapping or starting a second animation while the first is still running)?

dannycochran added a commit to dannycochran/react-native-website that referenced this pull request Jan 3, 2018
Update scrollview.md to reflect duration argument
facebook/react-native#17422 adds the ability to control the duration of a scrollView animation, for Android only.
@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

@janicduplessis thanks for the response. I imagine we'll have to go through a few iterations here as this is my first PR with react-native :-)

  1. The duration should be the same as "smoothScrollTo" (which calls startScroll, which defaults to a 250ms duration), but I'm not sure the easing curve is. If we wanted to, I could use the existing "smoothScrollTo" for when users specify {animated: true}, but if we just look at duration things get a little simpler from a code complexity perspective.

  2. I need to test this some more. What should the desired behavior be? If a user drags the scrollView in the middle of an animation, should it cancel the existing animation? That probably is the best behavior, and if devs want to prevent an animation from being canceled they set "pointerEvents='none'".

@dannycochran dannycochran force-pushed the dannycochran:scrollDuration branch from 9219228 to 64e5e7c Jan 3, 2018

@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2018

@janicduplessis I fixed it so that repeated calls to animate the ScrollView would cancel the previous animation, as well as canceling an animation that was interrupted by a touch event.

As for the interpolation, it looks like "smoothScrollTo" used ViscousFluidInterpolator, which is not public:
https://android.googlesource.com/platform/frameworks/base.git/+/master/core/java/android/widget/Scroller.java#561

Whereas ObjectAnimator defaults to the AccelerateDecelerateInterpolator:
https://developer.android.com/reference/android/view/animation/AccelerateDecelerateInterpolator.html

We could either just go with AccelerateDecelerateInterpolator, or copy in the ViscousFluidInterpolator code. I'm fine with either approach. The existing "smoothScrollTo" animation duration is so fast it's pretty difficult to notice the difference between interpolation methods.

@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

@janicduplessis Anything else needed on my end? The failing Android test appears to be unrelated.

@alpamys-qanybet

This comment has been minimized.

Copy link

commented Jan 19, 2018

@dannycochran, Hi there,
I have implemented your code for Android, everything works ok, built from source, now I need to implement it in ios.
pull request for ios seems to be on forked project, not react-native itself.
#1334
how can I implement it for ios?

@alpamys-qanybet

This comment has been minimized.

Copy link

commented Jan 19, 2018

@dannycochran,
I implemented the code for iOS, however I do not know iOS with this commit, also, there is your code too.
Can you or somebody else who know iOS check the code?

Especially this part

     [UIView animateWithDuration:duration
          delay:0
        options:UIViewAnimationOptionBeginFromCurrentState
     animations:^(void) {
       [_scrollView setContentOffset:offset];
     } completion:^(__unused BOOL finished) {
     }];
@sahrens

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

cc @shergin for iOS code.

@sahrens

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

cc @skevy as well since he wrote the original implementation.

Daniel Cochran
@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Pinging anyone who might be available to review :-) @janicduplessis @sahrens @skevy @shergin

* Helper method for animating to a ScrollView position with a given duration,
* instead of using "smoothScrollTo", which does not expose a duration argument.
*/
public static ObjectAnimator animateScroll(final ViewGroup scrollView, int mDestX, int mDestY, int mDuration) {

This comment has been minimized.

Copy link
@sahrens

sahrens Feb 9, 2018

Contributor

I have no idea if this Animator object is legit - @janicduplessis or @mdvacca?

This comment has been minimized.

Copy link
@dannycochran

dannycochran Feb 9, 2018

Author Contributor

It has support from API 11:

https://developer.android.com/reference/android/animation/ObjectAnimator.html

FWIW we've been using this ScrollView (via overwriting the ScrollView package in MainApplication.Java) in production for the past 6 weeks, in production / release. We also use it in a fairly expensive SectionList which does all sorts of crazy optimizations and is a good stress test for performance.

This comment has been minimized.

Copy link
@sahrens

sahrens Mar 9, 2018

Contributor

I would still like signoff from someone that knows our android codebase...@janicduplessis, @mdvacca, or @axe-fb?

This comment has been minimized.

Copy link
@dannycochran

dannycochran Mar 12, 2018

Author Contributor

Sounds good.

}
this.getScrollResponder().scrollResponderScrollTo(
{x: x || 0, y: y || 0, animated: animated !== false}
{x: x || 0, y: y || 0, animated: animated !== false, duration: duration || 0}

This comment has been minimized.

Copy link
@sahrens

sahrens Feb 9, 2018

Contributor

duration || 0 seems wrong - should probably just send the null. I could imagine people explicitly setting 0 as an equivalent to animated: false.

This comment has been minimized.

Copy link
@dannycochran

dannycochran Feb 9, 2018

Author Contributor

I was following the existing pattern with this API where a default value is always provided.

In ReactScrollViewCommandHelper.java, a duration of zero is effectively ignored.

It's tricky, though, because we want to support the existing animated API so this is backwards compatible. So what happens if a user says "animated: true, duration: 0", or "animated: false, duration: 250"? In ReactScrollViewCommandHelper, I defer first to duration value. If duration is non-zero, we animate with that duration. Otherwise, if animated is true, we use the legacy animation duration (250).

This comment has been minimized.

Copy link
@sahrens

sahrens Mar 9, 2018

Contributor

Sorry, thought I replied earlier.

I'm not too concerned about cases where users give ambiguous input. If a user says "animated: true, duration: 0" there is no right answer, so we can do whatever - undefined behavior.

If the user calls scrollTo({y, duration: 0}) it's very clear that they don't want it to animate, and we should respect that clear intention and not animate it rather than making them figure out they should set animated: false instead. At the very least we could add an invariant that catches that case

This comment has been minimized.

Copy link
@dannycochran

dannycochran Mar 12, 2018

Author Contributor

Ah I see, I was missing the part where animated defaults to true in this code, and we'd probably want to preserve that, but then not respect it if a user passes duration: 0.

Daniel Cochran
@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

@sahrens any update here? I replied to your comments above. I'm happy to make some changes if you'd like but it'd be nice if we could have a more regular back and forth cadence :)

@sahrens
Copy link
Contributor

left a comment

again, let's just pass null instead of 0 by default and ignore it so that 0 is a valid input that effectively disables animation.

*
* Example:
*
* `scrollTo({x: 0, y: 0, animated: true})`
* `scrollTo({x: 0, y: 0, animated: true, duration: 0})`

This comment has been minimized.

Copy link
@sahrens

sahrens Mar 9, 2018

Contributor

This is not a good example because it's unclear what this should do.

This comment has been minimized.

Copy link
@dannycochran

dannycochran Mar 12, 2018

Author Contributor

Added a separate example just for duration.

Daniel Cochran added 3 commits Mar 12, 2018
Daniel Cochran
Daniel Cochran
@mdvacca

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

Yeah, that's actually a problem. Neither ReactScrollView or ReactHorizontalScrollView have an onTouchListener right now, that's why it is working well. But this will be a problem to maintain in the future, because if we need to set an onTouchListener in ReactScrollViewManager, it is gonna be override everytime animateScroll starts / is canceled by the user.

Could you find another way to accomplish this?

@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

@mdvacca you know it made a lot more sense to just put the "mAnimator" object inside the ScrollView itself, and have the managers call "animateScroll". That way the ScrollView can use its "onTouchEvent" to cancel the mAnimator if there's one currently in flight:

6161dd8

Tested locally and it's working on both vertical / horizontal scroll views.

Daniel Cochran
@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

@mdvacca @sahrens Anything else needed here? Would love to get this merged :-)

@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@grabbou pinging for help here on a reviewer as well. I seem to have gotten sign off on the JS and Android side of things, but still need an official approval.

@grabbou

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2018

Would be great to have a word from @janicduplessis before landing as he did some comments previously.

CC: @hramos this PR has been already approved by few developers and is looking for official (and final) approval to land it. Would you be able to help or find someone who can look into it?

@@ -558,29 +558,36 @@ const ScrollView = createReactClass({
},

/**
* Scrolls to a given x, y offset, either immediately or with a smooth animation.
* Scrolls to a given x, y offset, either immediately, with a smooth animation, or,

This comment has been minimized.

Copy link
@hramos

hramos Apr 10, 2018

Contributor

Can you open a PR that applies these same changes to https://github.com/facebook/react-native-website/blob/master/docs/scrollview.md ?

These comments are not synced. At some point we need to update these comments with links to the generated docs at http://facebook.github.io/react-native instead (I've done this with a few source files already but had not gotten to ScrollView yet).

This comment has been minimized.

Copy link
@dannycochran
@hramos

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

Looks good to me, but looks like we'd need confirmation from @janicduplessis and others.

Please reach out to them via Slack or other means.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Apr 23, 2018

@dannycochran 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.

@dannycochran

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@facebook-github-bot @janicduplessis I've rebased changes here. This change remains relevant and one of the most upvoted feature requests in Canny.

@janicduplessis
Copy link
Collaborator

left a comment

Could you make this so that it uses smoothScrollTo when using the default duration (just {animated: true})? I'd rather keep the default android behavior and this will avoid changing the scrolling behavior in subtle ways.

Looking at the source for smoothScrollTo it seems to do a lot more than just animating the scroll position. See https://android.googlesource.com/platform/frameworks/base/+/jb-release/core/java/android/widget/ScrollView.java#1103 and https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/java/android/widget/OverScroller.java. Sadly there isn't a way to customize the OverScroll instance easily (maybe possible with some crazy reflection stuff).

@janicduplessis

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2018

Also cc @mdvacca :)

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 25, 2018

@dannycochran 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.

@sahrens

This comment has been minimized.

Copy link
Contributor

commented May 26, 2018

Thanks for reviewing, @janicduplessis!

@dannycochran: can you make those changes? I agree this would be a nice improvement :)

@edlo
edlo approved these changes Aug 10, 2018
@evanjmg

This comment has been minimized.

Copy link

commented Nov 1, 2018

Any update on this? @dannycochran can you fix your conflicts?

@kabus202

This comment has been minimized.

Copy link

commented Dec 23, 2018

Any updates on this?

@paulmand3l

This comment has been minimized.

Copy link

commented Jan 2, 2019

Ping. This is so close, it would be a shame for it to get dropped now.

@osdnk

This comment has been minimized.

Copy link
Contributor

commented Jan 6, 2019

I would love to have it merged, @janicduplessis @mdvacca @dannycochran

@hramos

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

In cases like this, where a pull request has been thoroughly reviewed but still remains in a "Changes Requested" state, I'd encourage the community to "commandeer" the PR by creating their own pull request that incorporates the requested changes. If anyone does this, please ping me or Spencer and we'll take a look.

@hramos

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Thanks @osdnk for picking up the work: #22884 is a copy of this PR with resolved conflicts.

@cpojer

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2019

Closing in favor of #22884.

@cpojer cpojer closed this Jan 29, 2019

cpojer added a commit to facebook/react-native-website that referenced this pull request Feb 7, 2019
Update scrollview.md to reflect duration argument (#115)
* Update scrollview.md to reflect duration argument

facebook/react-native#17422 adds the ability to control the duration of a scrollView animation, for Android only.

* sync comments with ScrollView.js
sunnylqm added a commit to reactnativecn/react-native-website that referenced this pull request Feb 8, 2019
190208 sync 0.58 (#45)
* [android] document required Network Security Config for React Native packager (facebook#772)

* add documentation for applying Network Security Config cleartext policy exceptions for RN packager IPs

* add documentation for applying Network Security Config cleartext policy exceptions for RN packager IPs

* Update navigation.md (facebook#770)

Use createAppContainer instead of createAppNavigator 
see function declaration here https://github.com/react-navigation/react-navigation/blob/master/src/react-navigation.js#L5

* Update README.md

Fixed a few spelling errors in README.md

* Deprecating NavigatorIOS (facebook#776)

* Deprecating NavigatorIOS

* Remove navigator ios

* Remove TabBarIOS, TabBarIOS.Item, NavigatorIOS from docs

* Make Hello World prettier (facebook#760)

Fix facebook#743

* Remove outdated Stetho documentation.

* Fix Fresco docs link on the Image Style Props page (facebook#783)

* Remove CLI docs (facebook#782)

* Update scrollview.md to reflect duration argument (facebook#115)

* Update scrollview.md to reflect duration argument

facebook/react-native#17422 adds the ability to control the duration of a scrollView animation, for Android only.

* sync comments with ScrollView.js

* add documentation about `.native.js` extensions (facebook#346)

* add documentation about `.native.js` extensions

Lots of projects use the `.native.js` to share code between ReactJS and React Native (including core projects, like Jest) but there's no official documentation about it. I'm adding some info so projects can easily discover and refer to this feature.

* fix comments and add pro-tip about `.native.js`

Fixed the comment on a code block and added a pro-tip telling users to ignore `.native.js` extensions in their web bundlers in order to optimize final bundle size.

* Update platform-specific-code.md

* Link to all KeyboardType screenshots (facebook#774)

* Node version requirement made more prevalent (facebook#775)

Reading the first page of https://facebook.github.io/react-native/docs/getting-started.html, it is not clear that the node version needs to be 8+. Just adding this here to help others with installation, specially those of us who already have node running and need to upgrade.

* Explicitly state that `activeOpacity` requires `underlayColor` (facebook#781)

* Explicitly state that `activeOpacity` requires `underlayColor`

It's clear from facebook/react-native#11834 that the underlying issue won't be fixed, so we should update the documentation so people understand that for `activeOpacity` to work, they need to set `underlayColor`.

* Update touchablehighlight.md

* Add section : Known Issues with fetch and [..] (facebook#769)

* Add section : Known Issues with fetch and [..]

Cookie based authentication is currently wobbly. Had it not been for the problematic redirect scenario using fetch workarounds could be found (e.g clearing cookies before every single request and manually adding them to the headers). However, because of the 302 issue there is a scenario where using cookies can result in halt of development.

The rest of the issues are merely added here so that people that can follow workarounds ( please take a look as unintended consequences might occur )

* Minor text changes and remove the last paragraph.

* Document how to publish custom React Native version (facebook#675)

* Add section for how to make a git dependency branch (facebook#653)

* move publishing a fork to separate page (facebook#653)

* updated review comments

* link back from building android sources and updated docker link

* changed to —force instead of changing .gitignore

* Update publishing.md

* Update correct signing information for iOS (facebook#766)

Ran into this problem while following this guide verbatim.

* Update Wix link and title in showcast.json (facebook#763)

* change showcast wix logo app

* fix file name

* Update Wix link and title in showcast

* Change wording to improve sentence (facebook#759)

* Text/TextInput: Android now supports `maxFontSizeMultiplier` prop (facebook#755)

Introduced by this commit: facebook/react-native@4936d28

* Fix typo (facebook#738)

* Fix typo

* Update linking-libraries-ios.md

* update to rn-diff-purge (facebook#761)

* Prettify

* Sync guides
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.