-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Improve z-index implementation on iOS #14011
Conversation
cc @shergin |
Oh, that's really great! I like this simplification! Let's me review this carefully! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to be super confident in hitTest
performance.
React/Views/RCTView.m
Outdated
} else { | ||
// Ensure sorting is stable by treating equal zIndex as ascending so | ||
// that original order is preserved. | ||
return NSOrderedAscending; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/Subview
methods family (I personally prefer this one even if it requires overriding half of dozen methods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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! |
@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. |
@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.zPosition
and 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.