Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New Arch] RTL to LTR of vise versa Layout Direction Change Requires App Termination to Apply #48311

Closed
1 task done
a-eid opened this issue Dec 17, 2024 · 24 comments
Closed
1 task done
Labels
Resolution: Fixed A PR that fixes this issue has been merged.

Comments

@a-eid
Copy link

a-eid commented Dec 17, 2024

Description

When toggling RTL layout direction dynamically using I18nManager.forceRTL(true) or switching between languages in the system settings, the Yoga layout engine does not immediately apply the updated layout direction. Even with a forced app restart (e.g., via RNRestart), the changes are not respected.

A complete termination and relaunch of the app is required for the changes to take effect. This creates significant friction for users and developers aiming to support dynamic language and layout changes for right-to-left (RTL) languages.

Report

Issues and Steps to Reproduce

Toggle RTL programmatically:

I18nManager.forceRTL(true);
I18nManager.allowRTL(true);
RNRestart.Restart();

Alternatively, change the device language to an RTL language (e.g., Arabic ) in the app settings. ( assuming you added such functionality )

Observe that the layout direction does not change immediately.

Terminate the app completely and relaunch it manually. Only then does the layout respect the RTL direction.

Expected Behavior

the layout direction should change.

Actual Behavior

the layout direction does not change.

related react-native Issue

@a-eid a-eid changed the title RTL to LTR of vise versa Layout Direction Requires App Termination to Apply RTL to LTR of vise versa Layout Direction Change Requires App Termination to Apply Dec 17, 2024
@NickGerleman NickGerleman transferred this issue from facebook/yoga Dec 17, 2024
@react-native-bot react-native-bot added Needs: Author Feedback Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. labels Dec 17, 2024
@react-native-bot
Copy link
Collaborator

Warning

Missing reproducer: We could not detect a reproducible example in your issue report. Please provide either:

@NickGerleman
Copy link
Contributor

Moved to RN repo since this is implementation issue with RN iOS New arch instead of Yoga layer.

@a-eid
Copy link
Author

a-eid commented Dec 17, 2024

@NickGerleman unfortunately the RN issue is locked and I don't think there is any plans to fix this issue any time soon.
it's blocking many RTL apps from migrating to the new arch.

I thought there might be an issue with how yoga handles RTL & LTR with the new arch..

reproducer

@serhii-yalla

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Dec 17, 2024
@a-eid a-eid changed the title RTL to LTR of vise versa Layout Direction Change Requires App Termination to Apply [New Arch] RTL to LTR of vise versa Layout Direction Change Requires App Termination to Apply Dec 17, 2024
@Linuhusainnk
Copy link

Any update on this?

@MehadND
Copy link

MehadND commented Jan 3, 2025

Any update?

@a-eid
Copy link
Author

a-eid commented Jan 20, 2025

@blakef are there any plans to fix this issue ? most apps are moving to the new arch, we're unable to because of this issue unfortunately. we'd appreciate if you could share some updates.

@blakef
Copy link
Contributor

blakef commented Jan 20, 2025

AFAIK there isn't work planned to fix this. PRs are always welcome.

Cc @cipolleschi, might know more.

@a-eid
Copy link
Author

a-eid commented Jan 23, 2025

With libraries targeting only the new arch, eg: reanimated v4, unistyles v3 & shadowlist, I think this is very essential to fix.

Edit: Nitro Views will also only target the new arch.

@Linuhusainnk
Copy link

Is this issue addressed in latest version? 0.77

@blakef
Copy link
Contributor

blakef commented Jan 24, 2025

No, it isn't.

@zaidqureshi95
Copy link

anyone fixed the issue?

@Linuhusainnk
Copy link

Not still now, waiting for the fix

@Makhdoom-Sharif
Copy link

I am experiencing the same issue. Has anyone found a solution?

@Abdulazeez-98
Copy link

Abdulazeez-98 commented Feb 10, 2025

I faced the same problem when I updated to RN 0.76 (expo-52). One solution that worked for me is to specify the 'direction' property of the <NavigationContainer /> component (from '@react-navigation/native') which wraps the whole app (set it to either 'ltr' or 'rtl').

Edit: this solved like 90% of the issue. Many views are still rendered as RTL when the device is set to RTL.

@a-eid
Copy link
Author

