Improve z-index implementation on iOS #14011
Conversation
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
cc @shergin |
|
Oh, that's really great! I like this simplification! Let's me review this carefully! |
React/Views/RCTView.m
Outdated
| } else { | ||
| // Ensure sorting is stable by treating equal zIndex as ascending so | ||
| // that original order is preserved. | ||
| return NSOrderedAscending; |
React/Views/RCTViewManager.m
Outdated
| @@ -255,7 +255,15 @@ - (RCTViewManagerUIBlock)uiBlockToAmendWithShadowViewRegistry:(__unused NSDictio | |||
| RCT_VIEW_BORDER_RADIUS_PROPERTY(BottomLeft) | |||
| RCT_VIEW_BORDER_RADIUS_PROPERTY(BottomRight) | |||
|
|
|||
| RCT_REMAP_VIEW_PROPERTY(zIndex, reactZIndex, NSInteger) | |||
| RCT_CUSTOM_VIEW_PROPERTY(zIndex, NSInteger, RCTView) | |||
There was a problem hiding this comment.
I believe we have to move this logic to RCTView and UIView+React introducing new category method reactZIndex. (Where we also have to reset the cache for superview... So we also have to have something like reactInvalidateZIndex.)
React/Views/RCTView.m
Outdated
| break; | ||
| } | ||
| } | ||
| NSArray<UIView *> *sortedSubviews = sortingRequired ? [self.reactSubviews sortedArrayUsingComparator:^NSComparisonResult(UIView *a, UIView *b) { |
There was a problem hiding this comment.
As you know, hitTest must be incredibly performant, so I believe we have to decouple this logic and introduce caching.
I think we can implement at least two cache invalidation strategies:
- Use weak references and invalidate inside something like
setNeedsLayout; - Use strong NSArray and destroy it in all
insert/add/remove/Subviewmethods family (I personally prefer this one even if it requires overriding half of dozen methods).
There was a problem hiding this comment.
Yea I held off doing caching to keep this simpler but I do agree it might be needed. I also think the second solution is ideal and is also similar to what we do on Android.
|
I think twice, I am also okey with another strategy: |
|
@shergin With the current code we only sort if there is a view with a z-index set, although we loop through all subviews to check that. |
|
I think one more time, and I am generally okay with this approach. :) I prefer simplicity over premature optimization; @janicduplessis Do you mind:
|
|
@shergin Sounds good |
janicduplessis
force-pushed
the
z-index-ios
branch
from
f542b3f
to
a7ad964
Compare
|
Moved most of the implementation to UIView+React, will make it easy if any other view ends up having to do something special related to z-index. |
|
@janicduplessis That's great! Thank you so much for your effort! |
facebook-github-bot
added
GH Review: accepted
Import Started
|
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
facebook-github-bot
added
Import Failed
and removed
Import Started
|
@janicduplessis Landing of this diff is blocked by failed tests... and the test is failing because of this: Do you have any idea how to fix that? |
|
Yeah... Seems switching to |
|
@shergin Yea I was thinking we could do that too after reading the SO post. |
…shot tests Summary: We found that `-[CALayer renderInContext:]` produces bad results in some cases (which is actually documented thing!), so we decided to replace it with `-[UIView drawViewHierarchyInRect:]` which is more reliable (I hope). As part of this change I completly removed support of `CALayer` from local fork of `RNTesterIntegrationTests`. See #14011 (comment) for more details. janicduplessis Reviewed By: javache Differential Revision: D5129492 fbshipit-source-id: 6a9227037c85bb8f862d55267f5301e177985ad9
|
So, I fixed snapshot infra 🎉 and I will try to land this PR on Monday. |
…shot tests Summary: We found that `-[CALayer renderInContext:]` produces bad results in some cases (which is actually documented thing!), so we decided to replace it with `-[UIView drawViewHierarchyInRect:]` which is more reliable (I hope). As part of this change I completly removed support of `CALayer` from local fork of `RNTesterIntegrationTests`. See facebook/react-native#14011 (comment) for more details. janicduplessis Reviewed By: javache Differential Revision: D5129492 fbshipit-source-id: 6a9227037c85bb8f862d55267f5301e177985ad9
Summary: This was leftovers from old implementation of zIndex feature. Janic janicduplessis refactored this and moved all logic to UIView layer, so we don't need this prop anymore in shadow realm. More info: #14011 Reviewed By: mmmulani Differential Revision: D6574414 fbshipit-source-id: 2cae19350765689784d7884ed875878d39b4e3f1
Summary: This was leftovers from old implementation of zIndex feature. Janic janicduplessis refactored this and moved all logic to UIView layer, so we don't need this prop anymore in shadow realm. More info: facebook#14011 Reviewed By: mmmulani Differential Revision: D6574414 fbshipit-source-id: 2cae19350765689784d7884ed875878d39b4e3f1
This avoids reordering views because it created some bugs when the native hierarchy is different from the shadow views. This leverages
layer.zPositionand takes z-index in consideration when we check what view should be the target of a touch.Test plan
Tested that this fixes some layout issues that occurred when using sticky headers in the Expo home screen.