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

[Android] Implement ScrollView sticky headers on Android #9957

Conversation

janicduplessis
Copy link
Contributor

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:

Adds a stickySectionHeaders prop on ListView to control whether section headers are sticky. This is true on iOS and false on Android by default.

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.

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.

Fixes issues after the original PR (#9456) was reverted .

Test plan
Tested using the UIExplorer main menu on android.
Tested the Paging ListView example that uses LayoutAnimation to make sure it works.
Tested that touches get dispatched to the sticky header when over other touchable views.
Tested that touchables inside a sticky header work properly.

@ghost
Copy link

ghost commented Sep 16, 2016

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

@ghost ghost 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 Sep 16, 2016
@janicduplessis
Copy link
Contributor Author

This includes @astreet's fix for nodes that was also reverted as part of the original PR.

I think the main issue that caused the revert was the fact that I used to simply inverse the drawing order instead of changing the drawing order to only affect sticky headers. This adds quite a bit of complexity but it works well and should fix the bug that caused this to be reverted.

This also adds a fix for a bug that happened with LayoutAnimation where the sticked header would get animated in a weird way. To fix this I added a way for a view to opt out of LayoutAnimation.

Lastly this adds a prop on ListView to control whether sections headers should be sticky or not. Defaults to true on ios and false on android so it should make this PR not affect any ListView unless they opt in. This will still affect apps that use stickySectionIndices directly as it used to be a noop on android.

@@ -66,6 +68,12 @@ public void reset() {
}

public boolean shouldAnimateLayout(View viewToAnimate) {
if (viewToAnimate instanceof ReactViewGroup) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this approach but layout animation didn't work properly at all here since the new position of the view isn't docked yet it gets animated in a weird way. What I did is simply add a flag to disable layout animation on ReactViewGroup.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 16, 2016
@astreet
Copy link
Contributor

astreet commented Sep 16, 2016

@facebook-github-bot import

@astreet
Copy link
Contributor

astreet commented Sep 16, 2016

I'm gonna pass this to the adsmanager team so they can test it out :)

@ghost
Copy link

ghost commented Sep 16, 2016

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

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 16, 2016
@ghost
Copy link

ghost commented Sep 17, 2016

@janicduplessis updated the pull request - view changes - changes since last import

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 17, 2016
@janicduplessis
Copy link
Contributor Author

@astreet Oups it didn't build with buck, fixed.

@ghost
Copy link

ghost commented Sep 17, 2016

@janicduplessis updated the pull request - view changes - changes since last import

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 17, 2016
@nihgwu
Copy link
Contributor

nihgwu commented Sep 17, 2016

got time to test this PR, and it's really awesome, all the bugs I mentioned before are resolved
BTW, I think you can also add stickySectionHeaders={true} to UIExplorerExampleList to show power of this feature 😄

@nihgwu
Copy link
Contributor

nihgwu commented Sep 17, 2016

unfortunately this new PR break my app completely 💔 , I'm making further investigating

@nihgwu
Copy link
Contributor

nihgwu commented Sep 17, 2016

Every time I change the stickySectionHeaders I get this error(HMR is enabled) seems change whatever will occur this error
image
image

The header should be transparent but get white background
image

The sticky header is overlapped by the content until another sticky header is triggered
image

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Sep 17, 2016

@nihgwu Could you send me the code that reproduces these issues?

Also thanks a lot for testing this :)

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 17, 2016
@nihgwu
Copy link
Contributor

nihgwu commented Sep 17, 2016

@janicduplessis The first issue should be introduced by recent commits to the master branch, the rest are only found in my app, but it works quite well with the old PR, quite strange, I've try a similar situation in UIExplorer, this new PR works well as expected. I'm struggling to find the cause

ScollView sticky header on Android and Native Animated Event are the last features I'd love to have, thanks for you great work

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 17, 2016
@facebook-github-bot
Copy link
Contributor

@janicduplessis updated the pull request - view changes - changes since last import

@janicduplessis
Copy link
Contributor Author

@nihgwu This PR changed the way we handle z-index by changing drawing order so it might be a bug with be new technique. First PR used to just reverse drawing order but that caused other issues, now it will keep the drawing order for non-sticky views and draw all sticky views at the end.

Lets say you have 5 rows:
1: sticky, 2: normal, 3: normal, 4: sticky, 5: normal

old PR:
5, 4, 3, 2, 1
new PR:
2, 3, 5, 1, 4

Code: https://github.com/facebook/react-native/pull/9957/files#diff-49a8f2a72edb3b8c02365cd11a8f2984R471

@janicduplessis
Copy link
Contributor Author

Thinking about this more I should not have to change the drawing order for all sticky headers, I could only change it for the one that is currenty sticked on top. It would probably simplify this quite a bit.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 17, 2016
@nihgwu
Copy link
Contributor

nihgwu commented Sep 17, 2016

I've notice that you change the drawing order, so I suspect that would be the cause for the different behave, I just don't know why the UIExplore is OK but my app isn't

@nihgwu
Copy link
Contributor

nihgwu commented Sep 18, 2016

Just test with the newest PR, the first issue is another one and I've create an issue #9969 , the second one no longer exists, but the third is still there, should be something wrong with the drawing order logic

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 27, 2016
@astreet
Copy link
Contributor

astreet commented Sep 28, 2016

I'm trying to think, is there a chance that the view that's supposed to be sticky could be collapsed? If so, we might need to automatically wrap the children at the sticky header indices in a View that has collapsable=false in JS to make sure this approach works.

@janicduplessis
Copy link
Contributor Author

Yea it should be possible that it happens. Something like

<View>
  <Text />
  <View style={{ backgroundColor: '#eee', height: 1 }} />
</View>

should get collapsed and is kind of common (pretty sure UIExplorer has something like this).

@astreet
Copy link
Contributor

astreet commented Oct 5, 2016

Felix, can you keep an eye on this/help review while I'm away?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 5, 2016
@nihgwu
Copy link
Contributor

nihgwu commented Oct 13, 2016

any progress on this?

@janicduplessis
Copy link
Contributor Author

@nihgwu No, haven't had the time to look at this yet

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2016
@facebook-github-bot
Copy link
Contributor

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @astreet as a potential reviewer. Could you take a look please or cc someone with more context?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 21, 2016
@astreet
Copy link
Contributor

astreet commented Nov 2, 2016

Hey, I was away during October. It looks like this is waiting on changes from you @janicduplessis?

@janicduplessis
Copy link
Contributor Author

Yes, I haven't had time to update this with what we discussed yet.

@ouabing
Copy link

ouabing commented Nov 4, 2016

looking forward to this feature too :)

@vitorebatista
Copy link

I need this!! :)

@Ehesp
Copy link
Contributor

Ehesp commented Nov 30, 2016

Any updates @janicduplessis? Would love to see this in!

@janicduplessis
Copy link
Contributor Author

Planning to work on this in the next few weeks.

@janicduplessis
Copy link
Contributor Author

Closing this and will open a new PR as I've used a completely different approach.

@janicduplessis
Copy link
Contributor Author

Follow up: #11315

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants