Skip to content

fix: ReactRootView checkForKeyboardEvents to check if rootInsets are set - #35869

Closed
enahum wants to merge 1 commit into
react:mainfrom
enahum:main
Closed

fix: ReactRootView checkForKeyboardEvents to check if rootInsets are set#35869
enahum wants to merge 1 commit into
react:mainfrom
enahum:main

Conversation

@enahum

@enahumenahum commented Jan 18, 2023

Copy link
Copy Markdown
Contributor

Summary

react-native-navigation allows to register React components to be included in the navigation top bar as buttons, the way this work is by using the AppRegistry. When the ViewTreeObserver executes the CustomGlobalLayout we are checking for the RootWindowInsets in the checkKeyboardEvents which in the case for the top bar component it returns null and the WindowInsetsCompat.toWindowInsetsCompat function throws if the insets are null causing the app to crash.

Interestingly in the function checkForKeyboardEventsLegacy the null value is being checked, so I guess it was overlooked in the newer function.

Changelog

[ANDROID] [FIXED] - Fix ReactRootView crash when root view window insets are null

Test Plan

The following videos show how the app crashes as soon as we attempt to pop a screen that contains a react component as a button in the navigation top bar and how it correctly pops to the previous screen after applying the fix

CrashFix
https://user-images.githubusercontent.com/6757047/213116971-fe693989-f978-438c-b8f9-fc56f2a477c8.mp4https://user-images.githubusercontent.com/6757047/213118352-fe258f28-07aa-4d17-98d2-97136464ffd5.mp4

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 18, 2023
@analysis-bot

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,465,402-55
androidhermesarmeabi-v7a7,786,005-65
androidhermesx868,938,958-50
androidhermesx86_648,796,917-61
androidjscarm64-v8a9,650,630-24
androidjscarmeabi-v7a8,385,110-19
androidjscx869,712,837-10
androidjscx86_6410,190,014-17

Base commit: f4072b1
Branch: main

@fabOnReact

Copy link
Copy Markdown
Contributor

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Jan 19, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cortinico merged this pull request in 4cdc2c4.

cipolleschi pushed a commit that referenced this pull request Jan 19, 2023
…set (#35869)
Summary:
react-native-navigation allows to register React components to be included in the navigation top bar as buttons, the way this work is by using the AppRegistry. When the ViewTreeObserver executes the `CustomGlobalLayout` we are checking for the RootWindowInsets in the `checkKeyboardEvents` which in the case for the top bar component it returns null and the **WindowInsetsCompat.toWindowInsetsCompat** function throws if the insets are null causing the app to crash.
Interestingly in the function `checkForKeyboardEventsLegacy` the null value is being checked, so I guess it was overlooked in the newer function.
## Changelog
[ANDROID] [FIXED] - Fix ReactRootView crash when root view window insets are null
Pull Request resolved: #35869
Test Plan:
The following videos show how the app crashes as soon as we attempt to pop a screen that contains a react component as a button in the navigation top bar and how it correctly pops to the previous screen after applying the fix
| Crash | Fix |
| -- | -- |
| https://user-images.githubusercontent.com/6757047/213116971-fe693989-f978-438c-b8f9-fc56f2a477c8.mp4 | https://user-images.githubusercontent.com/6757047/213118352-fe258f28-07aa-4d17-98d2-97136464ffd5.mp4 |
Reviewed By: cipolleschi
Differential Revision: D42580156
Pulled By: cortinico
fbshipit-source-id: 4dbd656d7c8148df67668a2a50913206bc35c07f
@cipolleschicipolleschi mentioned this pull request Oct 11, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@enahum@analysis-bot@fabOnReact@facebook-github-bot@cortinico