-
-
Notifications
You must be signed in to change notification settings - Fork 576
fix(Android,Fabric): prevent header subview disappearance when using setOptions
#2812
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
Conversation
When setting subviews via `setOptions` from `useEffect` hook in a component, the first frame received might be computed by native layout & completely invalid (zero height). RN layout is the source of a subview **size** (not origin). When we send such update with zero height Yoga might (or might not, depending on exact update timing in relation to other ongoing commits / layouts) set the subview height to 0! This causes the subview to become invisible & we want to avoid that. This had not been a problem before #2696, because we would filter out this kind of frame in the `setSize` guard in the `ComponentDescriptor.adopt` method of the `HeaderSubview`. #2696 allowed for zero-sized frames for other, unrelated reason & we must allow these as long as they come from React Native layout & not native one.
2969fbb
to
7d3205e
Compare
@sbeigel I think this should be the final PR to solve these problems |
Yes, it works! Thank you! I initially commented that the header size was wrong but I only applied your latest change and reverted the previous ones (RNSScreenStackHeaderConfigComponentDescriptor.h and RNSScreenStackHeaderSubviewComponentDescriptor.h). I deleted these comments. I have now applied all three changes against RNS 4.9.2 and it really seems to work. I've only tested on the emulator in dev mode though. We will check the release build on real devices! Thank you again! |
Thanks! Let me know in case you find any issues. I'm about to release stable 4.10.0 & this was the last blocker. |
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.
👍
…es (#2905) ## Description Regression most likely introduced in 4.5.0 by #2466. Fixes #2714 Fixes #2815 Supersedes #2845 This is a ugly hack to workaround issue with dynamic content change. When the size of this shadow node contents (children) change due to JS update, e.g. new icon is added, if the size is set for the yogaNode corresponding to this shadow node, the enforced size will be used and the size won't be updated by Yoga to reflect the contents size change -> host tree won't get layout metrics update -> we won't trigger native layout -> the views in header will be positioned incorrectly. > [!important] > This PR handles iOS only, however there is **similar** issue on Android. The issue can be reproduced on the same test example. Android will be handled in separate PR. ## Changes ## Test code and steps to reproduce In this approach I've settled with: 1. not calling set size on iOS for `RNSScreenStackHeaderSubviewShadowNode`, 2. updating header config padding & sending it as state to shadow tree. Added `Test2714` Most of the fragile header interactions must be tested: * [x] Header title truncation - `TestHeaderTitle` ~❌ This PR introduces regression here on iOS (Android not tested yet)~ ✅ Works * [x] Pressables in header - `Test2466` (iOS works, Android code is unmodified here) * [x] #2807 (this PR does not touch Android) * [x] #2811 (this PR does not touch Android) * [x] #2812 (this PR does not touch Android) * [x] Header behaviour on orientation changes - #2756 (this PR does not touch Android) * [x] New test `Test2714` handling header item resizing. ## Checklist - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes
…hanges (#2910) ## Description Fixes #2714 on Android Fixes #2815 on Android See #2905 for detailed description. ## Changes Removed call to `RNSScreenStaceaderSubviewShadowNode.setSize` in corresponding component descriptor. It seems that we do not need to enforce node size from HostTree. Setting appropriate content offset is enough for pressables to function correctly (assuming that native layout **does not change size of any subview**). I currently can't come up with any scenario where this would happen. ## Test code and steps to reproduce I've tested: * [x] `Test2714` introduced in PR with iOS fixes - #2905 * [x] Pressables in header - #2466, * [x] Header title truncation - #2325 (only few cases, as the list is long there) & noticed a regression (not related to this PR, described in comment below the PR description), * [x] Insets with orientation change (Android) - #2756 * [x] #2807 (on `TestHeaderTitle` with call to `setOptions` in `useLayoutEffect`) * [x] `Test2811` - #2811 * [x] #2812 (with snippet provided in that PR description) ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes
Description
When setting subviews via
setOptions
fromuseEffect
hook in acomponent, the first frame received might be computed by native
layout & completely invalid (zero height). RN layout is the source of a
subview size (not origin). When we send such update with zero height
Yoga might (or might not, depending on exact update timing in relation
to other ongoing commits / layouts) set the subview height to 0!
This causes the subview to become invisible & we want to avoid that.
This had not been a problem before #2696, because we would filter out
this kind of frame in the
setSize
guard in theComponentDescriptor.adopt
method of theHeaderSubview
. #2696 allowedfor zero-sized frames for other, unrelated reason & we must allow these
as long as they come from React Native layout & not native one.
Changes
We now filter these invalid frames on the side of HostTree, by detecting whether React has measured the subview or not.
Test code and steps to reproduce
I've tested the problem on slightly modified
Test2466
:Code snippet
Checklist