fix(Fabric,iOS): header subviews do not support dynamic content changes #2845
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
force-pushed
the
@kkafar/2714-headerright-sizing
branch
from
da44380 to
c5b8705
Compare
kkafar
changed the title
kkafar
commented
Apr 14, 2025
Comment on lines
69
to
77
| if (stateData.frameSize != mostRecentStateData.frameSize) { | ||
| std::printf("SubviewCD [%d] adopt APPLY\n", shadowNode.getTag()); | ||
| layoutableShadowNode.setSize(stateData.frameSize); | ||
| } else { | ||
| // If the state has not changed we assign undefined size, to allow Yoga | ||
| // to recompute the shadow node layout. | ||
| std::printf("SubviewCD [%d] adopt ZERO\n", shadowNode.getTag()); | ||
| layoutableShadowNode.setSize({YGUndefined, YGUndefined}); | ||
| } |
Member
Author
There was a problem hiding this comment.
This causes regression in header truncation (TestHeaderTitle).
Imagine case with two header subviews present in the navigation bar. Now, when one updates, the other receives YGUndefined values as its size (as it's state does not change), effectively ruining header subview sizing.
kkafar
force-pushed
the
@kkafar/2714-headerright-sizing
branch
from
caa21d1 to
93531f5
Compare
kkafar
force-pushed
the
@kkafar/2714-headerright-sizing
branch
from
93531f5 to
041b982
Compare
Warning: there is still issue on second screen, when you toggle all the buttons at once using provided button. This is a big commit & it needs to be explained. Besides some cleanup it contains following changes: 1. Now `RNSScreenStackHeaderSubviewShadowNode` layoutmetrics.frame is enforced to be the same as frame of its subview. Thinking of this now, I believe that it defeats the purpose of calling `shadowNode.setSize` at all, because I won't use the size provided in state anyway? <-- This is only partially true, because setting the size will impact the frame of the subview -> the interaction here is a bit more subtle, however the most important question is: do I really need to set size on `RNSScreenStackHeaderSubviewShadowNode`? Is this necessary? 2. `-[RNSHeaderSubviewContentWrapper updateLayoutMetrics:oldLayoutMetrics:]` now enforces origin of the view to be `(0, 0)`. This is because, when it's size changes due to JS-initiated render & `RNSScreenStackHeaderSubviewShadowNode.setSize` has been called, the first resulting transaction will contain udpated content size, but not `RNSScreenStackHeaderSubviewShadowNode` -> causing `RNSHeaderSubviewContentWrapper` to have incorrect origin, causing flicker (tested on iOS).
9 tasks
Member
Author
|
Superseded by #2905 |
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
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
Regression most likely introduced in 4.5.0 by #2466.
Fixes #2714
Fixes #2815
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
Most of the fragile header interactions must be tested:
TestHeaderTitle❌ This PR introduces regression here on iOS (Android not tested yet)Test2466(iOS works, Android code is unmodified here)setOptions#2812 (this PR does not touch Android)Test2714handling header item resizing.Checklist