Repository navigation
Conversation
|
spm_newapp in release is red. Can you check whether it is a flakyness issue or a real problem? |
|
@cipolleschi Reran ✅ |
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for tackling this, it's a real pain point and the before/after screenshots are great. The native event is well designed: it's deduped, it re-arms on prop set, and the synchronous render avoids a first-frame flash. My concerns are all in how the extra padding gets merged into contentContainerStyle. In a few common setups it drops either the user's padding or the safe-area padding. I left inline comments.
Since this changes layout for every iOS ScrollView using automatic, scrollableAxes or always, and you already flag landscape apps as a possible breakage, could we put it behind a ReactNativeFeatureFlags flag so we can roll it out gradually?
@cipolleschi The aim is we ship this for Expo end users ASAP. Additionally we (currently) have no API for overriding individual flags from Expo in order to test them out in our framework. Latter is solvable, but likely not in this timeframe. The edge case in which this affects users:
I'm trying to balance the new users compiling in Xcode 27.1 today and suffering from this, vs the theoretically tiny cases where |
|
@huntie Fair enough, I'm fine shipping this without a feature flag given the Expo timeline and the lack of a per-flag override. The inline comments still need to be addressed before we merge, though. They're not about the new landscape behavior; they're cases where the padding merge regresses apps that never opted into anything:
Because the override is emitted even when the cross-axis insets are 0, these hit every iPhone in portrait using The other inline comments (re-renders, stale insets with a user handler, edge cases) are lower priority. If they'd slow this PR down, could you open them as a follow-up? |
|
@cipolleschi Thanks, addressed and re-tested(!) in the latest push:
The padding and RTL additions are sizeable, but thankfully this base Lower-pri comments are also fixed in the same push. |
|
Thanks for the quick turnaround, most of the previous round is addressed. One blocker remains: The
The current tests miss this: the Jest test checks the flattened style, and the Fantom test only covers the first render in LTR, where Question: under Non-blocking follow-ups: these two can land in a separate PR, could you own them?
|
d5d4ff6 to
8eb2473
Compare
|
@cipolleschi Both addressed in the latest push. 1/ 2/ Navigation stack nesting Yup, this was a problem, and this went further than On its own, UIKit leaves a horizontal scroll view in a navigation stack alone. The scroll view now removes whatever UIKit adds across the scrollable axis itself, and keeps content pinned to the leading edge. This replaces the Verified in RNTester with its root hosted in a
Follow-ups
|
|
Thanks, both are addressed. The One interaction to check before we merge: Minor: scroll events report |
Visual regressionNo visual differences at ios: no capture |
…al safe area (iPhone Duo)
|
@cipolleschi Feedback addressed. Sorry this makes the overall logic quite heavy now, but all states are tested. I'll see in follow up PRs what we could do to refactor maybe. |


Summary
<ScrollView>'scontentInsetAdjustmentBehaviorprop provides built-in safe area handling today on the vertical axis (e.g. above the home indicator), however is missing safe area handling on the cross axis (i.e. iPhone Duo vertical rail).Since #58109 (
experimental_onSafeAreaInsetsChange), we now have the API to conveniently patch this behaviour in core.From the user's perspective
This presents a very frequent code authoring issue.
Typical ScrollView code at the top of a layout today (e.g. app screen in Expo Router):
Without this diff, needs to become:
⬆️
contentInsetAdjustmentBehavior="automatic"isn't doing its job on the horizontal safe areas on a device such as iPhone Duo.With this diff the scroll view handles it alone:
automatic)And the code sample becomes resolved:
ScrollView-only hierarchy — backwards compatible ✅Why this is a bug
Remember that:
<ScrollView contentInsetAdjustmentBehavior="automatic">already handles the bottom safe area inset on relevant devices.And again, with iOS 27.1 and iPhone Duo, a typical React Native screen with a
<ScrollView>(e.g. in Expo Router) that handled safe area before, does not work now. This is a combination of:"automatic"should be automatic. By fixing this we delete the problem for all RN users.Changes
ScrollViewobserves its own safe area when the behavior is not'never'and pads the content container across the scrollable axis, on top of the padding its style resolves to on that edge.'always', on an axis that always bounces, and when a navigation stack registers the scroll view as its content scroll view, as react-native-screens does for the first scroll view of a screen. ThecontentInsetprop stays as given along the scrollable axis.horizontalprop reaches the native view on iOS, as it already did on Android, so the scroll view knows which axis it scrolls along.Impact
At the same time, if anyone was handling this before with a nested SafeAreaView, they play nicely and is backwards compatible.
Notes
Changelog:
[iOS][Fixed] - ScrollView:
contentInsetAdjustmentBehaviorkeeps content clear of the safe area across the scrollable axis, such as the rail on iPhone Duo and the notch in landscapeTest Plan
RNTester on an iPhone Duo simulator (iOS 27.1), ScrollView > Content Inset Adjustment Behavior, which opens a full screen modal:
iPhone 18 Pro simulator (iOS 27.0) in landscape, the same example. Both sides carry a safe area inset here, the camera cutout side and the opposite edge:
automatic)neverdoesNesting the layout with
<SafeAreaView>(backwards compatible)Build with a temporary toggle added in the example that wraps the rows in
react-native-safe-area-context'sSafeAreaView, with a yellow background (today's temporarily necessary authoring behaviour):automaticselected.