fix(Android,Fabric): prevent header subviews from disappearing after hot-reload #2807
Conversation
This file contains hidden or 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
kkafar
changed the title
9 tasks
9 tasks
kkafar
added a commit
that referenced
this pull request
May 7, 2025
…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
9 tasks
kkafar
added a commit
that referenced
this pull request
May 8, 2025
…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
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.
Description
Exactly what the title says.
Currently if we have e.g. a headerRight & it is mounted in useEffect or
useLayoutEffectit'll disappear after hot-reload.This might also improve header flickering problems (haven't been able to reproduce myself).
The mounted state has been mistakenly used instead of current node state. This led to situations where we receive new (non-zero) state from HT, however we do not set it, because the mounted state is still zeroed (initial).
Changes
We now take current node state into account.
Test code and steps to reproduce
e.g.
TestHeaderTitle& mount the header component in useEffect.Checklist