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

compute correct Keyboard Height with Notch #30919

Closed
wants to merge 11 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Feb 9, 2021

Summary

fixes #27089 fixes #30191 fixes #26296 fixes #24353
Related #30052 #28004 #26536

The keyboard height of event keyboardDidShow is computed as the difference of two variables:

  • The screen height excluding the Android Notch
    DisplayMetricsHolder.getWindowDisplayMetrics().heightPixels returns the screen height excluding the Android Notch
  • The Visible Area excluding the Keyboard, but including the Android Notch
    getWindowVisibleDisplayFrame() which returns the visible area including the Android Notch

The computation of the keyboard height is wrong when the device has an Android Notch.
This pr adds the Android Notch computation for API levels 28+

More info at #27089 (comment)

Changelog

[Android] [Fixed] - Compute Android Notch in keyboardDidShow height calculation API 28+

Test Plan

adding a ReactRootViewTest for keyboardDidShow verifying correct functionality on API < 28

TEST CASE - BEFORE FIX

WITHOUT NOTCH

  • The black view on the bottom is visible
  • The keyboard height is 282
Full Screen Keyboard Did Show

WITH NOTCH

  • The black view on the bottom is not visible. The black view is not visible because keyboardDidHide is sending the wrong keyboard height value.
  • The keyboard height changes to 234. The keyboard height is the same from the previous test, but the value sent from keyboardDidHide changed for the Notch.
Full Screen Keyboard Did Show

TEST CASE - AFTER FIX

WITH NOTCH

  • The black view on the bottom is visible
  • The keyboard height is 282
Full Screen Keyboard Did Show

@facebook-github-bot facebook-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 Feb 9, 2021
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

@analysis-bot
Copy link

analysis-bot commented Feb 9, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3516090

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

@fabOnReact fabOnReact marked this pull request as ready for review February 9, 2021 14:28
@fabOnReact fabOnReact marked this pull request as draft February 12, 2021 16:00
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

@@ -7,10 +7,19 @@

package com.facebook.react;

import com.facebook.react.uimanager.ReactRoot;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google-java-format suggested changes:

@@ -10,7 +9,0 @@
-import com.facebook.react.uimanager.ReactRoot;
-import com.facebook.react.bridge.Arguments;
-import com.facebook.react.modules.core.DeviceEventManagerModule;
-import com.facebook.react.bridge.WritableMap;
-import android.util.Log;
-import android.graphics.Rect;
-

import static org.fest.assertions.api.Assertions.assertThat;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.eq;
import org.mockito.Mockito;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google-java-format suggested changes:

@@ -20 +12,0 @@
-import org.mockito.Mockito;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google-java-format suggested changes:

@@ -22 +13,0 @@
-import static org.mockito.Mockito.spy;

@analysis-bot
Copy link

analysis-bot commented Feb 15, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,709,443 -26
android hermes armeabi-v7a 7,239,438 -16
android hermes x86 8,129,905 -26
android hermes x86_64 8,095,108 -23
android jsc arm64-v8a 9,628,900 -78
android jsc armeabi-v7a 8,546,407 -84
android jsc x86 9,643,639 -85
android jsc x86_64 10,252,533 -87

Base commit: 3516090

@jsamr
Copy link

jsamr commented Mar 24, 2021

@fabriziobertoglio1987 Do you think it would also address #23693? (the issue was closed but the bug still thrives).

@fabOnReact

This comment has been minimized.

@OrLevy23
Copy link

is this something that will be merged? i'm having issues with this

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 27, 2021
@cortinico
Copy link
Contributor

Thanks for your PR @fabriziobertoglio1987.
Don't you mind rebasing? Currently the Android CI is failing with:

log.cpp:56: error: undefined reference to '__android_log_write'

which I believe is due to HEAD being old.
I'll import this internally and look into it tomorrow 👍

@facebook-github-bot
Copy link
Contributor

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

Without Notch
- DisplayMetricsHolder.getWindowDisplayMetrics().heightPixels is 1794
- mVisibleViewArea.bottom is 1794