a-eid commented Feb 10, 2025

@Abdulazeez-98 I thought this would only control the stack navigation direction, does it also control the layout direction ?

@Abdulazeez-98
Copy link

@Abdulazeez-98 I thought this would only control the stack navigation direction, does it also control the layout direction ?

In my case, it solved the issue for the whole app. I applied the 'ltr' layout to the main navigator (which is a bottom tab navigator with 4 nested stack navigators). I think nested Views inherits this property.

@sabuhiteymurov
Copy link

I think this is a major issue which should not be ignored by the core team. We are unable to migrate to new architecture because of this issue.

@a-eid
Copy link
Author

a-eid commented Feb 13, 2025

Sadly, it doesn't look like Meta is prioritizing this fix. However, @jamonholmgren and @mazenchami from @infinitered have shown interest in tackling it, so hopefully, it’ll be resolved soon.

@andremawad
Copy link

The below patch has fixed the issue for me:

diff --git a/node_modules/react-native/React/Fabric/Surface/RCTFabricSurface.mm b/node_modules/react-native/React/Fabric/Surface/RCTFabricSurface.mm
index 7b3ce59..3906df7 100644
--- a/node_modules/react-native/React/Fabric/Surface/RCTFabricSurface.mm
+++ b/node_modules/react-native/React/Fabric/Surface/RCTFabricSurface.mm
@@ -143,6 +143,7 @@ - (RCTSurfaceView *)view
 
   if (!_view) {
     _view = [[RCTSurfaceView alloc] initWithSurface:(RCTSurface *)self];
+    [self _updateLayoutContext];
     _touchHandler = [RCTSurfaceTouchHandler new];
     [_touchHandler attachToView:_view];
   }

@a-eid
Copy link
Author

a-eid commented Feb 19, 2025

@andremawad yeah, thanks hopefully @chrsmys PR will be merged soon.

@mazenchami
Copy link

it looks like 0.78 was just released but after Meta validates it against their main it might out with 0.78.1

@cortinico
Copy link
Contributor

it looks like 0.78 was just released but after Meta validates it against their main it might out with 0.78.1

Yup 0.78 was just out, but given the patch is a one-liner, this is easy to integrate in other previous versions as well

