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

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

j-piasecki
Copy link
Member

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 #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 #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

Screen.Recording.2024-02-15.at.16.12.50.mov

After

Screen.Recording.2024-02-15.at.16.29.21.mov

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: Expensify/App#13767.

Checklist

Base automatically changed from @wolewicki/proper-yoga-on-fabric to main March 15, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants