[Android] Implement ScrollView sticky headers on Android #9456

Closed
wants to merge 10 commits into
from

Conversation

Projects
None yet
9 participants
@janicduplessis
Collaborator

janicduplessis commented Aug 17, 2016

This adds support for sticky headers on Android. The implementation if based primarily on the iOS one (https://github.com/facebook/react-native/blob/master/React/Views/RCTScrollView.m#L272) and adds some stuff that was missing to be able to handle z-index, view clipping, view hierarchy optimization and touch handling properly.

Some notable changes:

  • Add ChildDrawingOrderDelegate interface to allow changing the ViewGroup drawing order using ViewGroup#getChildDrawingOrder. This is used to change the content view drawing order to make sure headers are drawn over the other cells. Right now I'm only reversing the drawing order as drawing only the header views last added a lot of complexity especially because of view clipping and I don't think it should cause issues.
  • Add collapsableChildren prop that works like collapsable but applies to every child of the view. This is needed to be able to reference sticky headers by their indices otherwise some subviews can get optimized out and break indexes.
  • Make TouchTargetHelper take drawing order into consideration when finding the touch target.
  • Take translate into consideration when clipping subviews.

Test plan
Tested using the UIExplorer main menu on android.
Tested that touches get dispatched to the sticky header when over other touchable views.
Tested that touchables inside a sticky header work properly.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 17, 2016

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

ghost commented Aug 17, 2016

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

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Aug 17, 2016

Collaborator

cc @brentvatne :D

Collaborator

satya164 commented Aug 17, 2016

cc @brentvatne :D

@ghost ghost added the CLA Signed label Aug 17, 2016

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Aug 18, 2016

Collaborator

@janicduplessis - tried it out, seems great! any idea about the CI failures?

Collaborator

brentvatne commented Aug 18, 2016

@janicduplessis - tried it out, seems great! any idea about the CI failures?

@ghost ghost added the CLA Signed label Aug 18, 2016

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 18, 2016

Collaborator

Looks like the instrumentation tests are failing, I'll run them locally to see what is going on.

Collaborator

janicduplessis commented Aug 18, 2016

Looks like the instrumentation tests are failing, I'll run them locally to see what is going on.

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Aug 18, 2016

Contributor

Did you consider implementing this with the new native-driven Animated.event? Seems like it would be a little awkward to describe with Animated, but doable.

Contributor

sahrens commented Aug 18, 2016

Did you consider implementing this with the new native-driven Animated.event? Seems like it would be a little awkward to describe with Animated, but doable.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Aug 19, 2016

Collaborator

Did you consider implementing this with the new native-driven Animated.event? Seems like it would be a little awkward to describe with Animated, but doable.

@sahrens - probably doable, but I think we should wait for offload Animated and Animated event stuff to be more mature first, then switch the iOS and Android implementations of sticky headers at the same time.

Collaborator

brentvatne commented Aug 19, 2016

Did you consider implementing this with the new native-driven Animated.event? Seems like it would be a little awkward to describe with Animated, but doable.

@sahrens - probably doable, but I think we should wait for offload Animated and Animated event stuff to be more mature first, then switch the iOS and Android implementations of sticky headers at the same time.

@ghost ghost added the CLA Signed label Aug 19, 2016

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Aug 19, 2016

Contributor

bravo

Contributor

nihgwu commented Aug 19, 2016

bravo

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 19, 2016

Collaborator

@sahrens I agree with @brentvatne, it would be really nice to eventually convert it but for now I think this is better.

Collaborator

janicduplessis commented Aug 19, 2016

@sahrens I agree with @brentvatne, it would be really nice to eventually convert it but for now I think this is better.

@ghost ghost added the CLA Signed label Aug 19, 2016

@@ -152,6 +175,22 @@ protected void onScrollChanged(int x, int y, int oldX, int oldY) {
}
+ @Override
+ public View hitTest(float[] coordinates) {

This comment has been minimized.

@astreet

astreet Aug 23, 2016

Contributor

Is this needed because our touch handling doesn't respect drawing order? If so, can we just make our normal touch handling handle drawing order so that we don't have to add a new touch handling code path?

@astreet

astreet Aug 23, 2016

Contributor

Is this needed because our touch handling doesn't respect drawing order? If so, can we just make our normal touch handling handle drawing order so that we don't have to add a new touch handling code path?

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Yea, good idea, I just checked and AOSP does use getChildDrawingOrder when dispatching a touch event to subviews.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Yea, good idea, I just checked and AOSP does use getChildDrawingOrder when dispatching a touch event to subviews.

@@ -289,7 +294,11 @@ private void updateClippingToRect(Rect clippingRect) {
private void updateSubviewClipStatus(Rect clippingRect, int idx, int clippedSoFar) {
View child = Assertions.assertNotNull(mAllChildren)[idx];
- sHelperRect.set(child.getLeft(), child.getTop(), child.getRight(), child.getBottom());
+ sHelperRect.set(
+ child.getLeft() + (int) child.getTranslationX(),

This comment has been minimized.

@astreet

astreet Aug 23, 2016

Contributor

This might not be the right way to round: what does AOSP do here, do you know?

@astreet

astreet Aug 23, 2016

Contributor

This might not be the right way to round: what does AOSP do here, do you know?

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

@astreet Updated to do like AOSP, use Matrix#mapRect and take a sligtly larger rect when converting from a float rect to a int rect. This make sure that float -> int doesn't remove views that may still be visible.

https://github.com/android/platform_frameworks_base/blob/4535e11fb7010f2b104d3f8b3954407b9f330e0f/core/java/android/view/ViewGroup.java#L5049

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

@astreet Updated to do like AOSP, use Matrix#mapRect and take a sligtly larger rect when converting from a float rect to a int rect. This make sure that float -> int doesn't remove views that may still be visible.

https://github.com/android/platform_frameworks_base/blob/4535e11fb7010f2b104d3f8b3954407b9f330e0f/core/java/android/view/ViewGroup.java#L5049

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Actually I changed it to (floor, floor, ceil, ceil) that way it keeps the same behaviour for non decimal numbers and will still make sure to never clip views that end up with decimal bounds after transform.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Actually I changed it to (floor, floor, ceil, ceil) that way it keeps the same behaviour for non decimal numbers and will still make sure to never clip views that end up with decimal bounds after transform.

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Aug 23, 2016

Contributor

Re collapsableChildren vs cloning for the collapsable prop: is there actually a perf concern here? If possible, I'd really rather not add another prop we need to support.

Contributor

astreet commented Aug 23, 2016

Re collapsableChildren vs cloning for the collapsable prop: is there actually a perf concern here? If possible, I'd really rather not add another prop we need to support.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 24, 2016

@janicduplessis updated the pull request - view changes

ghost commented Aug 24, 2016

@janicduplessis updated the pull request - view changes

@ghost ghost added the CLA Signed label Aug 24, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@janicduplessis updated the pull request - view changes

@ghost ghost added the CLA Signed label Aug 24, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 24, 2016

@janicduplessis updated the pull request - view changes

ghost commented Aug 24, 2016

@janicduplessis updated the pull request - view changes

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 24, 2016

@janicduplessis updated the pull request - view changes

ghost commented Aug 24, 2016

@janicduplessis updated the pull request - view changes

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

@astreet Yea, unfounded perf concerns :), changed it to cloning child elements in JS. We can change it in tthe future if it is actually an issue.

Collaborator

janicduplessis commented Aug 24, 2016

@astreet Yea, unfounded perf concerns :), changed it to cloning child elements in JS. We can change it in tthe future if it is actually an issue.

@ghost ghost added the CLA Signed label Aug 24, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 24, 2016

@janicduplessis updated the pull request - view changes

ghost commented Aug 24, 2016

@janicduplessis updated the pull request - view changes

@ghost ghost added CLA Signed labels Aug 24, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 24, 2016

@janicduplessis updated the pull request - view changes

ghost commented Aug 24, 2016

@janicduplessis updated the pull request - view changes

@ghost ghost added the CLA Signed label Aug 24, 2016

@@ -437,7 +437,7 @@ private void transitionLayoutOnlyViewToNativeView(
private static boolean isLayoutOnlyAndCollapsable(@Nullable ReactStylesDiffMap props) {
if (props == null) {
- return true;
+ return false;

This comment has been minimized.

@astreet

astreet Aug 24, 2016

Contributor

Why this change?

@astreet

astreet Aug 24, 2016

Contributor

Why this change?

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Yea I was going to comment on this, If I don't do this native view counts doesn't match sticky indices. I think it is because of null react elements. Any other idea other than doing this?

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Yea I was going to comment on this, If I don't do this native view counts doesn't match sticky indices. I think it is because of null react elements. Any other idea other than doing this?

This comment has been minimized.

@astreet

astreet Aug 24, 2016

Contributor

But we're setting collapsable=false on all those children though -- I don't understand why that wouldn't be enough.

@astreet

astreet Aug 24, 2016

Contributor

But we're setting collapsable=false on all those children though -- I don't understand why that wouldn't be enough.

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Some children are null so we can't set collapsable = false on them. I had to add a null check in children map here 1e87a60#diff-f8f8b220fc0d1573e4f8b78c84415075R455

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Some children are null so we can't set collapsable = false on them. I had to add a null check in children map here 1e87a60#diff-f8f8b220fc0d1573e4f8b78c84415075R455

This comment has been minimized.

@astreet

astreet Aug 26, 2016

Contributor

I see. Then we should either have JS send the tags of the children that are sticky headers which the native view could then convert into indices (which would also fix the whole collapsable problem), or less ideally remap the indices in JS.

@astreet

astreet Aug 26, 2016

Contributor

I see. Then we should either have JS send the tags of the children that are sticky headers which the native view could then convert into indices (which would also fix the whole collapsable problem), or less ideally remap the indices in JS.

This comment has been minimized.

@janicduplessis

janicduplessis Aug 26, 2016

Collaborator

How would you go about getting the sticky headers view tags in JS? Seems like it would be relatively complex since we can only get them after the view has been rendered using refs. Maybe remapping indexes to offset for null views that will be removed would end up a lot simpler.

@janicduplessis

janicduplessis Aug 26, 2016

Collaborator

How would you go about getting the sticky headers view tags in JS? Seems like it would be relatively complex since we can only get them after the view has been rendered using refs. Maybe remapping indexes to offset for null views that will be removed would end up a lot simpler.

This comment has been minimized.

@janicduplessis

janicduplessis Aug 27, 2016

Collaborator

Ok this is actually harder than I thought because just adding collapsable=false doesn't really work because children are not actually View components so adding the collapsable prop doesn't do anything. In the case of ListView it's always a StaticRenderer which ignores the collapsable prop. I'm kind of leaning back towards adding the collapsableChildren prop to View that ensures that all it's children map to a native View and don't get optimized out.

Here's a simple example to show why adding the collapsable props to ScrollView children in JS doesn't work:

const CustomView = ({ text }) => <View><Text>{text}</Text></View>;

<ScrollView>
   // Works, view gets rendered with `collapsable=false`
  <View />
  // Doesn't work because the collapsable prop gets passed to `CustomView` but is not 
  // passed down to it's `View` child
  <CustomView text="hello" />
</ScrollView>
@janicduplessis

janicduplessis Aug 27, 2016

Collaborator

Ok this is actually harder than I thought because just adding collapsable=false doesn't really work because children are not actually View components so adding the collapsable prop doesn't do anything. In the case of ListView it's always a StaticRenderer which ignores the collapsable prop. I'm kind of leaning back towards adding the collapsableChildren prop to View that ensures that all it's children map to a native View and don't get optimized out.

Here's a simple example to show why adding the collapsable props to ScrollView children in JS doesn't work:

const CustomView = ({ text }) => <View><Text>{text}</Text></View>;

<ScrollView>
   // Works, view gets rendered with `collapsable=false`
  <View />
  // Doesn't work because the collapsable prop gets passed to `CustomView` but is not 
  // passed down to it's `View` child
  <CustomView text="hello" />
</ScrollView>

This comment has been minimized.

@astreet

astreet Sep 2, 2016

Contributor

Ok, I understand the problems you're having. Ideally we'd have some way to refer to the child views other than by index (since it isn't stable), but that doesn't exist right now and I don't know a good way to do it. I'm fine doing collapsableChildren false in this case then. (Actually, another option would be a create a custom StickyHeader native view class that ScrollView knows about and can do instanceof checks in native... but that would require a change in API)

@astreet

astreet Sep 2, 2016

Contributor

Ok, I understand the problems you're having. Ideally we'd have some way to refer to the child views other than by index (since it isn't stable), but that doesn't exist right now and I don't know a good way to do it. I'm fine doing collapsableChildren false in this case then. (Actually, another option would be a create a custom StickyHeader native view class that ScrollView knows about and can do instanceof checks in native... but that would require a change in API)

This comment has been minimized.

@janicduplessis

janicduplessis Sep 5, 2016

Collaborator

Let's go with collapsableChildren, this will allow to keep the same API and could even be useful in the future for other cases where we want an index based API like stickyHeaderIndices.

@janicduplessis

janicduplessis Sep 5, 2016

Collaborator

Let's go with collapsableChildren, this will allow to keep the same API and could even be useful in the future for other cases where we want an index based API like stickyHeaderIndices.

- boolean intersects = mClippingRect
- .intersects(sHelperRect.left, sHelperRect.top, sHelperRect.right, sHelperRect.bottom);
+ Matrix matrix = subview.getMatrix();
+ if (!matrix.isIdentity()) {

This comment has been minimized.

@astreet

astreet Aug 24, 2016

Contributor

So, I realized that if the translation/rotation/etc are updated after this happens, clipping will no longer be correct. It's probably better to not factor in translation/etc until we have a way to solve that (if we ever do: it doesn't seem trivial to me :( )

@astreet

astreet Aug 24, 2016

Contributor

So, I realized that if the translation/rotation/etc are updated after this happens, clipping will no longer be correct. It's probably better to not factor in translation/etc until we have a way to solve that (if we ever do: it doesn't seem trivial to me :( )

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Maybe disable clipping if the view has a transform? Because clipping + transform is broken at the moment anyway.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

Maybe disable clipping if the view has a transform? Because clipping + transform is broken at the moment anyway.

This comment has been minimized.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

We could also call updateClippingRect after updating the transform prop. This will cause a bit of extra work but will make sure that whatever happens last (setting transform or updating layout) will calculate the right clipping.

@janicduplessis

janicduplessis Aug 24, 2016

Collaborator

We could also call updateClippingRect after updating the transform prop. This will cause a bit of extra work but will make sure that whatever happens last (setting transform or updating layout) will calculate the right clipping.

This comment has been minimized.

@astreet

astreet Aug 25, 2016

Contributor

Are we actually hitting this right now?

@astreet

astreet Aug 25, 2016

Contributor

Are we actually hitting this right now?

This comment has been minimized.

@janicduplessis

janicduplessis Aug 25, 2016

Collaborator

Yea, with sticky headers we translate them so they stay on the top of the scrollview when normally they should be outside of clip bounds without the transform.

@janicduplessis

janicduplessis Aug 25, 2016

Collaborator

Yea, with sticky headers we translate them so they stay on the top of the scrollview when normally they should be outside of clip bounds without the transform.

This comment has been minimized.

@astreet

astreet Aug 25, 2016

Contributor

mm, right. When you say

We could also call updateClippingRect after updating the transform prop.

How exactly would that work?

@astreet

astreet Aug 25, 2016

Contributor

mm, right. When you say

We could also call updateClippingRect after updating the transform prop.

How exactly would that work?

This comment has been minimized.

@janicduplessis

janicduplessis Aug 25, 2016

Collaborator

I was thinking of something like

if (view instanceof ReactClippingViewGroup) {
  ((ReactClippingViewGroup) view).updateClippingRect();
}

in https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java#L53

Edit: actually we need to call it on the view's parent but it should be similar.

@janicduplessis

janicduplessis Aug 25, 2016

Collaborator

I was thinking of something like

if (view instanceof ReactClippingViewGroup) {
  ((ReactClippingViewGroup) view).updateClippingRect();
}

in https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java#L53

Edit: actually we need to call it on the view's parent but it should be similar.

This comment has been minimized.

@astreet

astreet Aug 25, 2016

Contributor

Yeah, that sounds reasonable if you can get it to work. The best other solution I can think of is just not clipping transformed views no matter what, but I'm worried some developer is going to translate images (e.g. SoundCloud's android app as you scroll the main screen) and have a bad time.

@astreet

astreet Aug 25, 2016

Contributor

Yeah, that sounds reasonable if you can get it to work. The best other solution I can think of is just not clipping transformed views no matter what, but I'm worried some developer is going to translate images (e.g. SoundCloud's android app as you scroll the main screen) and have a bad time.

This comment has been minimized.

@janicduplessis

janicduplessis Aug 27, 2016

Collaborator

Got it working with https://github.com/facebook/react-native/pull/9456/files#diff-a73c2a0c22eceb34cf86aaede40f931fR192. Tested that it does indeed work by animating the transform prop in a clipped view and I can confirm that you were right and it did not work before but now works with this fix.

@janicduplessis

janicduplessis Aug 27, 2016

Collaborator

Got it working with https://github.com/facebook/react-native/pull/9456/files#diff-a73c2a0c22eceb34cf86aaede40f931fR192. Tested that it does indeed work by animating the transform prop in a clipped view and I can confirm that you were right and it did not work before but now works with this fix.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 25, 2016

@janicduplessis updated the pull request - view changes

ghost commented Aug 25, 2016

@janicduplessis updated the pull request - view changes

@ghost ghost added the CLA Signed label Aug 25, 2016

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Aug 30, 2016

Contributor

what's going on, waiting for this feature to be merged😁

Contributor

nihgwu commented Aug 30, 2016

what's going on, waiting for this feature to be merged😁

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Aug 31, 2016

Collaborator

Just need to figure out a few implementation details with @astreet, shouldn't take too long :)

Collaborator

janicduplessis commented Aug 31, 2016

Just need to figure out a few implementation details with @astreet, shouldn't take too long :)

@ghost ghost added the CLA Signed label Aug 31, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 5, 2016

@janicduplessis updated the pull request - view changes

ghost commented Sep 5, 2016

@janicduplessis updated the pull request - view changes

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 5, 2016

@janicduplessis updated the pull request - view changes

ghost commented Sep 5, 2016

@janicduplessis updated the pull request - view changes

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Sep 14, 2016

Contributor

Cool, scrollview refactor landed as 2cf2fdb

Contributor

astreet commented Sep 14, 2016

Cool, scrollview refactor landed as 2cf2fdb

@ghost

This comment has been minimized.

Show comment
Hide comment

@ghost ghost added the CLA Signed label Sep 14, 2016

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 14, 2016

Collaborator

@astreet Ok should be all good now, tests were failing because the change in NativeViewHierachyOptimizer caused it not to optimize out views that their parent is null. Fixed in 51e0acc. Now tests are passing locally.

Collaborator

janicduplessis commented Sep 14, 2016

@astreet Ok should be all good now, tests were failing because the change in NativeViewHierachyOptimizer caused it not to optimize out views that their parent is null. Fixed in 51e0acc. Now tests are passing locally.

@ghost ghost added the CLA Signed label Sep 14, 2016

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 14, 2016

Collaborator
Collaborator

ide commented Sep 14, 2016

@ghost ghost added the Import Started label Sep 14, 2016

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 14, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

ghost commented Sep 14, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost added the CLA Signed label Sep 14, 2016

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 15, 2016

Contributor

OMG, failing still?

Contributor

nihgwu commented Sep 15, 2016

OMG, failing still?

@ghost ghost closed this in 0e8b75b Sep 15, 2016

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 15, 2016

Collaborator

Landed now :D

Collaborator

janicduplessis commented Sep 15, 2016

Landed now :D

nihgwu added a commit to nihgwu/react-native that referenced this pull request Sep 15, 2016

add missing examples
since #9456 has been landed, it's the time to add ListViewPagingExample to the example list for Android
@kenma9123

This comment has been minimized.

Show comment
Hide comment
@kenma9123

kenma9123 Sep 15, 2016

Been waiting for this. Thank you!

Been waiting for this. Thank you!

ghost pushed a commit that referenced this pull request Sep 15, 2016

add missing uiexplorer examples on android
Summary:
since #9456 has been landed, it's the time to add ListViewPagingExample to the example list for Android
Closes #9930

Differential Revision: D3871517

fbshipit-source-id: dafa8ab2d704bcc5e7bc25b5fcec18a1a6cfa28a

ghost pushed a commit that referenced this pull request Sep 15, 2016

Reverted commit D3827366
Summary:
This adds support for sticky headers on Android. The implementation if based primarily on the iOS one (https://github.com/facebook/react-native/blob/master/React/Views/RCTScrollView.m#L272) and adds some stuff that was missing to be able to handle z-index, view clipping, view hierarchy optimization and touch handling properly.

Some notable changes:
- Add `ChildDrawingOrderDelegate` interface to allow changing the `ViewGroup` drawing order using `ViewGroup#getChildDrawingOrder`. This is used to change the content view drawing order to make sure headers are drawn over the other cells. Right now I'm only reversing the drawing order as drawing only the header views last added a lot of complexity especially because of view clipping and I don't think it should cause issues.

- Add `collapsableChildren` prop that works like `collapsable` but applies to every child of the view. This is needed to be able to reference sticky headers by their indices otherwise some subviews can get optimized out and break indexes.
Closes #9456

Differential Revision: D3827366

Pulled By: fred2028

fbshipit-source-id: d346068734c5b987518794ab23e13914ed13b5c4
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 15, 2016

Collaborator

Reverted in d0d1712

@astreet Could you paste stack traces / the reason for the revert here so we can fix it?

Collaborator

ide commented Sep 15, 2016

Reverted in d0d1712

@astreet Could you paste stack traces / the reason for the revert here so we can fix it?

@brentvatne brentvatne reopened this Sep 15, 2016

@ghost ghost added the CLA Signed label Sep 15, 2016

@ghost ghost added the CLA Signed label Sep 15, 2016

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Sep 16, 2016

Contributor

Yep, sorry for posting after the fact, this got reverted after I left for the day. We hit a couple problems after this landed:

  1. The next-gen version of the RN Android rendering stack (nodes) doesn't use ReactViewGroup (and therefore doesn't use ReactViewManager). This was fixed in 5c3f954 (but this was also reverted as part of the revert of the larger stickyheader commit). When we re-land, we should incorporate this fix.
  2. We got this bug report from adsmanager that bisects to this diff ("our scrollview has 2 views, 1 on top of another, lets say A on top of B. We can swipe A horizontally to reveal B. With this diff, B is always shown instead of A"). I haven't had a chance to try to repro, but I imagine this has to do with the drawing order changes. I'll try to provide more information when I get a chance to look into it, but I had them go ahead and revert since they have a release coming up in a few days.
Contributor

astreet commented Sep 16, 2016

Yep, sorry for posting after the fact, this got reverted after I left for the day. We hit a couple problems after this landed:

  1. The next-gen version of the RN Android rendering stack (nodes) doesn't use ReactViewGroup (and therefore doesn't use ReactViewManager). This was fixed in 5c3f954 (but this was also reverted as part of the revert of the larger stickyheader commit). When we re-land, we should incorporate this fix.
  2. We got this bug report from adsmanager that bisects to this diff ("our scrollview has 2 views, 1 on top of another, lets say A on top of B. We can swipe A horizontally to reveal B. With this diff, B is always shown instead of A"). I haven't had a chance to try to repro, but I imagine this has to do with the drawing order changes. I'll try to provide more information when I get a chance to look into it, but I had them go ahead and revert since they have a release coming up in a few days.
@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Sep 16, 2016

Contributor

Looking into this more, one of the things that broke was that ListView automatically enables sticky headers for section headers. Some of the code in adsmanager for android apparently doesn't properly identify what things are section headers vs. normal rows and we now have random rows sticking to the top ha.

I think before this diff lands, we need to have whether stick headers are enabled for ListView customizable. And, given the design patterns on the two platforms, it should probably be enabled by default on iOS (as it is currently) and DISABLED by default on Android. Blindly copying design patterns from iOS to Android, like with sticky headers, is one of those things that identifies your framework as a lowest-common denominator crossplatform solution which we don't want. That being said, I think there are legit use cases for it, I just don't want it to become a 'thing' for RN apps on Android.

Contributor

astreet commented Sep 16, 2016

Looking into this more, one of the things that broke was that ListView automatically enables sticky headers for section headers. Some of the code in adsmanager for android apparently doesn't properly identify what things are section headers vs. normal rows and we now have random rows sticking to the top ha.

I think before this diff lands, we need to have whether stick headers are enabled for ListView customizable. And, given the design patterns on the two platforms, it should probably be enabled by default on iOS (as it is currently) and DISABLED by default on Android. Blindly copying design patterns from iOS to Android, like with sticky headers, is one of those things that identifies your framework as a lowest-common denominator crossplatform solution which we don't want. That being said, I think there are legit use cases for it, I just don't want it to become a 'thing' for RN apps on Android.

@astreet

This comment has been minimized.

Show comment
Hide comment
@astreet

astreet Sep 16, 2016

Contributor

I haven't been able to get a repro for the rendering issue that was described to me (where B renders over A), but I have a feeling it's because stickyHeaderIndices is causing us to identify the wrong View as being sticky somehow.

I'd really feel a lot better about this API if, instead of passing in indices, we wrapped the views that are supposed to be sticky in a custom native view that the ScrollView can identify. That way, we aren't hurt by collapsable optimizations and don't need to make sure stickyHeaderIndices and the ListView dataSource stay in sync.

Contributor

astreet commented Sep 16, 2016

I haven't been able to get a repro for the rendering issue that was described to me (where B renders over A), but I have a feeling it's because stickyHeaderIndices is causing us to identify the wrong View as being sticky somehow.

I'd really feel a lot better about this API if, instead of passing in indices, we wrapped the views that are supposed to be sticky in a custom native view that the ScrollView can identify. That way, we aren't hurt by collapsable optimizations and don't need to make sure stickyHeaderIndices and the ListView dataSource stay in sync.

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 16, 2016

Contributor

ScrollView Sticky Header on Android is really important to me, I'm building a cross-platform app, it's really ugly without sticky header on Android, I've made a js workaround but far from satisfaction, so I build RN from source to use this feature, and it works great, although there are still some bugs but it's fine in my case.

I think it's deserve to be kept in the master branch for further improvement, while providing an option to disable it on Android for consistency, e.g. stickyHeaderIndices={[]}

Contributor

nihgwu commented Sep 16, 2016

ScrollView Sticky Header on Android is really important to me, I'm building a cross-platform app, it's really ugly without sticky header on Android, I've made a js workaround but far from satisfaction, so I build RN from source to use this feature, and it works great, although there are still some bugs but it's fine in my case.

I think it's deserve to be kept in the master branch for further improvement, while providing an option to disable it on Android for consistency, e.g. stickyHeaderIndices={[]}

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 16, 2016

Contributor

And I've noticed the random sticking issue in UIExplorer, the example list in the drawer and React Native Examples has section headers but stick unexpectedly, while it's works well in ListViewPagingExample

Contributor

nihgwu commented Sep 16, 2016

And I've noticed the random sticking issue in UIExplorer, the example list in the drawer and React Native Examples has section headers but stick unexpectedly, while it's works well in ListViewPagingExample

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 16, 2016

Collaborator

@astreet I think I know why tbe rendering but you describe happens. Gonna have a look at this as well as the issues @nihgwu found out today.

I also agree that we should make this disabled by default on android and add a flag to listview to enable it. Thisatches better what is common on the platform and will also avoid making this a breaking change.

Collaborator

janicduplessis commented Sep 16, 2016

@astreet I think I know why tbe rendering but you describe happens. Gonna have a look at this as well as the issues @nihgwu found out today.

I also agree that we should make this disabled by default on android and add a flag to listview to enable it. Thisatches better what is common on the platform and will also avoid making this a breaking change.

@ghost ghost added the CLA Signed label Sep 16, 2016

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 16, 2016

Collaborator

Also for using a custom native view for headers I agree this would be a better and more reliable API but it should still be possible to make indices work, it's just a matter of fixing a few remaining bugs then it will be stable.

Collaborator

janicduplessis commented Sep 16, 2016

Also for using a custom native view for headers I agree this would be a better and more reliable API but it should still be possible to make indices work, it's just a matter of fixing a few remaining bugs then it will be stable.

@nihgwu

This comment has been minimized.

Show comment
Hide comment
@nihgwu

nihgwu Sep 16, 2016

Contributor

I add a renderStickyHeader in my workaround ListView for Android implemented in js , but it looks silly as there is already renderSectionHeader means to render sticky header on iOS

Contributor

nihgwu commented Sep 16, 2016

I add a renderStickyHeader in my workaround ListView for Android implemented in js , but it looks silly as there is already renderSectionHeader means to render sticky header on iOS

@janicduplessis

This comment has been minimized.

Show comment
Hide comment
@janicduplessis

janicduplessis Sep 16, 2016

Collaborator

I'll close this PR and open a new one since we already shipped this, not sure if the bot will work properly.

Collaborator

janicduplessis commented Sep 16, 2016

I'll close this PR and open a new one since we already shipped this, not sure if the bot will work properly.

rozele pushed a commit to Microsoft/react-native-windows that referenced this pull request Sep 27, 2016

Implement ScrollView sticky headers on Android
Summary:
This adds support for sticky headers on Android. The implementation if based primarily on the iOS one (https://github.com/facebook/react-native/blob/master/React/Views/RCTScrollView.m#L272) and adds some stuff that was missing to be able to handle z-index, view clipping, view hierarchy optimization and touch handling properly.

Some notable changes:
- Add `ChildDrawingOrderDelegate` interface to allow changing the `ViewGroup` drawing order using `ViewGroup#getChildDrawingOrder`. This is used to change the content view drawing order to make sure headers are drawn over the other cells. Right now I'm only reversing the drawing order as drawing only the header views last added a lot of complexity especially because of view clipping and I don't think it should cause issues.

- Add `collapsableChildren` prop that works like `collapsable` but applies to every child of the view. This is needed to be able to reference sticky headers by their indices otherwise some subviews can get optimized out and break indexes.
Closes facebook/react-native#9456

Differential Revision: D3827366

fbshipit-source-id: cab044cfdbe2ccb98e1ecd3e02ed3ceaa253eb78

rozele pushed a commit to Microsoft/react-native-windows that referenced this pull request Sep 27, 2016

Reverted commit D3827366
Summary:
This adds support for sticky headers on Android. The implementation if based primarily on the iOS one (https://github.com/facebook/react-native/blob/master/React/Views/RCTScrollView.m#L272) and adds some stuff that was missing to be able to handle z-index, view clipping, view hierarchy optimization and touch handling properly.

Some notable changes:
- Add `ChildDrawingOrderDelegate` interface to allow changing the `ViewGroup` drawing order using `ViewGroup#getChildDrawingOrder`. This is used to change the content view drawing order to make sure headers are drawn over the other cells. Right now I'm only reversing the drawing order as drawing only the header views last added a lot of complexity especially because of view clipping and I don't think it should cause issues.

- Add `collapsableChildren` prop that works like `collapsable` but applies to every child of the view. This is needed to be able to reference sticky headers by their indices otherwise some subviews can get optimized out and break indexes.
Closes facebook/react-native#9456

Differential Revision: D3827366

Pulled By: fred2028

fbshipit-source-id: d346068734c5b987518794ab23e13914ed13b5c4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment