react-native-screens icon indicating copy to clipboard operation
react-native-screens copied to clipboard

fix(new arch): change how screen size is calculated

Open j-piasecki opened this issue 6 months ago • 0 comments

Description

During the migration of Expensify App to the new architecture, we found a bug where the screen size consistently doesn't get updated to the correct one when navigating between screens while the keyboard is opened. It seems like updateScreenSizeFabric function was the problem, as removing it solved the problem (we're using JS stack).

The problem this method was supposed to fix is that yoga doesn't know about the header's existence so the screen would be too big (by the height of the header) by setting the size of the screen to match one from the native layout. I think that the issue with the keyboard could be caused by reusing the values from yoga during the native layout, keeping the wrong screen dimensions.

In https://github.com/software-mansion/react-native-screens/pull/2028 @WoLewicki added the header height to the state of the screen, which gave me an idea: we could let yoga layout the screen, giving a result with the wrong height and cut it by the size of the header we now have access to.

For now it's done using padding, which means that calling measure on screen would return wrong height, other than that I'm not aware of situations where this approach could break. The better approach would be to apply this change during the layout pass, but overriding layoutmetrics didn't really work for me.

Note: there are probably parts of this PR that are better suited to be in https://github.com/software-mansion/react-native-screens/pull/2028. Once @WoLewicki is back we'll need to discuss it.

Changes

  • Changed how screen size is calculated on the new architecture

Screenshots / GIFs

Before

https://github.com/software-mansion/react-native-screens/assets/21055725/03e68f4a-8ae0-47fc-ba02-e21676d97196

After

https://github.com/software-mansion/react-native-screens/assets/21055725/d7d2ced3-7aa7-4326-9d72-f5b3821ec1bd

Test code and steps to reproduce

The only place I could reproduce this issue is on the Expensify App with the new arch enabled in release mode on Android. This is the PR: https://github.com/Expensify/App/pull/13767.

Checklist

  • [ ] Included code example that can be used to test this change
  • [ ] Updated TS types
  • [ ] Updated documentation:
    • [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
    • [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
    • [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
    • [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
  • [ ] Ensured that CI passes

j-piasecki avatar Feb 15 '24 15:02 j-piasecki