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

Possible regression on #29292 #30770

Closed
andres-torres-marroquin opened this issue Jan 20, 2021 · 13 comments
Closed

Possible regression on #29292 #30770

andres-torres-marroquin opened this issue Jan 20, 2021 · 13 comments
Labels
API: Keyboard Platform: Android Android applications. Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@andres-torres-marroquin

Description

The issue #29292 makes a regression on the keyboardDidHide behavior for the
Android platform, if it happens that you have multiple ReactRootViews on the
same application at once, the keyboardDidHide event happens for every ReactRootView
and the last event data will prevail.

Steps To Reproduce

Having multiple ReactRootViews is specially common using react-native-navigation,
in this example we provide two ReactRootView, one as main canvas (in red) and one on the
top right screen border (in blue), the event keyboardDidHide is triggered twice,
where in the first trigger it sends the "right" value, because it is being triggered
by the main canvas (in red), and on the second trigger it sends the "wrong" value, since
it is being triggered by the top right screen view (in blue).

Comparing this behavior with the iOS counterpart the event is triggered only once
and it sends the right value, for this exact same code.

Snack, code example, screenshot, or link to a repository:

https://github.com/andres-torres-marroquin/RN29292

@safaiyeh
Copy link
Contributor

@fabriziobertoglio1987 could you take a look at this? The issue is referencing a PR you a made a while ago.

@safaiyeh safaiyeh added API: Keyboard Platform: Android Android applications. labels Jan 24, 2021
@fabOnReact
Copy link
Contributor

fabOnReact commented Jan 25, 2021

@safaiyeh thanks. Yes, I will reproduce the issue and try to find a solution during the weekend. Thanks a lot 🙏 ☮️

Hopefully I'll find an android method to get the correct height as some libraries like react-native-navigation use ReactRootView

@fabOnReact
Copy link
Contributor

Thanks a lot @andres-torres-marroquin
My observations are the following

  • my commit was introduced in 0.63.3
  • I tested your example on both 0.63.4 and 0.63.2 and can not see any (visual) difference in behaviour in the KeyboardDidHide functionality
  • Unluckily I do not know or contribute to react-native-navigation
  • Seems to me that the functionality behaves in the same way with/out react-native-navigation

I tested in my repo with react-native 0.63.2. My commit was introduced with 0.63.3

Full Screen After Keyboard Did Hide

The results of the tests from your repo

Full Screen After Keyboard Did Hide

I remain available and I'm sorry if I misunderstood or ignore this issue. I am willing to work during the weekends to fix this regression. Thanks a lot 🙏 Fabrizio

@andres-torres-marroquin
Copy link
Author

Hi @fabriziobertoglio1987, thanks for you reply.

Unfortunately, both examples don't show keyboardDidHide, they do show keyboardDidShow.

Full Screen

keyboardDidShow

keyboardDidHide

I also emphasize that it happens when you have more than one ReactRootView on Android, on my repo there are two, the red one and the blue one. It won't happen on your repo since there you have only one ReactRootView.

@fabOnReact
Copy link
Contributor

fabOnReact commented Feb 2, 2021

Thanks @andres-torres-marroquin

can you override in the 2nd child ReactRootView?

private void checkForKeyboardEvents() {
getRootView().getWindowVisibleDisplayFrame(mVisibleViewArea);
final int heightDiff =
DisplayMetricsHolder.getWindowDisplayMetrics().heightPixels - mVisibleViewArea.bottom;
boolean isKeyboardShowingOrKeyboardHeightChanged =
mKeyboardHeight != heightDiff && heightDiff > mMinKeyboardHeightDetected;
if (isKeyboardShowingOrKeyboardHeightChanged) {
mKeyboardHeight = heightDiff;
sendEvent(
"keyboardDidShow",
createKeyboardEventPayload(
PixelUtil.toDIPFromPixel(mVisibleViewArea.bottom),
PixelUtil.toDIPFromPixel(mVisibleViewArea.left),
PixelUtil.toDIPFromPixel(mVisibleViewArea.width()),
PixelUtil.toDIPFromPixel(mKeyboardHeight)));
return;
}
boolean isKeyboardHidden = mKeyboardHeight != 0 && heightDiff <= mMinKeyboardHeightDetected;
if (isKeyboardHidden) {
mKeyboardHeight = 0;
sendEvent(
"keyboardDidHide",
createKeyboardEventPayload(
PixelUtil.toDIPFromPixel(mLastHeight),
0,
PixelUtil.toDIPFromPixel(mVisibleViewArea.width()),
0));
}
}

with

