Correctly create the first modal state - #52835
Closed
cortinico wants to merge 1 commit into
Closed
Conversation
facebook-github-bot
added
CLA Signed
Contributor
|
@cortinico has imported this pull request. If you are a Meta employee, you can view this in D78975126. |
Job Summary for GradleTest All :: build_android
|
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 28, 2025
Summary: There is currently a bug with Modals with New Architecture where the first frame is rendered incorrectly, specifically not accounting for all the vertical insets (only the status bar). This fixes it. ## Changelog: [ANDROID] [FIXED] - Correctly account for insets on first render of Modals on New Arch Test Plan: See internal CI Differential Revision: D78975126 Pulled By: cortinico
facebook-github-bot
force-pushed
the
nc/modal-fix
branch
from
July 28, 2025 15:30
7fcaae2 to
1e619d1
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D78975126 |
Job Summary for GradleTest All :: run_fantom_tests
|
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 30, 2025
Summary: There is currently a bug with Modals with New Architecture where the first frame is rendered incorrectly, specifically not accounting for all the vertical insets (only the status bar). This fixes it. Specifically: 1. I've removed the caching of the statusbar height from `ReactModalHostView` as that was not working correctly. Sometimes the value returned `0` meaning that it was not yet computed when Fabric was asking for it. In the updated implementation we now query `FabricUIManager` given the `surfaceId` of the modal. 2. I've modified the logic to account for all the vertical insets, not just the status bar. ## Changelog: [ANDROID] [FIXED] - Correctly account for insets on first render of Modals on New Arch Test Plan: Tested on Marketplace Location Picker and the picker is still working correctly: https://pxl.cl/7NjtJ Differential Revision: D78975126 Pulled By: cortinico
facebook-github-bot
force-pushed
the
nc/modal-fix
branch
from
July 30, 2025 10:08
1e619d1 to
7384f6f
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D78975126 |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D78975126 |
cortinico
added a commit
that referenced
this pull request
Aug 1, 2025
Summary: There is currently a bug with Modals with New Architecture where the first frame is rendered incorrectly, specifically not accounting for all the vertical insets (only the status bar). This fixes it. Specifically: 1. I've removed the caching of the statusbar height from `ReactModalHostView` as that was not working correctly. Sometimes the value returned `0` meaning that it was not yet computed when Fabric was asking for it. In the updated implementation we now query `FabricUIManager` given the `surfaceId` of the modal. 2. I've modified the logic to account for all the vertical insets, not just the status bar. ## Changelog: [ANDROID] [FIXED] - Correctly account for insets on first render of Modals on New Arch Pull Request resolved: #52835 Test Plan: Tested on Marketplace Location Picker and the picker is still working correctly: https://pxl.cl/7NjtJ Reviewed By: mdvacca Differential Revision: D78975126 Pulled By: cortinico
Collaborator
|
This pull request was successfully merged by @cortinico in 3799da0 When will my fix make it into a release? | How to file a pick request? |
Job Summary for GradleTest All :: run_fantom_tests
|
Job Summary for GradleTest All :: build_android
|
Summary: There is currently a bug with Modals with New Architecture where the first frame is rendered incorrectly, specifically not accounting for all the vertical insets (only the status bar). This fixes it. Specifically: 1. I've removed the caching of the statusbar height from `ReactModalHostView` as that was not working correctly. Sometimes the value returned `0` meaning that it was not yet computed when Fabric was asking for it. In the updated implementation we now query `FabricUIManager` given the `surfaceId` of the modal. 2. I've modified the logic to account for all the vertical insets, not just the status bar. ## Changelog: [ANDROID] [FIXED] - Correctly account for insets on first render of Modals on New Arch Test Plan: Tested on Marketplace Location Picker and the picker is still working correctly: https://pxl.cl/7NjtJ Reviewed By: mdvacca Differential Revision: D78975126 Pulled By: cortinico
facebook-github-bot
force-pushed
the
nc/modal-fix
branch
from
August 7, 2025 15:01
3799da0 to
807ace6
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D78975126 |
cortinico
added a commit
to cortinico/react-native
that referenced
this pull request
Aug 7, 2025
Summary: There is currently a bug with Modals with New Architecture where the first frame is rendered incorrectly, specifically not accounting for all the vertical insets (only the status bar). This fixes it. ## Changelog: [ANDROID] [FIXED] - Correctly account for insets on first render of Modals on New Arch Pull Request resolved: react#52835 Test Plan: See internal CI Differential Revision: D78975126 Pulled By: cortinico
Collaborator
|
This pull request was successfully merged by @cortinico in 2e76fc8 When will my fix make it into a release? | How to file a pick request? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary:
There is currently a bug with Modals with New Architecture where the first frame is rendered incorrectly, specifically not accounting for all the vertical insets (only the status bar). This fixes it.
Changelog:
[ANDROID] [FIXED] - Correctly account for insets on first render of Modals on New Arch
Test Plan:
See internal CI