facebook-github-bot pushed a commit that referenced this issue Feb 19, 2025
Summary:
This fixes an issue in Fabric where changing the layout direction and then reloading the JS bundle did not honor the layout direction until the app was restarted on iOS. This now calls  `_updateLayoutContext` whenever RCTSurfaceView is recreated which happens on bundle reload. This is not an issue on the old architecture because the layout direction is determined within the [SurfaceViews](https://github.com/facebook/react-native/blob/acdddef48eb60b002c954d7d2447cb9c2883c8b3/packages/react-native/React/Views/RCTRootShadowView.m#L18) which were recreated on bundle reload.

## Related Issues:
- react-native-community/discussions-and-proposals#847
- #49451
- #48311
- #45661

## How can we take this further?
If we want to make it so that it doesn't require an entire bundle reload for RTL to take effect I believe these are the steps that would need to be taken:
- Make it so [RCTI18nManager](https://github.com/facebook/react-native/blob/acdddef48eb60b002c954d7d2447cb9c2883c8b3/packages/react-native/React/CoreModules/RCTI18nManager.mm#L52) exports isRTL as a method instead of consts
- Send Notification Center notif when RTL is forced on or off
- Listen for that notification RCTSurfaceView and call _updateLayoutContext similar to UIContentSizeCategoryDidChangeNotification.

## Changelog:

[iOS] [FIXED] - Layout direction changes are now honored on bundle reload.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #49455

Test Plan:
On the new architecture change force the layout direction and reload the bundle:
```
import React, { useCallback } from "react";
import { Button, I18nManager, StyleSheet, Text, View } from "react-native";
import RNRestart from "react-native-restart";

export default function Explore() {
    const onApplyRTL = useCallback(() => {
        if (!I18nManager.isRTL) {
            I18nManager.forceRTL(true);
            RNRestart.restart();
        }
    }, []);

    const onApplyLTR = useCallback(() => {
        if (I18nManager.isRTL) {
            I18nManager.forceRTL(false);
            RNRestart.restart();
        }
    }, []);

    return (
        <View style={styles.area}>
            <Text>Test Block</Text>
            <View style={styles.testBlock}>
                <Text>Leading</Text>
                <Text>Trailing</Text>
            </View>
            <Button title={"Apply RTL"} onPress={onApplyRTL} />
            <Button title={"Apply LTR"} onPress={onApplyLTR} />
        </View>
    );
}

const styles = StyleSheet.create({
    area: {
        marginVertical: 50,
        paddingHorizontal: 24,
    },
    testBlock: {
        paddingVertical: 10,
        flexDirection: "row",
        justifyContent: "space-between",
    },
});

```

https://github.com/user-attachments/assets/0eab0d79-de3f-4eeb-abd0-439ba4fe25c0

Reviewed By: cortinico, cipolleschi

Differential Revision: D69797645

Pulled By: NickGerleman

fbshipit-source-id: 97499621f3dd735d466f5119e0f2a0eccf1c3c05
@cipolleschi
Copy link
Contributor

make sure to open pick requests. Please, one pick request per each RN version! 🙏

@cortinico
Copy link
Contributor

Closing as this was solved with #49455

@cortinico cortinico added Resolution: Fixed A PR that fixes this issue has been merged. and removed Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. Needs: Attention Issues where the author has responded to feedback. labels Feb 21, 2025
react-native-bot pushed a commit that referenced this issue Mar 10, 2025
Summary:
This fixes an issue in Fabric where changing the layout direction and then reloading the JS bundle did not honor the layout direction until the app was restarted on iOS. This now calls  `_updateLayoutContext` whenever RCTSurfaceView is recreated which happens on bundle reload. This is not an issue on the old architecture because the layout direction is determined within the [SurfaceViews](https://github.com/facebook/react-native/blob/acdddef48eb60b002c954d7d2447cb9c2883c8b3/packages/react-native/React/Views/RCTRootShadowView.m#L18) which were recreated on bundle reload.

## Related Issues:
- react-native-community/discussions-and-proposals#847
- #49451
- #48311
- #45661

## How can we take this further?
If we want to make it so that it doesn't require an entire bundle reload for RTL to take effect I believe these are the steps that would need to be taken:
- Make it so [RCTI18nManager](https://github.com/facebook/react-native/blob/acdddef48eb60b002c954d7d2447cb9c2883c8b3/packages/react-native/React/CoreModules/RCTI18nManager.mm#L52) exports isRTL as a method instead of consts
- Send Notification Center notif when RTL is forced on or off
- Listen for that notification RCTSurfaceView and call _updateLayoutContext similar to UIContentSizeCategoryDidChangeNotification.

## Changelog:

[iOS] [FIXED] - Layout direction changes are now honored on bundle reload.

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #49455

Test Plan:
On the new architecture change force the layout direction and reload the bundle:
```
import React, { useCallback } from "react";
import { Button, I18nManager, StyleSheet, Text, View } from "react-native";
import RNRestart from "react-native-restart";

export default function Explore() {
    const onApplyRTL = useCallback(() => {
        if (!I18nManager.isRTL) {
            I18nManager.forceRTL(true);
            RNRestart.restart();
        }
    }, []);

    const onApplyLTR = useCallback(() => {
        if (I18nManager.isRTL) {
            I18nManager.forceRTL(false);
            RNRestart.restart();
        }
    }, []);

    return (
        <View style={styles.area}>
            <Text>Test Block</Text>
            <View style={styles.testBlock}>
                <Text>Leading</Text>
                <Text>Trailing</Text>
            </View>
            <Button title={"Apply RTL"} onPress={onApplyRTL} />
            <Button title={"Apply LTR"} onPress={onApplyLTR} />
        </View>
    );
}

const styles = StyleSheet.create({
    area: {
        marginVertical: 50,
        paddingHorizontal: 24,
    },
    testBlock: {
        paddingVertical: 10,
        flexDirection: "row",
        justifyContent: "space-between",
    },
});

```

https://github.com/user-attachments/assets/0eab0d79-de3f-4eeb-abd0-439ba4fe25c0

Reviewed By: cortinico, cipolleschi

Differential Revision: D69797645

Pulled By: NickGerleman

fbshipit-source-id: 97499621f3dd735d466f5119e0f2a0eccf1c3c05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
Development

No branches or pull requests