Implement sticky headers in JS using Native Animated #11315

janicduplessis · 2016-12-06T03:49:54Z

This re-implements sticky headers in JS to make it work on Android.

The only change that was needed was to expose a way to attach a an animated value to an event manually since we can't use the Animated wrapper and Animated.event to do it for us because this is implemented directly in the ScrollView component. Simply exposed attachNativeEvent that takes a ref, event name and event object mapping. This is what is used by Animated.event.

TODO:

Need to check why momentum scrolling isn't triggering scroll events properly on Android.
Remove native iOS implementation
cleanup / fix flow

Test plan
Test the example list in UIExplorer, test the ListViewPaging example.

facebook-github-bot · 2016-12-06T03:50:08Z

By analyzing the blame information on this pull request, we identified @sahrens and @emilsjolander to be potential reviewers.

janicduplessis · 2016-12-06T04:58:33Z

Libraries/Animated/src/Animated.js

 };
+
+Object.assign((Animated: Object), AnimatedImplementation);


Is there a way to express this properly in flow? I want to add props to an object using Object.assign. Can't create a new object here because it breaks the getter.

Why not just include AnimatedImplementation in the initial Object creation?

@sahrens Using { ...AnimatedImplementation, ... } will compile to Object.assign({}, AnimatedImplementation, ...) but there seems to be an issue with getters on iOS, the getter doesn't get assigned but it's value is instead, losing the getter altogether.

This is why I used Object.assign(Animated, AnimatedImplementation) instead to avoid having to copy Animated props and getters get preserved properly. However doing that causes a bunch of flow issues because we are assigning props that don't exist on the initial object.

I meant as a field, like Animated.AnimatedImplementation.createAnimatedComponent. Not a big deal.

nihgwu · 2016-12-06T05:05:23Z

It's interesting you are into the JS way to implement stickyHeader, why do you want to remove the native iOS implementation since it works well now, and why do you give up the former native Android implementation, I've been waiting this feature for so long

janicduplessis · 2016-12-06T05:09:53Z

There is no point in keeping the iOS implementation since that will replace it. I tested that it behaves the exact same way so you should not see any difference on iOS.

The Android implementation required a lot of changes because the view hierachy in react is different from the one in native because of the view hierarchy optimization that we do on Android so working with view indices doesn't work well at all. It would have been possible but after thinking about it more and now that native animations are a lot more stable this makes more sense.

janicduplessis · 2016-12-06T05:24:28Z

Also it uses native animations so it won't really be slower than the native implementation if that was your concern.

nihgwu · 2016-12-06T05:31:05Z

I know that and great thanks for your works on this 👍 , and I have a new issue for your 😄

brentvatne · 2016-12-07T22:03:51Z

@nihgwu - if it is very important to you, you're welcome to contribute too ;) no need to wait for other people to do things for you

ide · 2016-12-07T22:54:18Z

++ Also another way to contribute is to sponsor development of the feature.

nihgwu · 2016-12-08T00:14:29Z

I'd love to contribute but I know nothing about native development though ? So for myself the best way to contribute is to use and to test it even it hasn't be shipped into the master branch I'm switching all the animations to be driven in my apps now, although there are still some issues but I'm trying my best to work them out BTW, I did have a pure js version workaround for sticky header on Android on my own way before, but I thought it was not good enough so I dropped it out ? 2016?12?8??06:04?Brent Vatne <notifications@github.com<mailto:notifications@github.com>> ??? @nihgwu<https://github.com/nihgwu> - if it is very important to you, you're welcome to contribute too ;) no need to wait for other people to do things for you - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#11315 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACeY8k0aeGkvAyV7M3EET3j6M39kaZDsks5rFy1YgaJpZM4LE_sl>.

nihgwu · 2017-01-06T02:14:12Z

any progress on this?

jeanregisser · 2017-01-10T16:49:09Z

Nice work @janicduplessis! 👍

I've implemented sticky headers with a similar technique as the one you used here (but not as generic as what you did) and noticed the issue with momentum scrolling on Android. No issue on iOS though.
If I leave my finger on the scrollview and move it around, everything is smooth.
But once I lift my finger and momentum kicks in, the headers positions are late / incorrect until the movement stops completely.
i.e. it shows an ugly jump of the headers.