private void checkForKeyboardEvents() {
getRootView().getWindowVisibleDisplayFrame(mVisibleViewArea);
final int heightDiff =
DisplayMetricsHolder.getWindowDisplayMetrics().heightPixels - mVisibleViewArea.bottom;
boolean isKeyboardShowingOrKeyboardHeightChanged =
mKeyboardHeight != heightDiff && heightDiff > mMinKeyboardHeightDetected;
if (isKeyboardShowingOrKeyboardHeightChanged) {
mKeyboardHeight = heightDiff;
sendEvent(
"keyboardDidShow",
createKeyboardEventPayload(
PixelUtil.toDIPFromPixel(mVisibleViewArea.bottom),
PixelUtil.toDIPFromPixel(mVisibleViewArea.left),
PixelUtil.toDIPFromPixel(mVisibleViewArea.width()),
PixelUtil.toDIPFromPixel(mKeyboardHeight)));
return;
}
boolean isKeyboardHidden = mKeyboardHeight != 0 && heightDiff <= mMinKeyboardHeightDetected;
if (isKeyboardHidden) {
mKeyboardHeight = 0;
sendEvent(
"keyboardDidHide",
createKeyboardEventPayload(
PixelUtil.toDIPFromPixel(mVisibleViewArea.height()),
0,
PixelUtil.toDIPFromPixel(mVisibleViewArea.width()),
0));
}
}

like something (sorry if it not correct java logic):

class ChildRootView extends ReactRootView {
   @override
   private void checkForKeyboardEvents() {
       // copy paste and change the logic
   }
}

@andres-torres-marroquin
Copy link
Author

andres-torres-marroquin commented Feb 2, 2021

I cannot override that 2nd view, that is handled by react-native-navigation under the hood.

P.S. If I were able to do it, for sure it would work.

@fabOnReact
Copy link
Contributor

fabOnReact commented Feb 2, 2021

I'll send pr to react-native-navigation to override checkForKeyboardEvents in ReactView, if those are the views used for this scenario.

public class ReactView extends ReactRootView implements IReactView, Renderable {
    @Override
    protected void checkForKeyboardEvents() {
        super.checkForKeyboardEvents();
        // copy below logic
    }
 }

https://github.com/wix/react-native-navigation/blob/15c7a7e799f3cff9ad83b782dccc92aa1fe8a523/lib/android/app/src/main/java/com/reactnativenavigation/react/ReactView.java#L23-L43

onMeasure should look like this

private void checkForKeyboardEvents() {
getRootView().getWindowVisibleDisplayFrame(mVisibleViewArea);
final int heightDiff =
DisplayMetricsHolder.getWindowDisplayMetrics().heightPixels - mVisibleViewArea.bottom;
boolean isKeyboardShowingOrKeyboardHeightChanged =
mKeyboardHeight != heightDiff && heightDiff > mMinKeyboardHeightDetected;
if (isKeyboardShowingOrKeyboardHeightChanged) {
mKeyboardHeight = heightDiff;
sendEvent(
"keyboardDidShow",
createKeyboardEventPayload(
PixelUtil.toDIPFromPixel(mVisibleViewArea.bottom),
PixelUtil.toDIPFromPixel(mVisibleViewArea.left),
PixelUtil.toDIPFromPixel(mVisibleViewArea.width()),
PixelUtil.toDIPFromPixel(mKeyboardHeight)));
return;
}
boolean isKeyboardHidden = mKeyboardHeight != 0 && heightDiff <= mMinKeyboardHeightDetected;
if (isKeyboardHidden) {
mKeyboardHeight = 0;
sendEvent(
"keyboardDidHide",
createKeyboardEventPayload(
PixelUtil.toDIPFromPixel(mVisibleViewArea.height()),
0,
PixelUtil.toDIPFromPixel(mVisibleViewArea.width()),
0));
}
}

hopefully it will work, otherwise will think about something else. Thanks

@andres-torres-marroquin
Copy link
Author

andres-torres-marroquin commented Feb 2, 2021

@fabriziobertoglio1987 I wonder about the behavior parity between iOS and Android, since on Android you could get multiple calls from keyboard events, if happens that you have more than one ReactRootView, I wonder if there is a way to do it as it is done on iOS, where you get the event triggered just once. (therefore depending of the app, might be a performance hit on Android)

@andres-torres-marroquin
Copy link
Author

@fabriziobertoglio1987 this has become suddenly really important, since there is a bug for uploading files in 0.63.2 for iOS, so we either downgrade even further than 0.62.2, or compile the iOS app with 0.63.4 and the Android app with 0.63.2.

How can this issue get more attention?

@carlosalbertm
Copy link

It's seems that guy doesn't want to help to the community, what a shame

@safaiyeh
Copy link
Contributor

safaiyeh commented Mar 8, 2021

@carlosalbertm comments like that aren't appreciated. @fabriziobertoglio1987 has been an amazing contributor to many React Native issues, help on issues like this would be appreciated from anyone including you 😁

@andres-torres-marroquin i would suggest speaking to the react-native-navigation team for more context! They are very responsive especially in their discord cc @guyca

Copy link

github-actions bot commented Dec 8, 2023

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added Stale There has been a lack of activity on this issue and it may be closed soon. and removed Stale There has been a lack of activity on this issue and it may be closed soon. labels Dec 8, 2023
@cortinico cortinico added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 8, 2023
Copy link

This issue was closed because it has been stalled for 7 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Keyboard Platform: Android Android applications. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

No branches or pull requests

5 participants