Fix updating a view z-index on Android #15203
Conversation
facebook-github-bot
added
CLA Signed
|
This PR has been submitted by a core contributor. Generated by 🚫 dangerJS |
|
Thank you sir! I will test this tonight! :) |
|
I never tested something like this, but I tried my hand. I saw the 4 files you changed, and made those files changes in my node_modules folder. Then I re-ran It's a very simple repo, may you please try with this commit - https://github.com/Noitidart/Qaida-Board/tree/3ec2502c3682073fc43c8738a7849d09e867674f/App I am using react native 0.46.1 + the edits I saw in your commit here. |
|
Testing on Android is a bit more complex because you need to build the project from source, see http://facebook.github.io/react-native/docs/android-building-from-source.html. |
|
I can test with the project you linked tomorrow if you don't get it working, I know building from source can be a pain :) |
|
Oh thanks, I'll try this now and let you know how it goes. :) |
|
+1 for this |
|
My sincerest apologies sir, @janicduplessis - I was not able to figure this testing process out. I'm feeling like a fool right now, it seems the instructions are very good, I just am screwing up somewhere. :( |
|
I did take a screencast of it being bad vs good though the other day. On Android it works like this for me: But it should work like this: Recording of iOS - https://youtu.be/3cwRMAnB45A |
| * Redraw the view based on updated z-index. This should be called after updating one of it's child | ||
| * z-index. | ||
| */ | ||
| void updateZIndex(); |
There was a problem hiding this comment.
I think that name of this method is a bit too short. Maybe updateViewParentZIndex() would be better as I suppose this method belongs to parent not child.
There was a problem hiding this comment.
Yea I had a hard time figuring out a name. updateViewParentZIndex would not work in this case because it doesn't update the parent, it redraws the view based on the updated z-index of its children.
| @@ -12,4 +12,10 @@ | |||
| * @return The child view index considering z-index | |||
| */ | |||
| int getZIndexMappedChildIndex(int index); | |||
|
|
|||
| /** | |||
| * Redraw the view based on updated z-index. This should be called after updating one of it's child | |||
There was a problem hiding this comment.
"it's child z-index" -> "its child z-indexes"
|
Passing on to Andrew Chen for review. |
facebook-github-bot
added
the
Import Started
|
@achen1 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@janicduplessis were you able to test with the demo commit I gave? I am very eager to hear if that fixed it please sir. I don't think there is an error on my side because it worked as expected on iOS and even Windows (UWP). |
facebook-github-bot
added
GH Review: accepted
and removed
GH Review: review-needed
labels
|
@achen1 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@janicduplessis curious how this is going? Doesn't seem resolved yet? |
|
@rtman this PR has been merged and the fix should be on master. Please confirm that you're not seeing this resolved on master. |
Summary: If the z-index was updated after the initial mount, changes would not be reflected because we did not recalculate the z-index mapped child views and redraw the view. This adds code to do that and call it whenever we update z-index. **Test plan** Tested by reproducing the bug with 2 overlapping views that change z-index every second. Made sure it now works properly and z-index changes are reflected. Closes #15203 Differential Revision: D5564832 Pulled By: achen1 fbshipit-source-id: 5b6c20147211ce0b7e8954d60f8614eafe128fb4
|
Thanks @hramos is there some steps for novices to follow? I followed the previous guide and failed miserably. |
|
I cherry picked this into 0.48 as well. It should be easier to test that way. When 0.48-rc.2 gets published (no ETA though) it will include this change. |
|
@rtman, this is not in 0.48-rc.1 but if rc.2 is published it will be there. |
|
@ide ok thanks! |
|
Just tested out0.49 with my zIndex example above and it works, yahoo! Thanks! I had an animated view, the animation was janky but this might be because it was not a production build and I had console.log's. |
|
Yea it works for me on 0.49.3! Sent from my Google Nexus 5 using FastHub |
If the z-index was updated after the initial mount, changes would not be reflected because we did not recalculate the z-index mapped child views and redraw the view. This adds code to do that and call it whenever we update z-index.
Test plan
Tested by reproducing the bug with 2 overlapping views that change z-index every second. Made sure it now works properly and z-index changes are reflected.