**With Notch**
- DisplayMetricsHolder.getWindowDisplayMetrics().heightPixels is 1668
- mVisibleViewArea.bottom is 1794

Observations:
- getWindowVisibleDisplayFrame().heightPixels returns correct height for devices with Notch
- mVisibleViewArea.bottom does not take in consideration the notch (the
  value does not change)

Solution:
- Including the Notch Height in the Keyboard Height calculation

Clear Explanation at facebook#27089 (comment)

https://github.com/facebook/react-native/blob/d79212120b7168015d3d0225ef372ed851a230fa/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java#L757-L758
basic setup of the test passing green verifies that the event is invoked
twice

Still have to understand why the event is triggered twice

I faked the implementation of getWindowVisibleDisplayFrame to simulate
the keyboard opening that triggers keyboardDidShow event
The method has annotation @VisibleForTesting and is available only for
testing purposes
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good on my end, thanks for doing it 👍
I've manually rebased on your behalf and will wait for a green CI before merging

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fabriziobertoglio1987 in 8bef3b1.

When will my fix make it into a release? | Upcoming Releases

2 similar comments
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fabriziobertoglio1987 in 8bef3b1.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fabriziobertoglio1987 in 8bef3b1.

When will my fix make it into a release? | Upcoming Releases

facebook-github-bot pushed a commit that referenced this pull request Jan 31, 2022
Summary:
Fixes a potential crash was introduced by #30919 that aimed to get the keyboard height on devices with a Notch. The problem is that it considers that any ReactRootView will have an insets available.