Any hint on where to start tracking this issue? I'm very confortable with iOS development, but quite a beginner on Android.

janicduplessis · 2017-01-10T20:13:25Z

Yea that is mainly what is blocking this PR at the moment, I will try to have a look at this this week.

nihgwu · 2017-01-11T02:44:53Z

some bugs on iOS:

sticky header will bounce when there are layout animations in rows
when you over pull over the scrollview, the sticky header will stay sticky instead of move with the scrollview

on Android:

current implementation utilizes zIndex witch hasn't been supported by Nodes introduced in RN0.41

jeanregisser · 2017-01-11T09:41:30Z

Interesting @nihgwu.
I've seen the jump of the sticky headers when using LayoutAnimation only on Android sofar.

janicduplessis · 2017-01-11T10:10:27Z

Yea I think the bug with layout animation is on Android only and has always been there, more or less related to this PR.

nihgwu · 2017-01-11T12:10:43Z

sorry, my mistake, layout animation issue exists on Android only
I'm using Nodes now, so I'm willing to see a implementation to support it, that is not using the zIndex but nature hierarchy, would it be possible?

nihgwu · 2017-01-11T12:12:30Z

I've to say that I've tried to solve the momentum issue, but seems sendMomentumEvents can't be set to false and I have no idea.

janicduplessis · 2017-01-11T22:07:45Z

Node is still experimental and will eventually support z-index so I'm not too worried at the moment.

sahrens · 2017-01-17T22:25:46Z

@ide: are you or anyone else reviewing this?

janicduplessis · 2017-01-17T22:59:08Z

@sahrens I don't think anyone is reviewing this at the moment, right now iOS works really well but android has a few bugs with events during momentum scrolling and view clipping. The JS code shouldn't change so feel free to review if you want :)

sahrens

This is looking sweet :)

How are those known bugs going? Are you planning on fixing them in this diff or followups? How much have you tested this?

sahrens · 2017-01-17T22:30:28Z

Libraries/Animated/src/Animated.js

 };
+
+Object.assign((Animated: Object), AnimatedImplementation);


Why not just include AnimatedImplementation in the initial Object creation?

sahrens · 2017-01-17T22:31:33Z

Libraries/Animated/src/Animated.js

  View: AnimatedImplementation.createAnimatedComponent(View),
  Text: AnimatedImplementation.createAnimatedComponent(Text),
  Image: AnimatedImplementation.createAnimatedComponent(Image),
