facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.51k stars 24.37k forks source link

No hot reloading when change font scale #45655

Open yzhe554 opened 4 months ago

yzhe554 commented 4 months ago

Description

In old arch, we can change font scale in realtime with command option +/-. In new arch, it has to be refreshed manually with pressing R to get new font scale applied.

Steps to reproduce

  1. gh repo clone yzhe554/new-arch-font-scale
  2. cd new-arch-font-scale
  3. npm i
  4. npm run ios
  5. change font scale with option command +/-

React Native Version

0.74.3

Affected Platforms

Runtime - iOS

Areas

Fabric - The New Renderer

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU: (10) arm64 Apple M1 Max
  Memory: 107.00 MB / 32.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.18.0
    path: ~/.nvm/versions/node/v18.18.0/bin/node
  Yarn:
    version: 1.22.18
    path: /usr/local/bin/yarn
  npm:
    version: 9.8.1
    path: ~/.nvm/versions/node/v18.18.0/bin/npm
  Watchman:
    version: 2024.05.06.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.15.2
    path: /opt/homebrew/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.2 AI-232.10300.40.2321.11567975
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.9
    path: /usr/bin/javac
  Ruby:
    version: 2.7.6
    path: /Users/yuzheng/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.3
    wanted: 0.74.3
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: Not found
  newArchEnabled: Not found
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

N/A

Reproducer

https://github.com/yzhe554/new-arch-font-scale

Screenshots and Videos

In Old Arch

https://github.com/user-attachments/assets/b1e2d7a7-1070-4d3d-ac2c-a7e75952ef03

In New Arch

https://github.com/user-attachments/assets/e7ef203c-c355-4674-80de-e508c046e61f

jpudysz commented 4 months ago

Hey @cortinico, there is one more side effect connected to this issue. I'm the author of react-native-unistyles, and Unistyles automatically re-renders the view on different events like font scale changes. In the new architecture, there is different behavior, and the text is cut off at the edges. It's not initially visible because the user needs to reset the app as mentioned above, but with Unistyles, it's visible after the first re-render.

Here is link to the unistyles issue: https://github.com/jpudysz/react-native-unistyles/issues/256 Repro: as above linked by @yzhe554

Minimal steps to reproduce:

export default function App() {
    const [counter, setCounter] = useState(0)

    return (
        <View 
            style={{
              flex: 1,
              backgroundColor: '#fff',
              alignItems: 'center',
              justifyContent: 'center',
            }}
        >
          <Text style={{fontSize: 24}}>RN 24 Label Testing!!!</Text>
          <Text style={{fontSize: 18}}>RN  18 Label Testing!!!</Text>
          <Text style={{fontSize: 16}}>RN 16 Label Testing!!!</Text>
          <Button onPress={() => setCounter(prevState => prevState + 1)} title="Re-render" />
          <StatusBar style="auto" />
        </View>
    )
}

Try to change font scale and then press re-render button.

Result for new architecture:

https://github.com/user-attachments/assets/e1a4f765-e055-4455-93f2-795f50c05269

Old arch:

https://github.com/user-attachments/assets/3158c494-ff04-4ca7-aedd-e27afb2fb0c6

cipolleschi commented 4 months ago

Hi there! These are actually two separate issues. So, let's tackle them separatedly.

First, @yzhe554 thanks for reporting the issue. I looked into it and discussed it with some colleagues that have more context.

This is a known limitation, as of today, because in the New Architecture we don't have a good mechanism to notify React that all the nodes received the updated text multiplier. It is also an edge case, as in production it is very rare that a user go in the settings to change the preferred text size and then it returns to the app. Furthermore, the issue solves itself if you navigate to another screen or you do a state update that force a re-render

We are exploring how to implement some features that can be used as foundations to fix this one as well, so the fix to this will wait until that thread of work is done.


Then, @jpudysz, this is a different issue. I'd love if you can do a test for me. What happens if the user:

The reason I ask is that my gut feeling is that this scenario will "fix" your issue. Text multiplier affects layout as well and, from the video, it looks like that the rerender from unistyle is not updating the layout correctly: the left margin in the New Architecture is unchanged, while, in the old architecture, the left margin shrinks while the font increase its size.

yzhe554 commented 4 months ago

@cipolleschi Thanks for the quick feedback. I am working on an enterprise level project that is trying to introduce RN as a brownfield project. This would be a key point whether we can go with new arch. If you have a rough timeline on the fix, it will be really helpful for us to plan.

jpudysz commented 4 months ago

hey @cipolleschi , I will create a separate issue with repro and all the details

cipolleschi commented 3 months ago

@yzhe554 we don't have a rough timeline as of today, unfortunately. Is it a use case you face often, of people going to the setting page, come back to the app and expect to see the app updated?

yzhe554 commented 3 months ago

@yzhe554 we don't have a rough timeline as of today, unfortunately. Is it a use case you face often, of people going to the setting page, come back to the app and expect to see the app updated?

Nah, I agree it's an edge case. But just need to convince other team(non tech). I will follow this for now. Thanks again.