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 phones with hidden bottom navbar have issues clicking bottom pixels because ModalStack has wrong translateY value and cover some of bottom content #42

Closed
tarasvakulka opened this issue Apr 9, 2021 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@tarasvakulka
Copy link

I reproduced issue that Android phones with hidden bottom navbar have issues clicking bottom pixels because ModalStack has wrong translateY value (see screenshot) and cover some of bottom content. I think problem with wrong Dimensions.get('window').height value on some android devices when native bottom navbar is hidden. In my case hidden android navbar programmatically in MainActivity.java:
@OverRide
public void onWindowFocusChanged(boolean hasFocus) {
super.onWindowFocusChanged(hasFocus);
if (hasFocus) {
getWindow().getDecorView().setSystemUiVisibility(
View.SYSTEM_UI_FLAG_LAYOUT_STABLE
| View.SYSTEM_UI_FLAG_LAYOUT_HIDE_NAVIGATION
| View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN
| View.SYSTEM_UI_FLAG_HIDE_NAVIGATION
| View.SYSTEM_UI_FLAG_FULLSCREEN
| View.SYSTEM_UI_FLAG_IMMERSIVE_STICKY);
}
}
inspector_screenshot

Test device: Huawei P smart 2019 (POT-LX1) Android 9
"react": "16.13.1",
"react-native": "0.63.4",
"react-native-modalfy": "2.1.1",
"react-native-gesture-handler": "1.6.1"

@jehartzog
Copy link

We are seeing this issue too. Confirmed reproduction: On Android, the bottom ~40 pixels on the screen are completely untouchable, even using the inspector. That made troubleshooting this pretty tricky. Disabling this lib corrects the issue.

It varies depending on the device, and the home button settings on the OS. An older pixel 2 XL with fixed home/back buttons does not show the issue. But a OnePlus 8T & Pixel 5 w/ gesture based home buttons expose this issue.

We're disabling this lib in our app for now, but will likely evaluate for a fix later.

@jehartzog
Copy link

We confirmed #43 fixes the issue for us.

@tarasvakulka
Copy link
Author

We confirmed #43 fixes the issue for us.

Thanks for confirmation. I will hope somebody review this PR soon.

@iremlopsum
Copy link
Collaborator

I think the main issue with the PR is that it requires a new native library, which is something that we wouldn't want to do.

Would be cool to find a way of getting the correct values without having to introduce a native dependency.

@jehartzog
Copy link

@iremlopsum I completely agree there, it's not ideal to introduce a dep on a third party native lib. On the other hand, I've hit this exact issue across many different places in many different apps. It's a bit shocking that there isn't a major, standard reliable way to get the true screen height on Android in react-native right now, but in all the code I've had to ship to prod, we've had to include the react-native-extra-dimensions-android lib that @tarasvakulka used to solve the issue here.

One comment though, I definitely wouldn't PR this in as specific version in package.json dependencies. I would leave as a loose version inside peerDependencies and update installation instructions for this lib. That way someone who already has the lib doesn't end up attempting to install two different versions of the same native code, or have to patch this lib to override it.

@jehartzog
Copy link

jehartzog commented May 7, 2021

If you really wanted to avoid including the lib by default, but provide a easy fix for users who hit this, we could modify the PR to make it an optional dependency. If the react-native-extra-dimensions-android is there, it will be used, otherwise the standard code will be used. Add a note in the docs about having the add the lib if a user sees this issue, and everyone should be happy enough?

If you want this, LMK and I'd be happy to extend the PR to do this.

@CharlesMangwa CharlesMangwa self-assigned this May 8, 2021
@CharlesMangwa CharlesMangwa added bug Something isn't working repro needed Reproducible demo app/Expo Snack is needed labels May 8, 2021
@CharlesMangwa
Copy link
Member

CharlesMangwa commented May 8, 2021

Hi @tarasvakulka @jehartzog and thanks for your inputs regarding this issue!

Would it be possible for you to provide a reproducible demo on Expo Snack please? We haven't been able to encounter this issue ourselves so far. Without that repro, it would be hard for us to review the PR and confirm that the issue is properly fixed.

Regarding react-native-extra-dimensions-android, if at the end we happen to need it, given that the library would rely on it to work, I also think we'd have to put it inside the peerDependencies (basing myself on this section of the node blog).

@tarasvakulka
Copy link
Author

Hi @tarasvakulka @jehartzog and thanks for your inputs regarding this issue!

Would it be possible for you to provide a reproducible demo on Expo Snack please? We haven't been able to encounter this issue ourselves so far. Without that repro, it would be hard for us to review the PR and confirm that the issue is properly fixed.

Regarding react-native-extra-dimensions-android, if at the end we happen to need it, given that the library would rely on it to work, I also think we'd have to put it inside the peerDependencies (basing myself on this section of the node blog).

Example to reproduce: https://snack.expo.io/@tarasvakulka/github.com-tarasvakulka-rn-modalfy-example
If you press on bottom part of green button you will see that it isn't touchable. But if you comment ModalProvider in App.js all works fine.

Test device: Huawei P smart 2019 (POT-LX1) Android 9

P.S. I don't understand why but if you run my example from Expo Snack "androidNavigationBar": { "visible": "sticky-immersive"} has no effect to nav bar, but issue is reproducing. Therefore it is better to clone my repo https://github.com/tarasvakulka/rn-modalfy-example.git and run npm start from command line.

@CharlesMangwa
Copy link
Member

Thanks so much for the repro @tarasvakulka! It actually helped me realised I was looking for the issue at the wrong place!

I may have found an easy fix: could you let me know if making these changes in /node_modules/react-native-modalfy/lib/ModalStack.tsx fixes it for you?

// #L2
import {
   TouchableWithoutFeedback,
+  Dimensions,
   StyleSheet,
   Animated,
   Easing,
} from 'react-native'
// #L40
- translateY: new Animated.Value(vh(100)),
+ translateY: new Animated.Value(Dimensions.get('screen').height),
// #L80
- translateY.setValue(vh(100))
+ translateY.setValue(Dimensions.get('screen').height)

@jehartzog If you also happen to have a minute to check this, it would be great!

@tarasvakulka
Copy link
Author

Thanks so much for the repro @tarasvakulka! It actually helped me realised I was looking for the issue at the wrong place!

I may have found an easy fix: could you let me know if making these changes in /node_modules/react-native-modalfy/lib/ModalStack.tsx fixes it for you?

// #L2
import {
   TouchableWithoutFeedback,
+  Dimensions,
   StyleSheet,
   Animated,
   Easing,
} from 'react-native'
// #L40
- translateY: new Animated.Value(vh(100)),
+ translateY: new Animated.Value(Dimensions.get('screen').height),
// #L80
- translateY.setValue(vh(100))
+ translateY.setValue(Dimensions.get('screen').height)

@jehartzog If you also happen to have a minute to check this, it would be great!

Yes it fixes. Dimensions.get('screen').height absolutely enough. I have use ExtraDimensions out of habit. Thanks for great and easy solution without a third party native lib.

@jehartzog
Copy link

jehartzog commented May 10, 2021

@CharlesMangwa I've confirmed your solution fixes the issue in our build as well.

@CharlesMangwa CharlesMangwa removed the repro needed Reproducible demo app/Expo Snack is needed label May 11, 2021
@CharlesMangwa
Copy link
Member

Thanks for confirming @tarasvakulka @jehartzog! Closing this issue as I've just published v2.1.2 which includes this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants