Improve z-index implementation on Android #13105

janicduplessis · 2017-03-23T08:03:09Z

Use getChildDrawingOrder instead of reordering views. The old implementation didn't work properly when removeClippedSubviews was enabled and this one should have better performance since we don't play with the view hierarchy at all.

This fixes weird bugs with sticky headers in SectionList and allows removing the hack that disabled removeClippedSubviews when using sticky section headers.

Test plan
Tested using the SectionList and ListViewPaging examples that use sticky headers which uses z-index.

nihgwu · 2017-03-23T08:06:04Z

👍

janicduplessis · 2017-03-23T08:09:17Z

cc @astreet

Summary: This adds support for both automagical sticky section headers in `SectionList` as well as the more free-form `stickyHeaderIndices` on `FlatList` or `VirtualizedList`. The basic concept is to take the initial `stickySectionHeaders` and remap them to the indices corresponding to the mounted subset in the render window. The main trick here is that the currently stuck header might itself be outside of the render window, so we need to search the gap to see if that's the case and render it (with spacers above and below it instead of one big spacer). In the `SectionList` we simply pre-compute the sticky headers at the same time as when we scan the sections to determine the flattened length and pass those to `VirtualizedList`. This also requires some updates to `ScrollView` to work in the churny environment of `VirtualizedList`. We propogate the keys on the children to the animated wrappers so that as items are removed and the indices of the remaining items change, react can keep proper track of them. We also fix the scroll back case where new headers are rendered from the top down and aren't updated with the `setNextLayoutY` callback because the `onLayout` call for the next header happened before it was mounted. This is done by just tracking all the layout values in a map and providing them to the sticky components at render time. This might also improve perf a little by property configuring the animations syncronously instead of waiting for the `onLayout` callback. We also need to protect against stale onLayout callbacks and other fun stuff. == Test Plan == https://www.facebook.com/groups/react.native.community/permalink/940332509435661/ Scroll a lot with and without debug mode on. Make sure spinner still spins and there are no crashes (lots of crashes during development due to the animated configuration being non-monotonic if anything stale values get through). Also made sure that tapping a row to change it's height would properly update the animation configurations so the collision point would still be correct. Reviewed By: yungsters Differential Revision: D4695065 fbshipit-source-id: 855c4e31c8f8b450d32150dbdb2e07f1a9f9f98e

ayc1 · 2017-03-23T16:27:34Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java

-    }
-    view.invalidate();
+  public static @Nullable Integer getViewZIndex(View view) {
+    return mZIndexHash.get(view);


nit: return 0 if null

Right now I'm using null as a way to know that the view doesn't have z-index set so we can avoid using custom view drawing when no child has z-index set.

another option is to change the increment/decrement condition to ReactViewManager.getViewZIndex(view) != 0

ayc1 · 2017-03-23T16:37:01Z

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java

+  }
+
+  @Override
+  protected int getChildDrawingOrder(int childCount, int index) {


one issue with this approach is that getChildDrawingOrder needs to be implemented by each ViewGroup (ScrollView, ModalHostView, any custom ViewGroups) in order to support z index for that specific ViewGroup.

can you add a warning for usages of z index if the parent ViewGroup doesn't support z index? possibly by using an interface

can you extract the add, remove, child drawing order logic into a separate class to make it easier to reuse this logic across other ViewGroups if needed? a unit test for this new class would be useful as well

astreet · 2017-03-23T17:26:37Z

I think @ayc1 is going to handle this review :)

janicduplessis · 2017-03-23T19:35:36Z

@ayc1 I checked both ScrollView and ModalHostView and they all wrap their content inside a View so we don't need to implement view ordering for those. I still moved the logic in a helper class to make it easy to implement for other ViewGroups.

ayc1

That's fair. I can't think of any other ViewGroups that may need this logic at the moment. Thanks for improving z-index!

ayc1 · 2017-03-23T21:48:02Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java

