Stop applying zPosition for views #42021
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D52181701 |
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Dec 20, 2023
Summary: # Context A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72 where the rotating view would clip into the background. There are a few issues here. * The current zIndex code is designed to be cross platform on the C++ layer and not the native layer. This diverges from the Android behavior and adds a special case for this platform when we have the ability to share all of this logic * Static nodes will apply a zIndex when they shouldn't. This code pre-empts the static check [here](https://www.internalfb.com/code/fbsource/[cf8ad268b4cf]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view/ConcreteViewShadowNode.h?lines=98) that zeroes out the "order index" for static nodes * As a result of the above, static nodes can eclipse their children which should never happen because static nodes ignore zIndex values # Reason for the clipping The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418} This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied. So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition. # This change The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved! Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52181701
Base commit: 0c37f8c |
joevilches
force-pushed
the
export-D52181701
branch
from
9eb9750 to
633f522
Compare
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Dec 20, 2023
Summary: # Context A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72 where the rotating view would clip into the background. There are a few issues here. * The current zIndex code is designed to be cross platform on the C++ layer and not the native layer. This diverges from the Android behavior and adds a special case for this platform when we have the ability to share all of this logic * Static nodes will apply a zIndex when they shouldn't. This code pre-empts the static check [here](https://www.internalfb.com/code/fbsource/[cf8ad268b4cf]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view/ConcreteViewShadowNode.h?lines=98) that zeroes out the "order index" for static nodes * As a result of the above, static nodes can eclipse their children which should never happen because static nodes ignore zIndex values # Reason for the clipping The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418} This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied. So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition. # This change The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved! Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52181701
|
This pull request was exported from Phabricator. Differential Revision: D52181701 |
joevilches
force-pushed
the
export-D52181701
branch
from
633f522 to
4deb9d5
Compare
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Dec 26, 2023
Summary: # Context A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72 where the rotating view would clip into the background. There are a few issues here. * The current zIndex code is designed to be cross platform on the C++ layer and not the native layer. This diverges from the Android behavior and adds a special case for this platform when we have the ability to share all of this logic * Static nodes will apply a zIndex when they shouldn't. This code pre-empts the static check [here](https://www.internalfb.com/code/fbsource/[cf8ad268b4cf]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view/ConcreteViewShadowNode.h?lines=98) that zeroes out the "order index" for static nodes * As a result of the above, static nodes can eclipse their children which should never happen because static nodes ignore zIndex values # Reason for the clipping The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418} This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied. So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition. # This change The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved! Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52181701
|
This pull request was exported from Phabricator. Differential Revision: D52181701 |
joevilches
force-pushed
the
export-D52181701
branch
from
4deb9d5 to
7c5ad34
Compare
joevilches
added a commit
to joevilches/react-native
that referenced
this pull request
Jan 3, 2024
Summary: # Context A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72 where the rotating view would clip into the background. There are a few issues here. * The current zIndex code is designed to be cross platform on the C++ layer and not the native layer. This diverges from the Android behavior and adds a special case for this platform when we have the ability to share all of this logic * Static nodes will apply a zIndex when they shouldn't. This code pre-empts the static check [here](https://www.internalfb.com/code/fbsource/[cf8ad268b4cf]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view/ConcreteViewShadowNode.h?lines=98) that zeroes out the "order index" for static nodes * As a result of the above, static nodes can eclipse their children which should never happen because static nodes ignore zIndex values # Reason for the clipping The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418} This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied. So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition. # This change The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved! Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52181701
|
This pull request was exported from Phabricator. Differential Revision: D52181701 |
…acebook#42064) Summary: # Context Dealing with how we should stack Views in a world with static is actually a bit complicated, despite the fact that static cannot get zIndex. The outline of everything can be found in this spec: https://www.w3.org/TR/CSS21/zindex.html. That is a bit dense, but mainly because there is much more functionality to consider in the browser than we need to in RN (like block layout, inline, floats, etc). Basically it comes down to this order, with larger numbers stacked on top of smaller ones: 1) Root 2) Negative zIndex positioned nodes (ties in document order) 3) Non positioned nodes in document order 4) Positioned nodes with no zIndex (or 0) in document order* 5) Positioned nodes with positive zIndex (ties in document order) Document order is a pre-order traversal of the tree. The asterisk on 4 is where it gets a bit complicated. Even though there is no zIndex set, you should still treat these nodes as if they form a stacking context - with their descendants stacked relative to the parent stacking context like normal if they have zIndex set. Essentially what this means in our world is that a static child will not come before (under) a positioned parent if they share the same stacking context. Without this, you would need to form a stacking context on all positioned nodes with static children if you wanted them to appear at all since static comes before (under) positioned nodes. # Implementation Implementing this was a bit tricky. We had to go in pre-order traversal of the tree, but if we see a relative node it needs to form a "pseudo-stacking-context" to allow static to show over it, but if any of those descendants have a zIndex set, that should be relative to the parent stacking context like normal, not this "pseudo-stacking-context". Without static we take care of this just fine because it just ends up being a pre-order traversal of the tree, then sort by zIndex. My approach was to ignore zIndex's at first and gather all nodes that share the same stacking context in an array as if none of them had any zIndex set. After that was gathered, then sort by zIndex to get the final order for said stacking context. The second part was already implemented. For the first part I just did a pre-order traversal of the tree (stopping at nodes that form stacking contexts) and: * If a node was non static, add to the end of the list * If a node was static, insert into the list right after the previously inserted static node that shares the same "psuedo-stacking-context", or the parent if there were none. * Since inserting into arrays is slow, I opted to use `std::list` In effect this was a pre-order traversal where static nodes are "sorted" to come first in the list of children, which is what we want since these nodes should come under non-positioned nodes that have the same "pseudo-stacking-context". My implementation is a bit slower. The previous one just visits all nodes once with a pre-order traversal. This will also do that and thanks to `std::list`, we have no non-constant time operations while coming in that order. After the fact, however, we need to convert back to `std::vector` which is the type used across the mounting layer, so we have to iterate over this list again. I think this is the fastest we can do this and it is optimized for a world where most nodes are relative (since it will be the new default). Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52103057
Summary: # Context A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72 where the rotating view would clip into the background. There are a few issues here. * The current zIndex code is designed to be cross platform on the C++ layer and not the native layer. This diverges from the Android behavior and adds a special case for this platform when we have the ability to share all of this logic * Static nodes will apply a zIndex when they shouldn't. This code pre-empts the static check [here](https://www.internalfb.com/code/fbsource/[cf8ad268b4cf]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view/ConcreteViewShadowNode.h?lines=98) that zeroes out the "order index" for static nodes * As a result of the above, static nodes can eclipse their children which should never happen because static nodes ignore zIndex values # Reason for the clipping The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418} This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied. So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition. # This change The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved! Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52181701
joevilches
force-pushed
the
export-D52181701
branch
from
7c5ad34 to
a840698
Compare
|
This pull request was exported from Phabricator. Differential Revision: D52181701 |
|
This pull request has been merged in 1838c16. |
Othinn
pushed a commit
to Othinn/react-native
that referenced
this pull request
Jan 9, 2024
Summary: Pull Request resolved: facebook#42021 # Context A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72 where the rotating view would clip into the background. There are a few issues here. * The current zIndex code is designed to be cross platform on the C++ layer and not the native layer. This diverges from the Android behavior and adds a special case for this platform when we have the ability to share all of this logic * Static nodes will apply a zIndex when they shouldn't. This code pre-empts the static check [here](https://www.internalfb.com/code/fbsource/[cf8ad268b4cf]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/renderer/components/view/ConcreteViewShadowNode.h?lines=98) that zeroes out the "order index" for static nodes * As a result of the above, static nodes can eclipse their children which should never happen because static nodes ignore zIndex values # Reason for the clipping The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418} This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied. So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition. # This change The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved! Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D52181701 fbshipit-source-id: 580f860273b9c8470181d92d7ad542546664ed77
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Context
A while ago D48905010 was committed that set the CALayer's zPosition equal to the zIndex passed in via props. This was to avoid an issue like: https://pxl.cl/40F72
where the rotating view would clip into the background.
There are a few issues here.
Reason for the clipping
The reason this clipping is happening is because the red/blue views share the same stacking context as the white background (as indicated by the vertical black line). {F1175070418}
This in combination with the fact that our zIndex implementation will NOT set iOS's zPosition means that these three views (red view, blue view, white background view) all have the same zPosition (0) and will be laid out in the order described by the orderIndex mentioned earlier. This index essentially just changes the document order that is used for tiebreakers when zPosition is tied.
So, all the views are on the same stacking context and they all have no zPosition set. Add the rotation that the colored views are doing and you get this clipping. Apple will change the "zPosition" in a sense for the parts of the view that should be perceived as "further away" due to the rotation. So, we clip into our background which has a lower order index but the same zPosition.
This change
The fix here just makes it so that the rotating views are not on the same stacking context as the background so the changing zPosition from the rotation does not matter. This can be achieved by setting the zIndex of the container to any number (among other things). Note that this is only the case because the default position type is relative in this stack. Otherwise you would also need to set the position type as well. Now the stacking context looks like: {F1175083828} and the problem is solved!
Changelog: [Internal]
Reviewed By: NickGerleman
Differential Revision: D52181701