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

[Android] keyboardDidHide wrong screenY coordinates with windowTranslucentStatus=true #29292

Closed

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Jul 7, 2020

Summary

This issue fixes #27526 fixes #30052, 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

[Android] [Fixed] - keyboardDidHide wrong screenY coordinates with windowTranslucentStatus=true

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 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
AFTER

WITH android:windowTranslucentStatus

BEFORE
BEFORE (log)
AFTER
AFTER (log)

RNTester WITHOUT android:windowTranslucentStatus

BEFORE AFTER

I remain available to do improvements. Thanks a lot. Fabrizio.

@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 Jul 7, 2020
@fabOnReact fabOnReact changed the title keyboardDidHide wrong screenY coordinates with windowTranslucentStatus=true [Android] keyboardDidHide wrong screenY coordinates with windowTranslucentStatus=true Jul 7, 2020
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Jul 7, 2020
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,763,750 107
android hermes armeabi-v7a 6,426,685 83
android hermes x86 7,151,383 100
android hermes x86_64 7,041,303 100
android jsc arm64-v8a 8,935,703 100
android jsc armeabi-v7a 8,591,045 98
android jsc x86 8,766,446 85
android jsc x86_64 9,342,008 100

Base commit: e23e932

@analysis-bot
Copy link

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

Base commit: e23e932

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@mdvacca
Copy link
Contributor

mdvacca commented Jul 14, 2020

Importing to review

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fabriziobertoglio1987 in 45954ac.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 15, 2020
kelset pushed a commit that referenced this pull request Sep 29, 2020
…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
@fabOnReact
Copy link
Contributor Author

Thanks a lot David Vacca 🙏

@andres-torres-marroquin

This fix breaks the keyboardDidHide event in Android by sending screenY a value that is not correct, it basically crops the view at about 90% of the original height when the keyboard is hidden. I've tested 0.63.3 and 0.63.2, and the 0.63.2 works as intended. I'm using react-native-navigation with TopBar and BottomTabs, if that helps.

I just want to be sure if this change is actually correct?

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Jan 20, 2021

@andres-torres-marroquin I'll check into it this weekend, could you open and issue in react-native and reference this pr?
The issues needs to have a minimum reproducible example. Thanks a lot 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
6 participants