fix(Android): adapt header insets when interface orientation changes #2756
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
|
There is stil issue when the app start s in landscape mode -> the header font is weirdly small, but this is not a regression and a separate problem. I'll tackle it in separate PR.
|
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
We did not adapt header insets when interface orientation changes, leading to cases like on screenshots below 👇🏻
We want it to adapt to changing window insets properly.
What needs to be tested / answered:
this might lead to issues with pressables- checked onTest2466in e2e and not e2e - seems to work okTestHeaderTitle- seems to work ok (tested both e2e and not e2e & in any screen orientation)This is how it works now (look only at header):
SDK 34 (no edge-to-edge) + no cutout
Screen.Recording.2025-03-07.at.14.39.14.mov
SDK 34 (no edge-to-edge) + cutout
Screen.Recording.2025-03-07.at.14.41.50.mov
SDK 35 (edge-to-edge) + no cutout
Screen.Recording.2025-03-07.at.14.45.21.mov
SDK 35 (edge-to-edge) + cutout
Screen.Recording.2025-03-07.at.14.46.16.mov
Let it be noted that I've also tested that it work's fine with button navigation.
Changes
I've moved all inset/padding related logic to
CustomToolbar, because it is in different place in view hierarchy thanHeaderConfig& the insets it receives are sometimes different & I wanted the logic manipulatingCustomToolbarlayout to live inCustomToolbar.CustomToolbarnow listens for dispatches of window insets & updates its horizontal paddings accordingly.Currently I've decided that this behaviour will be controlled by
isTopInsetEnabledprop onHeaderConfigtogether with the topInset.Test code and steps to reproduce
TestFormSheetChecklist