fix(Fabric,iOS): header subviews do not support dynamic content changes #2905
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-approach-2
branch
from
a28fe82 to
22e632b
Compare
refactor (method naming)
kkafar
changed the title
Member
Author
|
Caution This PR was an exploration of alternative to #2845 approach of solving #2714. The approach here was to simply do not call Note However, I might be able to fix this by sending navigation bar insets. Currently there is no padding in shadow node set & Yoga probably believes that it has more space than there is in reality.
|
9 tasks
kkafar
commented
May 7, 2025
Comment on lines
+25
to
+43
| RNSScreenStackHeaderConfigState(const RNSScreenStackHeaderConfigState &source) | ||
| : frameSize{source.frameSize}, edgeInsets{source.edgeInsets} {} | ||
| RNSScreenStackHeaderConfigState &operator=( | ||
| const RNSScreenStackHeaderConfigState &source) { | ||
| this->frameSize.width = source.frameSize.width; | ||
| this->frameSize.height = source.frameSize.height; | ||
| this->edgeInsets = source.edgeInsets; | ||
| return *this; | ||
| } | ||
|
|
||
| bool operator==(const RNSScreenStackHeaderConfigState &other) { | ||
| return this->frameSize == other.frameSize && | ||
| this->edgeInsets == other.edgeInsets; | ||
| } | ||
|
|
||
| bool operator!=(const RNSScreenStackHeaderConfigState &other) { | ||
| return this->frameSize != other.frameSize || | ||
| this->edgeInsets != other.edgeInsets; | ||
| } |
Member
Author
There was a problem hiding this comment.
Making it copyable simplifies working with this type in HostTree (component implementation)
Comment on lines
+247
to
+265
| - (NSDirectionalEdgeInsets)computeEdgeInsetsOfNavigationBar:(nonnull UINavigationBar *)navigationBar | ||
| { | ||
| NSDirectionalEdgeInsets navBarMargins = [navigationBar directionalLayoutMargins]; | ||
| NSDirectionalEdgeInsets navBarContentMargins = [navigationBar.rnscreens_findContentView directionalLayoutMargins]; | ||
|
|
||
| BOOL isDisplayingBackButton = [self shouldBackButtonBeVisibleInNavigationBar:navigationBar]; | ||
|
|
||
| // 44.0 is just "closed eyes default". It is so on device I've tested with, nothing more. | ||
| UIView *barButtonView = isDisplayingBackButton ? navigationBar.rnscreens_findBackButtonWrapperView : nil; | ||
| CGFloat platformBackButtonWidth = barButtonView != nil ? barButtonView.frame.size.width : 44.0f; | ||
|
|
||
| const auto edgeInsets = NSDirectionalEdgeInsets{ | ||
| .leading = | ||
| navBarMargins.leading + navBarContentMargins.leading + (isDisplayingBackButton ? platformBackButtonWidth : 0), | ||
| .trailing = navBarMargins.trailing + navBarContentMargins.trailing, | ||
| }; | ||
|
|
||
| return edgeInsets; | ||
| } |
Member
Author
There was a problem hiding this comment.
This is so so. It works for now, but it's vulnerable to UIKit changes.
Member
Author
|
In case we ever find a use case that |
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
Regression most likely introduced in 4.5.0 by #2466.
Fixes #2714 on iOS
Fixes #2815 on iOS
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:
RNSScreenStackHeaderSubviewShadowNode,Added
Test2714Most of the fragile header interactions must be tested:
TestHeaderTitle❌ This PR introduces regression here on iOS (Android not tested yet)✅ WorksTest2466(iOS works, Android code is unmodified here)setOptions#2812 (this PR does not touch Android)Test2714handling header item resizing.Checklist