When using [react-native-navigation](https://github.com/wix/react-native-navigation) and assigning a Navigation button to the TopBar as a component, the component gets registered as a RootView but won't have any insets attach to the view.

[getRootWindowInsets()](https://developer.android.com/reference/android/view/View#getRootWindowInsets()) in fact return a `WindowInset` only available if the view is attached, so when executing `checkForKeyboardEvents` method from ReactRootView, is trying to access the `DisplayCutout` of a null object, leading to a crash.

## Changelog

[Android] [Fixed] - Fix potential crash if ReactRootView does not have insets attached.

Pull Request resolved: #32989

Test Plan:
Without the code change: Notice how the second screen being push contains a React Component on the top right of the navigation bar, and when component is unmounted (going back) the app crashes.

https://user-images.githubusercontent.com/6757047/151558235-39b9a8b5-be73-4c31-8053-02ce188637b8.mp4

crash log
```
2022-01-28 10:27:52.902 15600-15600/com.mattermost.rnbeta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.mattermost.rnbeta, PID: 15600
    java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.DisplayCutout android.view.WindowInsets.getDisplayCutout()' on a null object reference
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.checkForKeyboardEvents(ReactRootView.java:778)
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.onGlobalLayout(ReactRootView.java:769)
        at android.view.ViewTreeObserver.dispatchOnGlobalLayout(ViewTreeObserver.java:1061)
        at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3214)
        at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2143)
        at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8665)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1037)
        at android.view.Choreographer.doCallbacks(Choreographer.java:845)
        at android.view.Choreographer.doFrame(Choreographer.java:780)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1022)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loopOnce(Looper.java:201)
        at android.os.Looper.loop(Looper.java:288)
        at android.app.ActivityThread.main(ActivityThread.java:7839)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
```

After applying the patch which is only a null check validation and does not change any previous behavior

https://user-images.githubusercontent.com/6757047/151558429-9ff1a608-abb6-4168-8db9-df0c3c71d79e.mp4

Reviewed By: cortinico

Differential Revision: D33844955

Pulled By: ShikaSD

fbshipit-source-id: ed5579ad3afeed009c61cc1851eee45c70087cf5
ShikaSD pushed a commit that referenced this pull request Jan 31, 2022
Summary:
Fixes a potential crash was introduced by #30919 that aimed to get the keyboard height on devices with a Notch. The problem is that it considers that any ReactRootView will have an insets available.

When using [react-native-navigation](https://github.com/wix/react-native-navigation) and assigning a Navigation button to the TopBar as a component, the component gets registered as a RootView but won't have any insets attach to the view.

[getRootWindowInsets()](https://developer.android.com/reference/android/view/View#getRootWindowInsets()) in fact return a `WindowInset` only available if the view is attached, so when executing `checkForKeyboardEvents` method from ReactRootView, is trying to access the `DisplayCutout` of a null object, leading to a crash.

## Changelog

[Android] [Fixed] - Fix potential crash if ReactRootView does not have insets attached.

Pull Request resolved: #32989

Test Plan:
Without the code change: Notice how the second screen being push contains a React Component on the top right of the navigation bar, and when component is unmounted (going back) the app crashes.

https://user-images.githubusercontent.com/6757047/151558235-39b9a8b5-be73-4c31-8053-02ce188637b8.mp4

crash log
```
2022-01-28 10:27:52.902 15600-15600/com.mattermost.rnbeta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.mattermost.rnbeta, PID: 15600
    java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.DisplayCutout android.view.WindowInsets.getDisplayCutout()' on a null object reference
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.checkForKeyboardEvents(ReactRootView.java:778)
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.onGlobalLayout(ReactRootView.java:769)
        at android.view.ViewTreeObserver.dispatchOnGlobalLayout(ViewTreeObserver.java:1061)
        at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3214)
        at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2143)
        at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8665)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1037)
        at android.view.Choreographer.doCallbacks(Choreographer.java:845)
        at android.view.Choreographer.doFrame(Choreographer.java:780)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1022)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loopOnce(Looper.java:201)
        at android.os.Looper.loop(Looper.java:288)
        at android.app.ActivityThread.main(ActivityThread.java:7839)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
```

After applying the patch which is only a null check validation and does not change any previous behavior

https://user-images.githubusercontent.com/6757047/151558429-9ff1a608-abb6-4168-8db9-df0c3c71d79e.mp4

Reviewed By: cortinico

Differential Revision: D33844955

Pulled By: ShikaSD

fbshipit-source-id: ed5579ad3afeed009c61cc1851eee45c70087cf5
ShikaSD pushed a commit that referenced this pull request Feb 3, 2022
Summary:
Fixes a potential crash was introduced by #30919 that aimed to get the keyboard height on devices with a Notch. The problem is that it considers that any ReactRootView will have an insets available.

When using [react-native-navigation](https://github.com/wix/react-native-navigation) and assigning a Navigation button to the TopBar as a component, the component gets registered as a RootView but won't have any insets attach to the view.

[getRootWindowInsets()](https://developer.android.com/reference/android/view/View#getRootWindowInsets()) in fact return a `WindowInset` only available if the view is attached, so when executing `checkForKeyboardEvents` method from ReactRootView, is trying to access the `DisplayCutout` of a null object, leading to a crash.

## Changelog

[Android] [Fixed] - Fix potential crash if ReactRootView does not have insets attached.

Pull Request resolved: #32989

Test Plan:
Without the code change: Notice how the second screen being push contains a React Component on the top right of the navigation bar, and when component is unmounted (going back) the app crashes.

https://user-images.githubusercontent.com/6757047/151558235-39b9a8b5-be73-4c31-8053-02ce188637b8.mp4

crash log
```
2022-01-28 10:27:52.902 15600-15600/com.mattermost.rnbeta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.mattermost.rnbeta, PID: 15600
    java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.DisplayCutout android.view.WindowInsets.getDisplayCutout()' on a null object reference
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.checkForKeyboardEvents(ReactRootView.java:778)
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.onGlobalLayout(ReactRootView.java:769)
        at android.view.ViewTreeObserver.dispatchOnGlobalLayout(ViewTreeObserver.java:1061)
        at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3214)
        at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2143)
        at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8665)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1037)
        at android.view.Choreographer.doCallbacks(Choreographer.java:845)
        at android.view.Choreographer.doFrame(Choreographer.java:780)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1022)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loopOnce(Looper.java:201)
        at android.os.Looper.loop(Looper.java:288)
        at android.app.ActivityThread.main(ActivityThread.java:7839)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
```

After applying the patch which is only a null check validation and does not change any previous behavior

https://user-images.githubusercontent.com/6757047/151558429-9ff1a608-abb6-4168-8db9-df0c3c71d79e.mp4

Reviewed By: cortinico

Differential Revision: D33844955

Pulled By: ShikaSD

fbshipit-source-id: ed5579ad3afeed009c61cc1851eee45c70087cf5
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 5, 2023
…ok#32989)

Summary:
Fixes a potential crash was introduced by facebook#30919 that aimed to get the keyboard height on devices with a Notch. The problem is that it considers that any ReactRootView will have an insets available.

When using [react-native-navigation](https://github.com/wix/react-native-navigation) and assigning a Navigation button to the TopBar as a component, the component gets registered as a RootView but won't have any insets attach to the view.

[getRootWindowInsets()](https://developer.android.com/reference/android/view/View#getRootWindowInsets()) in fact return a `WindowInset` only available if the view is attached, so when executing `checkForKeyboardEvents` method from ReactRootView, is trying to access the `DisplayCutout` of a null object, leading to a crash.

## Changelog

[Android] [Fixed] - Fix potential crash if ReactRootView does not have insets attached.

Pull Request resolved: facebook#32989

Test Plan:
Without the code change: Notice how the second screen being push contains a React Component on the top right of the navigation bar, and when component is unmounted (going back) the app crashes.

https://user-images.githubusercontent.com/6757047/151558235-39b9a8b5-be73-4c31-8053-02ce188637b8.mp4

crash log
```
2022-01-28 10:27:52.902 15600-15600/com.mattermost.rnbeta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.mattermost.rnbeta, PID: 15600
    java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.DisplayCutout android.view.WindowInsets.getDisplayCutout()' on a null object reference
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.checkForKeyboardEvents(ReactRootView.java:778)
        at com.facebook.react.ReactRootView$CustomGlobalLayoutListener.onGlobalLayout(ReactRootView.java:769)
        at android.view.ViewTreeObserver.dispatchOnGlobalLayout(ViewTreeObserver.java:1061)
        at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3214)
        at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:2143)
        at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8665)
        at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1037)
        at android.view.Choreographer.doCallbacks(Choreographer.java:845)
        at android.view.Choreographer.doFrame(Choreographer.java:780)
        at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1022)
        at android.os.Handler.handleCallback(Handler.java:938)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loopOnce(Looper.java:201)
        at android.os.Looper.loop(Looper.java:288)
        at android.app.ActivityThread.main(ActivityThread.java:7839)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
```

After applying the patch which is only a null check validation and does not change any previous behavior

https://user-images.githubusercontent.com/6757047/151558429-9ff1a608-abb6-4168-8db9-df0c3c71d79e.mp4

Reviewed By: cortinico

Differential Revision: D33844955

Pulled By: ShikaSD

fbshipit-source-id: ed5579ad3afeed009c61cc1851eee45c70087cf5
facebook-github-bot pushed a commit that referenced this pull request Sep 10, 2024
Summary:
## Summary

This PR bumps Flow all the way to the latest 0.245.2.

Most of the suppressions comes from Flow v0.239.0's change to include
undefined in the return of `Array.pop`.

I also enabled `react.custom_jsx_typing=true` and added custom jsx
typing to match the old behavior that `React.createElement` is
effectively any typed. This is necessary since various builtin
components like `React.Fragment` is actually symbol in the React repo
instead of `React.AbstractComponent<...>`. It can be made more accurate
by customizing the `React$CustomJSXFactory` type, but I will leave it to
the React team to decide.

## How did you test this change?

`yarn flow` for all the renderers

DiffTrain build for commit facebook/react@e210d08.

Reviewed By: yungsters

Differential Revision: D62384646

Pulled By: SamChou19815

fbshipit-source-id: 727794f2d4091f37d771854b8e8a52f070309213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: React Native Team Attention Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
8 participants