-  ScrollView: AnimatedImplementation.createAnimatedComponent(ScrollView),
+  get ScrollView() {


Why do you even need to create and export this?

Native animated events do need a wrapper for ScrollView / ListView this is the reason it is exported here.

When is this getter accessed though? I don't see any code in this diff that would call it.

This isn't used in this diff but we export it as part of the public api like Animated.View. Had to make this change just because this diff creates a circular reference since ScrollView now imports animated.

sahrens · 2017-01-17T22:35:04Z

Libraries/Animated/src/AnimatedImplementation.js

@@ -2152,6 +2196,7 @@ class AnimatedEvent {
  ) {
    this._argMapping = argMapping;
    this._listener = config.listener;
+    this._attachment = null;


_attachment is a non-intuitive name for me - something like _attachedEvent or _nativeEvent might be better?

sahrens · 2017-01-17T22:41:34Z

Libraries/Components/ScrollView/ScrollView.js

+              ref={(ref) => this._setStickyHeaderRef(index, ref)}
+              onLayout={(event) => this._onStickyHeaderLayout(index, event)}
+              scrollAnimatedValue={this._scrollAnimatedValue}
+            >


nit: should stay on prev line

sahrens · 2017-01-17T22:43:01Z

Libraries/Components/ScrollView/ScrollView.js

+  },
+
+  _onStickyHeaderLayout: function(index, event) {
+    const previousHeaderIndice = this.props.stickyHeaderIndices[this.props.stickyHeaderIndices.indexOf(index) - 1];


s/Indice/Index

sahrens · 2017-01-18T02:03:33Z

Libraries/Components/ScrollView/ScrollViewStickyHeader.js

+        inputRange.push(layoutY + 1);
+        outputRange.push(1);
+      }
+      translateY = this.props.scrollAnimatedValue.interpolate({


All this interpolate range computation is pretty complicated and non-intuitive - can you add some comments, maybe a comment block, explaining the logic?

Added a comment, let me know if that makes sense.

Very helpful.

sahrens · 2017-01-18T02:05:07Z

Libraries/Components/ScrollView/ScrollViewStickyHeader.js

+
+const styles = StyleSheet.create({
+  header: {
+    zIndex: 10,


Anything less arbitrary/more robust you can do here?

I don't really have other ideas except maybe exporting this as a constant somewhere.

sahrens · 2017-01-18T02:06:19Z

Libraries/Components/ScrollView/ScrollView.js

+    const hasStickyHeaders = this.props.stickyHeaderIndices && this.props.stickyHeaderIndices.length > 0;
+
+    const children = hasStickyHeaders ?
+      React.Children.toArray(this.props.children).map((child, index) => {


Why not React.Children.map?

https://facebook.github.io/react/docs/react-api.html#react.children.map

I did try using React.Children.map but it doesn't work, looks like indices are different. My guess is that toArray handles null / nested children differently and that's the behavior we want here.

janicduplessis · 2017-01-19T23:13:54Z

Ok, I figured out the lag during momentum scrolling on android, fix in #11994

eslint-bot · 2017-01-20T00:13:41Z

Libraries/Components/ScrollView/ScrollView.js

@@ -11,13 +11,15 @@
 */
 'use strict';

+const Animated = require('Animated');
 const ColorPropType = require('ColorPropType');
 const EdgeInsetsPropType = require('EdgeInsetsPropType');
 const Platform = require('Platform');


access of computed property/element Computed property/element cannot be accessed on possibly undefined value undefined

eslint-bot · 2017-01-20T00:13:41Z

Libraries/Components/ScrollView/ScrollView.js

@@ -11,13 +11,15 @@
 */
 'use strict';

+const Animated = require('Animated');
 const ColorPropType = require('ColorPropType');
 const EdgeInsetsPropType = require('EdgeInsetsPropType');
 const Platform = require('Platform');


call of method indexOf Method cannot be called on possibly undefined value undefined

janicduplessis · 2017-02-13T21:13:33Z

@sahrens I noticed you talked about FlatList not supporting sections and creating a separate component that support this. I was thinking we could just hold off on this PR and keep the native iOS only impl for ListView and move to something like this for the new FlatList that support sections.

sahrens · 2017-02-13T21:42:36Z

I believe FlatList is orthogonal - it's still just going to set stickyHeaderIndices on the ScrollView, and it can work with the current impl or the Animated-based one here (I just wanted to make sure it works with this approach since this is probably the future). The main value of these diffs is to add Android support for sticky headers, which isn't that important to us internally.

@astreet/ @mkonicek: any progess on those test failures?

Summary: Native animated events sometimes end up lagging a frame behind on android because we perform the update in the normal animation loop instead of doing it immediately when we receive the event. We had the same issue on iOS and was fixed in a similar way. Moved some code around to have a method that updates a list of node that we can use to update the node in the animated event handler and also use it in the animation update loop. **Test plan** Tested that it did fix sticky headers lagging a frame behind during momentum scrolling in my PR #11315 and also tested the native animations examples still work properly. Closes #11994 Reviewed By: mkonicek Differential Revision: D4488977 Pulled By: sahrens fbshipit-source-id: 831a1565bc7b8fa88cadd5a8c1be876fbdefbf66

Summary: Native animated events sometimes end up lagging a frame behind on android because we perform the update in the normal animation loop instead of doing it immediately when we receive the event. We had the same issue on iOS and was fixed in a similar way. Moved some code around to have a method that updates a list of node that we can use to update the node in the animated event handler and also use it in the animation update loop. **Test plan** Tested that it did fix sticky headers lagging a frame behind during momentum scrolling in my PR facebook#11315 and also tested the native animations examples still work properly. Closes facebook#11994 Reviewed By: mkonicek Differential Revision: D4488977 Pulled By: sahrens fbshipit-source-id: 831a1565bc7b8fa88cadd5a8c1be876fbdefbf66

sahrens · 2017-03-02T03:58:15Z

Finally trying to land this again...wish it luck!

nihgwu · 2017-03-02T14:53:44Z

land to where?

sahrens · 2017-03-02T23:17:34Z

Fixed the internal test this was breaking so this should be in master soon!

janicduplessis · 2017-03-02T23:19:50Z

🎉 Thanks @sahrens for working on landing this!

janicduplessis · 2017-03-02T23:28:38Z

@sahrens I just recalled that when landing the first version of this that was implemented natively (and that was reverted because of some bugs), having sticky headers enabled by default in ListView caused some issues for product teams at FB, since it's a breaking change in term of UI behavior. I think a good solution would be to make sticky section headers disabled by default on Android and enabled on iOS (https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/ListView/ListView.js#L333).

sahrens · 2017-03-03T03:38:23Z

Ah yeah, totally forgot we were going to make that change.

sahrens · 2017-03-03T04:25:26Z

Change to the default should go out soon.

janicduplessis · 2017-03-03T04:38:14Z

@sahrens Cool, thanks!

sahrens · 2017-03-03T05:12:35Z

Hmm, this might be causing some crashes - the interpolation range isn't always monotonic, maybe if the scroll offset is negative? Need to investigate further, might have to revert... On Mar 2, 2017, at 8:39 PM, Janic Duplessis <notifications@github.com<mailto:notifications@github.com>> wrote: @sahrens<https://github.com/sahrens> Cool, thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#11315 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABcJx_HnMl3LcMaHx9Yy8gB0FerOGFsSks5rh5ljgaJpZM4LE_sl>.

janicduplessis · 2017-03-03T05:26:19Z

Hmm there might be cases where headers overlap? We'll have to safeguard against negative interpolations.

sahrens · 2017-03-03T05:29:58Z

Seeing this in some tests:

inputRange must be monotonically increasing -1,0,0,-33,-32

janicduplessis · 2017-03-03T05:34:22Z

Looks like some headers have negative y coords, the code currently doesn't handle these properly. https://github.com/facebook/react-native/blob/master/Libraries/Components/ScrollView/ScrollViewStickyHeader.js#L63

janicduplessis · 2017-03-03T05:37:01Z

I'm not sure if just making sure the y coord is > 0 would work properly, let me know if you find what the failing test looks like.

Summary: They aren't normal for android, so don't make that the default. some more context: #11315 (comment) Reviewed By: furdei Differential Revision: D4648714 fbshipit-source-id: 3232a6914e3db82c2b300409663ce63412d892a6

Summary: This re-implements sticky headers in JS to make it work on Android. The only change that was needed was to expose a way to attach a an animated value to an event manually since we can't use the Animated wrapper and `Animated.event` to do it for us because this is implemented directly in the `ScrollView` component. Simply exposed `attachNativeEvent` that takes a ref, event name and event object mapping. This is what is used by `Animated.event`. TODO: - Need to check why momentum scrolling isn't triggering scroll events properly on Android. - Remove native iOS implementation - cleanup / fix flow **Test plan** Test the example list in UIExplorer, test the ListViewPaging example. Closes facebook/react-native#11315 Differential Revision: D4450278 Pulled By: sahrens fbshipit-source-id: fec8da2cffce9807d74f8e518ebdefeb6a708667

jasan-s · 2017-03-10T13:17:29Z

is the latest commit still failing test? Are sticky list section header working in android?

sahrens · 2017-03-10T17:56:51Z

This should be all fixed and landed in master slated for v44 release around April. Note that behavior is slightly changed for headers because we wrap them in an Animated.View, so for example onLayout inside your header may give different results after this change.

Summary: This re-implements sticky headers in JS to make it work on Android. The only change that was needed was to expose a way to attach a an animated value to an event manually since we can't use the Animated wrapper and `Animated.event` to do it for us because this is implemented directly in the `ScrollView` component. Simply exposed `attachNativeEvent` that takes a ref, event name and event object mapping. This is what is used by `Animated.event`. TODO: - Need to check why momentum scrolling isn't triggering scroll events properly on Android. - Remove native iOS implementation - cleanup / fix flow **Test plan** Test the example list in UIExplorer, test the ListViewPaging example. Closes facebook#11315 Differential Revision: D4450278 Pulled By: sahrens fbshipit-source-id: fec8da2cffce9807d74f8e518ebdefeb6a708667

Summary: They aren't normal for android, so don't make that the default. some more context: facebook#11315 (comment) Reviewed By: furdei Differential Revision: D4648714 fbshipit-source-id: 3232a6914e3db82c2b300409663ce63412d892a6

chrisknepper · 2017-05-03T14:51:49Z

This appears to have been merged and released in 0.44. Is there any reason this (rewrite/Android support) was not mentioned in the release notes?

janicduplessis · 2017-05-03T18:09:03Z

Probably slipped through, let me see if I can edit release notes.

dgruseck · 2017-05-03T19:24:55Z

Works perfectly on Android and iOS.
Thank you very much, waited a long time for this.

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 Dec 6, 2016

janicduplessis mentioned this pull request Dec 6, 2016

[Android] Implement ScrollView sticky headers on Android #9957

Closed

janicduplessis commented Dec 6, 2016

View reviewed changes

nihgwu mentioned this pull request Jan 2, 2017

Add a prop to toggle ListView sticky section headers #11700

Closed

sahrens suggested changes Jan 18, 2017

View reviewed changes

janicduplessis force-pushed the sticky-headers-animated branch from 8ee2482 to 26e8c5a Compare January 19, 2017 21:12

janicduplessis mentioned this pull request Jan 19, 2017

Fix native animated event lag on Android #11994

Closed

eslint-bot reviewed Jan 20, 2017

View reviewed changes

facebook-github-bot closed this in 77b8c09 Mar 2, 2017

janicduplessis mentioned this pull request Apr 4, 2017

Improve z-index implementation on Android #13105

Closed

		};

		Object.assign((Animated: Object), AnimatedImplementation);

Implement sticky headers in JS using Native Animated #11315

Implement sticky headers in JS using Native Animated #11315

Conversation

janicduplessis commented Dec 6, 2016 • edited Loading

facebook-github-bot commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nihgwu commented Dec 6, 2016

janicduplessis commented Dec 6, 2016

janicduplessis commented Dec 6, 2016

nihgwu commented Dec 6, 2016

brentvatne commented Dec 7, 2016

ide commented Dec 7, 2016

nihgwu commented Dec 8, 2016 via email

nihgwu commented Jan 6, 2017

jeanregisser commented Jan 10, 2017

janicduplessis commented Jan 10, 2017

nihgwu commented Jan 11, 2017 • edited Loading

jeanregisser commented Jan 11, 2017

janicduplessis commented Jan 11, 2017

nihgwu commented Jan 11, 2017

nihgwu commented Jan 11, 2017

janicduplessis commented Jan 11, 2017

sahrens commented Jan 17, 2017

janicduplessis commented Jan 17, 2017

sahrens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Feb 13, 2017 • edited Loading

sahrens commented Feb 13, 2017

sahrens commented Mar 2, 2017

nihgwu commented Mar 2, 2017

sahrens commented Mar 2, 2017 • edited Loading

janicduplessis commented Mar 2, 2017

janicduplessis commented Mar 2, 2017

sahrens commented Mar 3, 2017

sahrens commented Mar 3, 2017

janicduplessis commented Mar 3, 2017

sahrens commented Mar 3, 2017 via email

janicduplessis commented Mar 3, 2017

sahrens commented Mar 3, 2017

janicduplessis commented Mar 3, 2017 • edited Loading

janicduplessis commented Mar 3, 2017

jasan-s commented Mar 10, 2017 • edited Loading

sahrens commented Mar 10, 2017

chrisknepper commented May 3, 2017

janicduplessis commented May 3, 2017

dgruseck commented May 3, 2017

janicduplessis commented Dec 6, 2016 •

edited

Loading

nihgwu commented Jan 11, 2017 •

edited

Loading

janicduplessis commented Feb 13, 2017 •

edited

Loading

sahrens commented Mar 2, 2017 •

edited

Loading

janicduplessis commented Mar 3, 2017 •

edited

Loading

jasan-s commented Mar 10, 2017 •

edited

Loading