-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Keyboard.addListener('keyboardDidShow', ...) callback is invoked with inaccurate measures in Android devices with notch #27089
Comments
This comment has been minimized.
This comment has been minimized.
I've also observed a similar issue between devices (in this case, Google Pixel 3 and Essential Phone) |
Do these devices have notches? I think this might be the same as this: #26296 |
Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions. |
Has anyone been able to reproduce the issue with the latest RN release? |
We haven't upgraded yet, so I haven't been able to test. Your demo repo should be easy to upgrade and test though. |
we need OnePlus 7 so this issue will not be solved... I don't have such device OnePlus 7 |
OK I am gooing to test soon. |
@jsamr I wrote a pull request #29292 that fixes the same issue with event react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java Lines 671 to 677 in cfa4260
you can build from source, change the above source code with either numbers like This is the definition of react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java Lines 755 to 770 in cfa4260
Those values are returned to the additionally there is no OxygenOS emulator https://stackoverflow.com/a/43549663/11452286 |
i have this issue on oneplus 6, i need to add exactly 30dp for it to be perfect. On the iOS simulator it's the other way around, i need to subtract some value, and the value is not always the same ( looks like it's -35 for iphones without home button and 0 for the others - maybe the reported keyboard height doesn't take into account the gesture bar which expands slightly when keyboard is opened? ) |
…s=true (#29292) Summary: This issue fixes #27526, related issue #27089 Avoid returning the wrong screenY coordinates on event keyboardDidHide. getWindowVisibleDisplayFrame retrieves the overall visible display size in which the window this view is attached to has been positioned in. This takes into account screen decorations above the window, for both cases where the window itself is being position inside of them or the window is being placed under then and covered insets are used for the window to position its content inside. In effect, this tells you the available area where content can be placed and remain visible to users, since anything below the StatusBar is not visible to the user, the method does not work with translucent StatusBar (android:windowTranslucentStatus). This commit fixes this issues removing the white bar at the bottom of the view when using windowTranslucentStatus. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - keyboardDidHide wrong screenY coordinates with windowTranslucentStatus=true Pull Request resolved: #29292 Test Plan: Works in all scenarios, but **I did not change RNTester `windowTranslucentStatus`**. I would like to discuss the potential breaking changes connected to not using [getWindowVisibleDisplayFrame](https://developer.android.com/reference/android/view/View#getWindowVisibleDisplayFrame(android.graphics.Rect)) with `keyboardDidHide`. I would be happy to build an alternative functionality to calculate the WindowVisibleDisplayFrameHeight under request from the Facebook Team. **WITHOUT** `android:windowTranslucentStatus` | **BEFORE** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804255-0eca4680-c077-11ea-8c79-95ba297d05ba.gif" />| | **AFTER** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804265-10940a00-c077-11ea-8cb8-a304cc5de363.gif" /> | **WITH** `android:windowTranslucentStatus` | **BEFORE** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804458-3a4d3100-c077-11ea-8f3e-7c43eaa08d70.gif" height="" />| | **BEFORE (log)** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86806330-0d018280-c079-11ea-9266-c3bcf23a35da.png" height="" />| | **AFTER** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804464-3b7e5e00-c077-11ea-8487-34c87f076e5f.gif" height="" /> | | **AFTER (log)** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86806042-c4e26000-c078-11ea-9c3c-cac5319bef65.png" height="" /> | RNTester **WITHOUT** `android:windowTranslucentStatus` | **BEFORE** | **AFTER** | |:-------------------------:|:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86805348-176f4c80-c078-11ea-8e2a-e2d84af5c278.gif" width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/86805324-13432f00-c078-11ea-9e70-fa2606eee86b.gif" width="300" height="" /> | I remain available to do improvements. Thanks a lot. Fabrizio. Reviewed By: JoshuaGross Differential Revision: D22521125 Pulled By: mdvacca fbshipit-source-id: e2cb90163abb1baa00b1916e431971b011522565
…s=true (#29292) Summary: This issue fixes #27526, related issue #27089 Avoid returning the wrong screenY coordinates on event keyboardDidHide. getWindowVisibleDisplayFrame retrieves the overall visible display size in which the window this view is attached to has been positioned in. This takes into account screen decorations above the window, for both cases where the window itself is being position inside of them or the window is being placed under then and covered insets are used for the window to position its content inside. In effect, this tells you the available area where content can be placed and remain visible to users, since anything below the StatusBar is not visible to the user, the method does not work with translucent StatusBar (android:windowTranslucentStatus). This commit fixes this issues removing the white bar at the bottom of the view when using windowTranslucentStatus. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fixed] - keyboardDidHide wrong screenY coordinates with windowTranslucentStatus=true Pull Request resolved: #29292 Test Plan: Works in all scenarios, but **I did not change RNTester `windowTranslucentStatus`**. I would like to discuss the potential breaking changes connected to not using [getWindowVisibleDisplayFrame](https://developer.android.com/reference/android/view/View#getWindowVisibleDisplayFrame(android.graphics.Rect)) with `keyboardDidHide`. I would be happy to build an alternative functionality to calculate the WindowVisibleDisplayFrameHeight under request from the Facebook Team. **WITHOUT** `android:windowTranslucentStatus` | **BEFORE** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804255-0eca4680-c077-11ea-8c79-95ba297d05ba.gif" />| | **AFTER** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804265-10940a00-c077-11ea-8cb8-a304cc5de363.gif" /> | **WITH** `android:windowTranslucentStatus` | **BEFORE** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804458-3a4d3100-c077-11ea-8f3e-7c43eaa08d70.gif" height="" />| | **BEFORE (log)** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86806330-0d018280-c079-11ea-9266-c3bcf23a35da.png" height="" />| | **AFTER** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86804464-3b7e5e00-c077-11ea-8487-34c87f076e5f.gif" height="" /> | | **AFTER (log)** | |:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86806042-c4e26000-c078-11ea-9c3c-cac5319bef65.png" height="" /> | RNTester **WITHOUT** `android:windowTranslucentStatus` | **BEFORE** | **AFTER** | |:-------------------------:|:-------------------------:| | <img src="https://user-images.githubusercontent.com/24992535/86805348-176f4c80-c078-11ea-8e2a-e2d84af5c278.gif" width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/86805324-13432f00-c078-11ea-9e70-fa2606eee86b.gif" width="300" height="" /> | I remain available to do improvements. Thanks a lot. Fabrizio. Reviewed By: JoshuaGross Differential Revision: D22521125 Pulled By: mdvacca fbshipit-source-id: e2cb90163abb1baa00b1916e431971b011522565
I have updated the reproduction to React Native 0.63.3 and it consistently fails on any device with a notch, including the Pixel 3XL in emulator. I noticed that the underestimated height consistently corresponds to the height of |
not releated to my commit which for my understanding is still in master. Hopefully gets fixed by https://github.com//pull/29292. Thanks |
@fabriziobertoglio1987 I thought your PR was only about keyboardDidHide? |
react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java Lines 757 to 758 in d792121
Without Notch With Notch CLICK TO OPEN TESTS RESULTS
Without Notch
With Notch
My conclusions are:
I believe this is not a bug in react-native. |
@fabriziobertoglio1987 Which version / branch are you using to perform those tests? |
@fabianeichinger I've updated the example here: https://github.com/jsamr/react-native-oneplus7-keyboard-height-discrepancy
I haven't dig into the native java code, and I am not familiar with Android UI APIs. But perhaps what you're showing is actually the fixed version on master. |
Thanks for your feedback @jsamr I allways work with the updated react-native master branch My main point in message #27089 (comment) was that the keyboard height was not causing this issue, but the presence of a Notch or No Notch Now you include different scenarios with different status bars.. I can not say about that, but I can say that The screenshots you include are:
I would avoid to add more troubleshooting scenarios, but actually to limit the number of scenarios and identify the bug What is causing your issue? Maybe you think the computation of the Keyboard Height is wrong? This does not make sense, because if I remove the Notch it works perfectly, while the Keyboard Height computation does not change.
|
As I understand it, the height of the keyboard, as reported in the event object, changes based on the presence of a notch. In fact, looking at your test screenshots, you can see this exact issue: in the case with no notch, the height is reported as 282dp; in the case with a notch, it reports 234dp! That would mean the height of the keyboard changed, but it doesn't look like that's the case. |
@fabianeichinger The goal of the screenshots is to compare with notch (Pixel 3XL) and without notch (Nexus 5X). With notch, the red vertical bar height doesn't match the keyboard's height. Without, it does.
That was because I used android 10 in the Pixel and android 11 in the nexus. I have fixed that issue and updated the screenshots. But anyway, the relevant part is that it seems that having a notch causes |
Thanks, you are both right. I believe the best solution is using the Android API to get the Keyboard height instead of calculating it as the difference react-native/ReactAndroid/src/main/java/com/facebook/react/ReactRootView.java Lines 757 to 758 in d792121
I'll work on this tomorrow 🙏 |
@fabianeichinger Thank you so much for your great involvement in this open source project, looking forwards for your take on this! |
Thanks a lot @jsamr My ambition is to really get to know the reactnative sourcecode and hopefully one day give relevant contributions to the community The issue is caused by
This computation is wrong and does not take in consideration the Notch or other screen decorations, as you can see below does not change the total height when using the notch. I found a workaround to fix this by calculating the notch height and substracting it from the above calculation. I will publish my pr soon. I researched and it is common practice on Android Sdk to calculate keyboard height by deducting it from other parameters
MORE DETAILS
Without Notch
With Notch
/**
* Retrieve the overall visible display size in which the window this view is
* attached to has been positioned in. This takes into account screen
* decorations above the window, for both cases where the window itself
* is being position inside of them or the window is being placed under
* then and covered insets are used for the window to position its content
* inside. In effect, this tells you the available area where content can
* be placed and remain visible to users.
*
* <p>This function requires an IPC back to the window manager to retrieve
* the requested information, so should not be used in performance critical
* code like drawing.
*
* @param outRect Filled in with the visible display frame. If the view
* is not attached to a window, this is simply the raw display size.
*/
public void getWindowVisibleDisplayFrame(Rect outRect) {
if (mAttachInfo != null) {
try {
mAttachInfo.mSession.getDisplayFrame(mAttachInfo.mWindow, outRect);
} catch (RemoteException e) {
return;
}
// XXX This is really broken, and probably all needs to be done
// in the window manager, and we need to know more about whether
// we want the area behind or in front of the IME.
final Rect insets = mAttachInfo.mVisibleInsets;
outRect.left += insets.left;
outRect.top += insets.top;
outRect.right -= insets.right;
outRect.bottom -= insets.bottom;
return;
}
// The view is not attached to a display so we don't have a context.
// Make a best guess about the display size.
Display d = DisplayManagerGlobal.getInstance().getRealDisplay(Display.DEFAULT_DISPLAY);
d.getRectSize(outRect);
} |
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
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
Incriminated API
Prerequisites
In
AndroidManifest.xml
, setandroid:windowSoftInputMode="adjustPan"
.Visual tests
The black horizontal bar should be the same width (40dp) as the red bar, in a container with a bottom padding of the keyboard's height. This component mimics KeyboardAvoidingView with behavior padding set. The height of keyboard should be the same as the height of the red bar. The issue has been reproduced with GBoard, SwiftKey...
Remarks: I would have imagined that the view height would equal keyboard height + keyboard Y. But that is only true ... on OnePlus7, which is the problematic device!
React Native version:
Steps To Reproduce
KeyboardAvoidingView
with behavior padding doesn't add enough padding.Describe what you expected to happen: The padding should match keyboard's real height.
Full reproduction : react-native-oneplus7-keyboard-height-discrepancy
Observations
StatusBar.currentHeight
is substituted to the keyboard layout end frameheight
.StatusBar.currentHeight
is also substituted from keyboard layout end framescreenY
The text was updated successfully, but these errors were encountered: