fix(new arch): change how screen size is calculated #2034
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 overridinglayoutmetrics
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
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