Conversation
There was a problem hiding this comment.
Pull request overview
Adds RTL/LTR layout direction control to the experimental SplitHost API, with iOS-version-specific native implementations to ensure correct trait propagation across the split view controller hierarchy.
Changes:
- Introduces a new
directionprop onSplitHostand wires it to the nativelayoutDirectiontrait override. - Implements iOS handling using
traitOverrides.layoutDirectionon iOS 17+ and parentsetOverrideTraitCollectionpropagation below iOS 17 (with a pending-update path until the controller is parented). - Adds a new single-feature test scenario for exercising direction changes.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/fabric/gamma/split/SplitHostNativeComponent.ts | Adds layoutDirection native prop (codegen) and layout direction enum. |
| src/components/gamma/split/index.ts | Re-exports new SplitHostDirection type. |
| src/components/gamma/split/SplitHost.types.ts | Adds SplitHostDirection type and documents new direction prop. |
| src/components/gamma/split/SplitHost.tsx | Passes JS direction through to native layoutDirection. |
| ios/gamma/split/RNSSplitHostController.swift | Adds below-iOS17 layout direction update signaling and pending application once parented. |
| ios/gamma/split/RNSSplitHostComponentView.mm | Tracks prop changes and applies layout direction via iOS17 trait overrides or below-iOS17 update path. |
| ios/gamma/split/RNSSplitHostComponentView.h | Exposes layoutDirection for Swift-side applicator access. |
| ios/gamma/split/RNSSplitAppearanceUpdateFlags.swift | Adds an appearance update flag for below-iOS17 direction updates. |
| ios/gamma/split/RNSSplitAppearanceApplicator.swift | Applies below-iOS17 trait override update via parent controller. |
| ios/conversion/RNSConversions.h | Declares conversion helper for SplitHost layout direction enum. |
| ios/conversion/RNSConversions-SplitView.mm | Implements conversion to UITraitEnvironmentLayoutDirection. |
| apps/src/tests/single-feature-tests/split/test-split-direction.tsx | Adds a manual scenario to toggle direction at runtime. |
| apps/src/tests/single-feature-tests/split/index.ts | Registers the new split direction scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This enables us to fully recreate the Split when necessary, ensuring the correct column configuration is always applied. | ||
| key={`columns-${columns.length}-inspectors-${inspectors.length}`} | ||
| {...props} | ||
| layoutDirection={direction} | ||
| style={styles.container}> |
There was a problem hiding this comment.
Copilot is right here, I had the same exact bug in Tabs and I've spent too much time trying to figure this out. We should destructure props and keep the rest of the props in filteredProps and only spread the filteredProps, as is done in e.g. TabsHost.ios.tsx.
cc @t0maboro
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| #if RNS_IPHONE_OS_VERSION_AVAILABLE(17_0) | ||
| if (@available(iOS 17.0, *)) { | ||
| _controller.traitOverrides.layoutDirection = _layoutDirection; | ||
| } else | ||
| #endif // RNS_IPHONE_OS_VERSION_AVAILABLE(17_0) | ||
| [_controller setNeedsLayoutDirectionUpdateBelowIOS17]; |
There was a problem hiding this comment.
In tabs this is also done in similar way but maybe for split we should keep the clean flow where in updateProps we set only the props and flags, here we set only the flag on controller (so more general [_controller setNeedsLayoutDirectionUpdate] and then at the level of appearance applicator we have some updateLayoutDirection function that will run updateLayoutDirectionBelowIOS17 / updateLayoutDirectionAboveIOS17 (traitOverrides.layoutDirection = _layoutDirection) depending on iOS version. What do you think?
There was a problem hiding this comment.
It makes sense. However, after change in b6470e1, it's controller who detects, which iOS version we're currently at (not the applicator). Such approach seems better for me, especially if we are below iOS17 and the controller.parent is still nil.
| // This enables us to fully recreate the Split when necessary, ensuring the correct column configuration is always applied. | ||
| key={`columns-${columns.length}-inspectors-${inspectors.length}`} | ||
| {...props} | ||
| layoutDirection={direction} | ||
| style={styles.container}> |
There was a problem hiding this comment.
Copilot is right here, I had the same exact bug in Tabs and I've spent too much time trying to figure this out. We should destructure props and keep the rest of the props in filteredProps and only spread the filteredProps, as is done in e.g. TabsHost.ios.tsx.
cc @t0maboro
| return proposedTopColumn | ||
| } | ||
|
|
||
| public override func didMove(toParent parent: UIViewController?) { |
There was a problem hiding this comment.
I don't think this should be in UISplitViewControllerDelegate extension block. It should probably be in main block with // MARK: UIKit callbacks
| <Split.Host direction={direction}> | ||
| <Split.Column> | ||
| <ColumnContent | ||
| columnTitle="Primary column" | ||
| currentDirection={direction} | ||
| onChangeDirection={setDirection} | ||
| /> | ||
| </Split.Column> | ||
| <Split.Column> | ||
| <ColumnContent columnTitle="Secondary column" /> | ||
| </Split.Column> | ||
| </Split.Host> |
There was a problem hiding this comment.
From what I know, the SplitView layout can be more complicated, you can have more columns, there is an inspector column. Should we maybe test with a more complicated Split layout to test more cases?
cc @t0maboro
There was a problem hiding this comment.
+1, afair changing number of columns should work even in runtime, so even conditional rendering could be possilbe to have a setup with 2 or 3 columns
| ); | ||
| } | ||
|
|
||
| export function ColumnContent(props: { |
There was a problem hiding this comment.
Why is the column cut off below iOS 26? It doesn't look centered.
Also, there seems to be some kind of bug with the shadow? Do you know about this, @t0maboro?
split_iOS_18_6.mov
There was a problem hiding this comment.
the native offset of this column on iOS < 26 has an offset around -100px on X axis, so that's fine; for shadow, not sure about the origin of this issue, will take a look while reviewing this PR
There was a problem hiding this comment.
the shadow is because the Primary column hasn't backgroundColor set, when it's transparent, it samples the color from the content behind, natively it works the same
sv-native.mov
There was a problem hiding this comment.
It's so weird but okay. Would SafeAreaView help with the content being cut off 100px?
There was a problem hiding this comment.
So we should probably use it here if we can @sgaczol
There was a problem hiding this comment.
I think that we should probably follow new test structure (separate directory, scenario.md file) for new tests.
There was a problem hiding this comment.
In this test I think that we're missing checking how our configuration behaves with react-native's I18nManager. Downstream implementations will probably use I18nManager.isRTL as suggested in the docs and how it's done in TabsContainer. I know that we don't have something like SplitContainer but we should probably add an option I18nManager.isRTL in test in addition to current inherit/ltr/rtl. We should also have a way to configure forceRTL and allowRTL.
fixes an issue where we passed direction prop to SplitHostNativeComponent (which is unknown on native side)
|
PR is currently blocked by https://github.com/software-mansion/react-native-screens-labs/issues/1429 - it needs to be fixed in order to make a stable test. |
Description
This PR introduces
RTLsupport toSplitHost.The approach differs on iOS versions prior to iOS 17 and newer ones.
On < iOS 17 versions, we use
setOverrideTraitCollection(_: forChild:)on the parent view controller to propagate the layout direction, accordingly to https://github.com/software-mansion/react-native-screens-labs/blob/main-issue-tracker/rfcs/0996-rtl-and-dark-mode.mdThere is a specific edge case on those iOS versions: if the layout direction is changed while a column is currently open/visible, the UI becomes inconsistent. The sidebar remains on its original side, while the display mode button moves to the new side. The layout only synchronizes correctly after the column is closed and reopened. I checked this issue and it's a native bug: https://github.com/software-mansion/react-native-screens-labs/issues/1237
On ≥ iOS 17 versions, we use
traitOverridesAPI on the view controller.Closes https://github.com/issues/assigned?issue=software-mansion%7Creact-native-screens-labs%7C1027
Currently blocked by https://github.com/software-mansion/react-native-screens-labs/issues/1429 - it needs to be fixed in order to make a stable test.
Changes
directionprop toSplitHostlayoutDirectionon iOS sidetest-split-direction.tsxAfter - visual documentation
iOS.Below.17.RTL.mov
iOS17.RTL.mov
inheritoption also works properly for locations withRTLdirection as a default:default.RTL.location.mov
Test plan
Run
test-split-direction.Checklist