colorfy-software / react-native-modalfy

🥞 Modal citizen of React Native.
https://colorfy-software.gitbook.io/react-native-modalfy
MIT License
1.08k stars 44 forks source link

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 closed 3 years ago

tarasvakulka commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

We confirmed #43 fixes the issue for us.

tarasvakulka commented 3 years ago

We confirmed #43 fixes the issue for us.

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

iremlopsum commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

CharlesMangwa commented 3 years ago

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