Repository navigation
Conversation
…al safe area (iPhone Duo)
|
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?
| keys.push(edge === 'top' ? 'paddingTop' : 'paddingBottom'); | ||
| keys.push('paddingVertical'); | ||
| } | ||
| keys.push('padding'); |
There was a problem hiding this comment.
resolvePadding doesn't handle paddingInline, paddingInlineStart, paddingInlineEnd, paddingBlock, paddingBlockStart or paddingBlockEnd. Fabric maps those onto the same Yoga edges (YogaLayoutableShadowNode.cpp, around L454–485), so:
paddingInline: 16is overridden by the emittedpaddingStart/paddingEnd, and the 16pt is lost.paddingInlineStart/Endare applied afterpaddingStart/End, so the user's value wins and the safe-area inset on that edge is lost.- For horizontal scroll views,
paddingBlockis overridden, andpaddingBlockStart/Endonly apply when Top/Bottom are undefined, which they never are now.
Could we handle these keys too, or clear them in the override style?
| const left = resolvePadding(style, 'left', isRTL) + insets.left; | ||
| const right = resolvePadding(style, 'right', isRTL) + insets.right; | ||
| return { | ||
| paddingStart: isRTL ? right : left, |
There was a problem hiding this comment.
In RTL with doLeftAndRightSwapInRTL (the default), Fabric's swapLeftAndRightInYogaStyleProps runs after the styles are merged. It copies Edge::Left/Right onto Start/End unconditionally. So with contentContainerStyle={{paddingLeft: 10}}, the emitted paddingStart: 94 gets overwritten with 10 and the inset is lost.
One fix is to have the override also set paddingLeft, paddingRight, paddingHorizontal and the paddingInline* keys to undefined, so the logical keys we emit are the only ones left on that axis.
| }); | ||
| }); | ||
|
|
||
| test('keeps physical insets on their edge in RTL', () => { |
There was a problem hiding this comment.
This test passes, but it only checks the helper's output, not what Yoga actually lays out (see the RTL comment in getSafeAreaContentPadding.js). Could we add a test of the final merged props, or a Fantom test, so a mismatch between the helper's precedence and Fabric's shows up?
| * style already has. Logical edges are used across the row, since they take | ||
| * precedence over every other padding key on that edge. | ||
| */ | ||
| export default function getSafeAreaContentPadding( |
There was a problem hiding this comment.
When the cross-axis insets are 0, which is every iPhone in portrait for a vertical ScrollView, this still returns {paddingStart: 0, paddingEnd: 0}. That spreads the problems above, and percentage padding resolving to 0, to every device using automatic, not just iPhone Duo or landscape. Can we return null when both cross-axis insets are 0, and ideally only override edges with a non-zero inset?
| }; | ||
|
|
||
| _handleSafeAreaInsetsChange = (e: SafeAreaInsetsChangeEvent) => { | ||
| this.setState({safeAreaInsets: e.nativeEvent.insets}); |
There was a problem hiding this comment.
Nit: this calls setState on any inset change, including top/bottom for a vertical ScrollView, which don't affect the padding. Since the event renders synchronously, it would be worth bailing out when the cross-axis insets haven't changed.
| onTouchStart: this._handleTouchStart, | ||
| onTouchCancel: this._handleTouchCancel, | ||
| onScroll: this._handleScroll, | ||
| experimental_onSafeAreaInsetsChange: padsContentForSafeArea |
There was a problem hiding this comment.
If the user passes their own experimental_onSafeAreaInsetsChange, the native prop stays set under 'never', so the native dedupe cache isn't reset. If the insets change while in 'never' and the behavior then switches to 'automatic', nothing is re-emitted and state.safeAreaInsets is stale. Routing through _handleSafeAreaInsetsChange whenever either handler is present would avoid that.
| // out clear of the system UI, such as the rail on iPhone Duo. Under | ||
| // 'always' UIKit insets that axis as well, which the contentInset passed | ||
| // to native cancels out. | ||
| const padsContentForSafeArea = |
There was a problem hiding this comment.
A few cases worth a quick check:
centerContent+'always': native ignores thecontentInsetprop whencenterContentis set (RCTScrollViewComponentView.mm:405), so the cancellation doesn't apply and the inset is applied twice.alwaysBounceHorizontalon a vertical ScrollView: I believe UIKit treats that axis as scrollable underautomatic/scrollableAxes, which would also apply the inset twice. I haven't verified this.- Vertical ScrollView inside a horizontal pager, in landscape: the inner ScrollView's left/right insets may change every frame while swiping. That would mean a synchronous render per frame and could trigger the dev "fired more than 10 times" warning.
@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 |
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', thecontentInsetpassed to native subtracts the safe area across the scrollable axis, so UIKit's content inset there nets to the caller's own and the safe area applies once.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.