-    }
-    view.invalidate();
+  public static @Nullable Integer getViewZIndex(View view) {
+    return mZIndexHash.get(view);


another option is to change the increment/decrement condition to ReactViewManager.getViewZIndex(view) != 0

facebook-github-bot · 2017-03-23T21:57:46Z

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

nihgwu · 2017-03-28T15:10:05Z

any progress?

janicduplessis · 2017-03-29T04:57:58Z

@achen1 @astreet Could you check why this didn't import?

ayc1

can you fix the test failures?

ayc1 · 2017-04-03T21:02:53Z

ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupDrawingOrderHelper.java

+import android.view.View;
+import android.view.ViewGroup;
+
+import com.facebook.react.views.view.ReactViewManager;


use ViewGroupManager.getViewZIndex instead of ReactViewManager.getViewZIndex to fix dep issue

ayc1 · 2017-04-03T21:03:33Z

Libraries/Components/ScrollView/ScrollView.js

@@ -649,9 +649,6 @@ const ScrollView = React.createClass({
        {...contentSizeChangeProps}
        ref={this._setInnerViewRef}
        style={contentContainerStyle}
-        removeClippedSubviews={
-          hasStickyHeaders && Platform.OS === 'android' ? false : this.props.removeClippedSubviews
-        }


this is causing the clipped subviews tests to fail. is this intended?

This was added in #11315 as a workaround for bugs with the old z-index implementation. Tested that it isn't needed anymore, maybe the test failure in unrelated?

this should be removeClippedSubviews={this.props.removeClippedSubviews} otherwise ScrollView will never remove clipped subviews

You're right, I should have actually checked the diff -_- https://github.com/facebook/react-native/pull/11315/files#diff-f8f8b220fc0d1573e4f8b78c84415075L478

@janicduplessis
stickySectionHeader on ListView is not work as expected after this change on both master branch and 0.45.0-rc.0, while it's OK on SectionList

You can see the sticked sectionHeader is overlapped by the others, works perfectly after reverted

We've actually hit a few issues with sticky sections headers when updating Expo to 0.44. I've fixed then and will make some PRs soon. Hopefuly it also fixes your issue.

@janicduplessis Great, BTW, can you help to review #13885

janicduplessis · 2017-04-04T00:25:11Z

Fixed ReactViewManager -> ViewGroupManager and rebased on last green CI build, let's see if tests pass now.

sahrens · 2017-05-02T18:41:20Z

Looks like this introduced a bug - stickyHeaders seem to no longer be tappable on Android after scrolling down :(

@janicduplessis: any idea what's wrong? Can you fix soon? I haven't been able to figure out a workaround besides not using sticky headers on android when interaction is needed :(

janicduplessis · 2017-05-02T18:46:41Z

@sahrens #13705

Summary: Use `getChildDrawingOrder` instead of reordering views. The old implementation didn't work properly when `removeClippedSubviews` was enabled and this one should have better performance since we don't play with the view hierarchy at all. This fixes weird bugs with sticky headers in `SectionList` and allows removing the hack that disabled `removeClippedSubviews` when using sticky section headers. **Test plan** Tested using the SectionList and ListViewPaging examples that use sticky headers which uses z-index. Closes facebook#13105 Reviewed By: sahrens Differential Revision: D4765869 Pulled By: achen1 fbshipit-source-id: be3c824658a3ce965b6e7324ad95c77cbd8a86ae

@public

Summary: @public This sync includes the following changes: - **[ca0941fce](facebook/react@ca0941fce)**: Add regression test for Placeholder fallbacks with lifecycle methods (#13254) //<Andrew Clark>// - **[a32c727f2](facebook/react@a32c727f2)**: Optimize readContext for Subsequent Reads of All Bits (#13248) //<Sebastian Markbåge>// - **[2b509e2c8](facebook/react@2b509e2c8)**: [Experimental] API for reading context from within any render phase function (#13139) //<Andrew Clark>// - **[5776fa3fc](facebook/react@5776fa3fc)**: Update www warning shim (#13244) //<Dan Abramov>// - **[3d3506d37](facebook/react@3d3506d37)**: Include Modes in the component stack (#13240) //<Dan Abramov>// - **[71b4e9990](facebook/react@71b4e9990)**: [react-test-renderer] Jest matchers for async tests (#13236) //<Andrew Clark>// - **[2c560cb99](facebook/react@2c560cb99)**: Fix unwinding starting with a wrong Fiber on error in the complete phase (#13237) //<Dan Abramov>// - **[ead08827d](facebook/react@ead08827d)**: Add more flexibility in testing errors in begin/complete phases (#13235) //<Dan Abramov>// - **[e4e58343e](facebook/react@e4e58343e)**: Move unstable_yield to main export (#13232) //<Andrew Clark>// - **[0e235bb8f](facebook/react@0e235bb8f)**: Removed unused state argument in unsubscribe method of <Subscription /> (#13233) //<Mateusz Burzyński>// - **[236f60872](facebook/react@236f60872)**: Fail tests if toWarnDev() does not wrap warnings in array (#13227) //<Dan Abramov>// - **[acbb4f93f](facebook/react@acbb4f93f)**: Remove the use of proxies for synthetic events in DEV (#13225) //<Dan Abramov>// - **[171e0b7d4](facebook/react@171e0b7d4)**: Fix “no onChange handler” warning to fire on falsy values ("", 0, false) too (#12628) //<Nicole Levy>// - **[606c30aa5](facebook/react@606c30aa5)**: fixed a typo in commentout in ReactFiberUnwindWork.js (#13172) //<Fumiya Shibusawa>// - **[9f78913b2](facebook/react@9f78913b2)**: Update prettier (#13205) //<Johan Henriksson>// - **[6d3e26288](facebook/react@6d3e26288)**: Remove unnecessary `typeof` checks (#13196) //<jddxf>// - **[82c7ca4cc](facebook/react@82c7ca4cc)**: Add component stacks to some warnings (#13218) //<Dan Abramov>// - **[21ac62c77](facebook/react@21ac62c77)**: Fix a portal unmounting crash for renderers with distinct Instance and Container (#13220) //<Thibault Malbranche>// - **[d6a0626b3](facebook/react@d6a0626b3)**: Set current fiber during before-mutation traversal (#13219) //<Dan Abramov>// - **[f9358c51c](facebook/react@f9358c51c)**: Change warning() to automatically inject the stack, and add warningWithoutStack() as opt-out (#13161) //<Dan Abramov>// - **[467d13910](facebook/react@467d13910)**: Enforce presence or absence of component stack in tests (#13215) //<Dan Abramov>// - **[43ffae2d1](facebook/react@43ffae2d1)**: Suspending inside a constructor outside of strict mode (#13200) //<Andrew Clark>// - **[659a29cec](facebook/react@659a29cec)**: Reorganize how shared internals are accessed (#13201) //<Dan Abramov>// - **[58f3b29d9](facebook/react@58f3b29d9)**: Added SSR/hydration tests for modes, forwardRef, and Profiler (#13195) //<Brian Vaughn>// - **[1c89cb62f](facebook/react@1c89cb62f)**: Use ReactDebugCurrentFrame.getStackAddendum() in element validator (#13198) //<Dan Abramov>// - **[e6076ecf4](facebook/react@e6076ecf4)**: Remove ad-hoc forks of getComponentName() and fix it (#13197) //<Dan Abramov>// - **[32f6f258b](facebook/react@32f6f258b)**: Remove event simulation of onChange events (#13176) //<Philipp Spieß>// - **[9ca37f843](facebook/react@9ca37f843)**: docs: update comments (#13043) //<Sen Yang>// - **[f89f25f47](facebook/react@f89f25f47)**: Correct type of `ref` in forwardRef render() (#13100) //<Moti Zilberman>// - **[7b99ceabe](facebook/react@7b99ceabe)**: Deprecate test utils mock component follow up (#13194) //<Brian Vaughn>// - **[6ebc8f3c0](facebook/react@6ebc8f3c0)**: Add support for re-entrant SSR stacks (#13181) //<Dan Abramov>// - **[d64d1ddb5](facebook/react@d64d1ddb5)**: Deprecate ReactTestUtils.mockComponent() (#13193) //<Brian Vaughn>// - **[e79366d54](facebook/react@e79366d54)**: Link create-subscription doc to GH issue with de-opt explanation (#13187) //<Brian Vaughn>// - **[1f32d3c6d](facebook/react@1f32d3c6d)**: Test renderer flushAll method verifies an array of expected yields (#13174) //<Brian Vaughn>// - **[377e1a049](facebook/react@377e1a049)**: Add a test for SSR stack traces (#13180) //<Dan Abramov>// - **[96d38d178](facebook/react@96d38d178)**: Fix concatenation of null to a warning message (#13166) //<Dan Abramov>// - **[095dd5049](facebook/react@095dd5049)**: Add DEV warning if forwardRef function doesn't use the ref param (#13168) //<Brian Vaughn>// - **[566259567](facebook/react@566259567)**: Refactor stack handling (no functional changes) (#13165) //<Dan Abramov>// - **[ebbd22143](facebook/react@ebbd22143)**: Configure react-test-renderer as a secondary (#13164) //<Brandon Dail>// - **[ddc91af79](facebook/react@ddc91af79)**: Decrease nested update limit from 1000 to 50 (#13163) //<Andrew Clark>// - **[3596e40b3](facebook/react@3596e40b3)**: Fix nested update bug (#13160) //<Andrew Clark>// - **[449f6ddd5](facebook/react@449f6ddd5)**: create a new FeatureFlags file for test renderer on www (#13159) //<Chang Yan>// - **[f762b3abb](facebook/react@f762b3abb)**: Run react-dom SSR import test in jsdom-less environment (#13157) //<Dan Abramov>// - **[6f6b560a6](facebook/react@6f6b560a6)**: Renamed selfBaseTime/treeBaseTime Fiber attributes to selfBaseDuration/treeBaseDuration (#13156) //<Brian Vaughn>// - **[1386ccddd](facebook/react@1386ccddd)**: Fix ReferenceError when requestAnimationFrame isn't defined (#13152) //<Dan Abramov>// - **[f5779bbc1](facebook/react@f5779bbc1)**: Run server rendering test on bundles (#13153) //<Dan Abramov>// - **[9faf389e7](facebook/react@9faf389e7)**: Reset profiler timer correctly after errors (#13123) //<Brian Vaughn>// - **[85fe4ddce](facebook/react@85fe4ddce)**: Fix - issue #12765 / the checked attribute is not initially set on the input (#13114) //<XuMM_12>// - **[07fefe333](facebook/react@07fefe333)**: Drop handling for ms and O prefixes for CSS transition and animation events. (#13133) //<Rouven Weßling>// - **[88d7ed8bf](facebook/react@88d7ed8bf)**: React.Timeout -> React.Placeholder (#13105) //<Andrew Clark>// - **[f128fdea4](facebook/react@f128fdea4)**: Suspending outside of strict trees and async trees (#13098) //<Andrew Clark>// - **[aa8266c4f](facebook/react@aa8266c4f)**: Prepare placeholders before timing out (#13092) //<Andrew Clark>// - **[c039c16f2](facebook/react@c039c16f2)**: Fix this in a functional component for ShallowRenderer (#13144) //<Toru Kobayashi>// - **[64e1921aa](facebook/react@64e1921aa)**: Fix Flow type that event target can be null (#13124) //<Sebastian Markbåge>// - **[6d6de6011](facebook/react@6d6de6011)**: Add PROFILE bundles for www+DOM and fbsource+RN/RF (#13112) //<Brian Vaughn>// - **[71a60ddb1](facebook/react@71a60ddb1)**: Add link to another article about React renderers //<Dan Abramov>// - **[6a530e3ba](facebook/react@6a530e3ba)**: adding check for mousemove (#13090) //<Jason Williams>// - **[c35a1e748](facebook/react@c35a1e748)**: Fix crash during server render in react 16.4.1. (#13088) //<Dustin Masters>// - **[076bbeace](facebook/react@076bbeace)**: Fall back to 'setTimeout' when 'requestAnimationFrame' is not called (#13091) //<Flarnie Marchan>// - **[da5c87bdf](facebook/react@da5c87bdf)**: Fixes children when using dangerouslySetInnerHtml in a selected <option> (#13078) //<Michael Ridgway>// - **[a960d18bc](facebook/react@a960d18bc)**: eliminate unnecessary do-while loop in renderRoot() (#13087) //<Nathan Quarles>// - **[5b3d17a5f](facebook/react@5b3d17a5f)**: setting a flag, so that the first movement will have the correct value (#13082) //<Jason Williams>// - **[b0f60895f](facebook/react@b0f60895f)**: Automatically Profile roots when DevTools is present (#13058) //<Brian Vaughn>// - **[ae8c6dd53](facebook/react@ae8c6dd53)**: remove some redundant lines (#13077) //<Nathan Quarles>// - **[0fcf92d06](facebook/react@0fcf92d06)**: Add a link to custom renderer intro article //<Dan Abramov>// - **[97af3e1f3](facebook/react@97af3e1f3)**: Do not add additional work to a batch that is already rendering (#13072) //<Andrew Clark>// - **[4fe6eec15](facebook/react@4fe6eec15)**: Always batch updates of like priority within the same event (#13071) //<Andrew Clark>// - **[8e87c139b](facebook/react@8e87c139b)**: Remove transitive dependency on fbjs (#13075) //<Dan Abramov>// - **[aeda7b745](facebook/react@aeda7b745)**: Remove fbjs dependency (#13069) //<Dan Abramov>// - **[b1b3acbd6](facebook/react@b1b3acbd6)**: Inline fbjs/lib/emptyObject (#13055) //<Dan Abramov>// Release Notes: [GENERAL] [FEATURE] [React] - React sync for revisions ae14317...ca0941f Reviewed By: bvaughn Differential Revision: D8979192 fbshipit-source-id: 7a14d3b0a253a81d162d7f8c899e99cf6ac4fee4

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Mar 23, 2017

janicduplessis force-pushed the better-zindex-android branch from 9adc807 to f48b474 Compare March 23, 2017 08:04

janicduplessis force-pushed the better-zindex-android branch from f48b474 to 5adc2a4 Compare March 23, 2017 08:07

ayc1 reviewed Mar 23, 2017

View reviewed changes

janicduplessis force-pushed the better-zindex-android branch from 5adc2a4 to 205a7f9 Compare March 23, 2017 19:10

ayc1 approved these changes Mar 23, 2017

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 23, 2017

ayc1 suggested changes Apr 3, 2017

View reviewed changes

janicduplessis added 3 commits April 3, 2017 20:20

Improve z-index implementation on Android

64806c6

Move drawing order login to helper class

a116aed

Add comments

70d3d9e

janicduplessis force-pushed the better-zindex-android branch from b1d7857 to 70d3d9e Compare April 4, 2017 00:20

Use ViewGroupManager instead of ReactViewManager

fc4e614

Fix removeClippedSubviews

b26b236

facebook-github-bot closed this in 9a51fa8 Apr 5, 2017

janicduplessis deleted the better-zindex-android branch May 2, 2017 19:00

nihgwu mentioned this pull request May 16, 2017

stickySectionHeader for ListView is broken on Android #14000

Closed

nihgwu mentioned this pull request May 29, 2017

Upgrade React Native Tab View to v0.0.66 react-navigation/react-navigation#1713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve z-index implementation on Android #13105

Improve z-index implementation on Android #13105

janicduplessis commented Mar 23, 2017

nihgwu commented Mar 23, 2017

janicduplessis commented Mar 23, 2017

ayc1 Mar 23, 2017

janicduplessis Mar 23, 2017

ayc1 Mar 23, 2017

ayc1 Mar 23, 2017

astreet commented Mar 23, 2017

janicduplessis commented Mar 23, 2017

ayc1 left a comment

ayc1 Mar 23, 2017

facebook-github-bot commented Mar 23, 2017

nihgwu commented Mar 28, 2017

janicduplessis commented Mar 29, 2017

ayc1 left a comment

ayc1 Apr 3, 2017

ayc1 Apr 3, 2017

janicduplessis Apr 4, 2017

ayc1 Apr 4, 2017

janicduplessis Apr 4, 2017

nihgwu May 16, 2017 •

edited

Loading

janicduplessis May 16, 2017

nihgwu May 16, 2017

janicduplessis commented Apr 4, 2017

sahrens commented May 2, 2017

janicduplessis commented May 2, 2017

Improve z-index implementation on Android #13105

Improve z-index implementation on Android #13105

Conversation

janicduplessis commented Mar 23, 2017

nihgwu commented Mar 23, 2017

janicduplessis commented Mar 23, 2017

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

astreet commented Mar 23, 2017

janicduplessis commented Mar 23, 2017

ayc1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 23, 2017

nihgwu commented Mar 28, 2017

janicduplessis commented Mar 29, 2017

ayc1 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

nihgwu May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis commented Apr 4, 2017

sahrens commented May 2, 2017

janicduplessis commented May 2, 2017

nihgwu May 16, 2017 •

edited

Loading