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.

Sign up for GitHub

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

Jump to bottom

WIP: CB-13300: (Android) Fixed keyboard from overlapping content #128

Closed
wants to merge 7 commits into from
Closed

WIP: CB-13300: (Android) Fixed keyboard from overlapping content #128

wants to merge 7 commits into from

Conversation

breautek
Copy link
Contributor

@breautek breautek commented Feb 4, 2019 •
edited
Loading

Platforms affected

  • Android

Fixes #110

What does this PR do?

What testing has been done on this change?

I've ran npm test command however it looks like that only tests the web side of the codebase. I've also tested manually on an Android 8.1 device.
People over at #110 confirmed the solution also fixed problems they were having.

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@breautek
Copy link
Contributor Author

breautek commented Feb 4, 2019

I left automated test coverage unchecked cause it appears there are not unit tests for native code. So I wasn't sure how this is usually handled.

@breautek breautek mentioned this pull request Feb 4, 2019
Open
@breautek breautek changed the title CB-13300: Fixed keyboard from overlapping content CB-13300: (Android) Fixed keyboard from overlapping content Feb 8, 2019
@pengkobe
Copy link

any updates?

@breautek
Copy link
Contributor Author

Not sure on the typical process on getting PRs merged in...

In the meantime, you can try using my fork
cordova plugin add https://github.com/breautek/cordova-plugin-statusbar.git#issue-110-statusbar-overlay-keyboard-resize

@breautek breautek mentioned this pull request Apr 10, 2019
Open
@breautek
Copy link
Contributor Author

@janpio Who would be the appropriate persons to request review from for android changes?

@purplecabbage
Copy link
Contributor

This looks good to me, any chance you can post before/after screenshots @breautek ?

@breautek
Copy link
Contributor Author

Sure thing @purplecabbage

For context I'll provide 2 before screenshots, and 1 after. This is using cordova-android 8.1.0 and using statusbar version ^2.4.2. The test app I used for these screenshots can be found here This screenshot below is the screen with no keyboard shown. The Textfield is positioned absolutely using CSS bottom: 0px;, so it is always displayed at the bottom edge of the webview.

Screenshot_20190923-152950_test

The next screenshot is when the text field receives focused. Note how the textfield isn't in view. It is impossible to have the textfield in view in this case with how the webview works without this PR changes.

Screenshot_20190923-152954_test

This next screenshot is with this PR statusbar changes. The textfield is now in view. This works because the webview native UI physically is resized to fit the screen real estate. So in this screenshot, the bottom of the webview is exactly where the keyboard starts. I've added a new class StatusBarViewHelper that watches the native UI nodes for geometry changes, and it resizes the webview accordingly so that the webview always fills the screen space minus the status bar (if solid), the navigation bar (for phones without physical buttons), and the keyboard.

Note that if your HTML is configured in a way where you define a fixed height for your document. If the webview resizes smaller than that document size it will make the webview scrollable (assuming you allow it via the overflow css). This is exactly the issue I had as without this PR, the webview would never be scrollable as the webview never resized when the keyboard is displayed.

In theory, if the user has custom keyboards installed with different potentially different sizes, this PR should just work as well.

Screenshot_20190923-153208_test

Additionally this PR makes the android webview behave a lot like the iOS webview, where by default, iOS just seems to handle things more intelligently when the keyboard is opened because they also do resize the webview.

@purplecabbage
Copy link
Contributor

Thanks @breautek! The visuals, and especially the extra explanations are super helpful. I'll try to get this merged this evening.

@breautek
Copy link
Contributor Author

@purplecabbage This should not be merged in yet. I just received an email from someone using my fork and this PR causes issues with devices that has reserved screen spaces for whatever reason, such as the Samsung S10 where they have a camera on screen and have a reserved screen space for that area.

I think that issue should be a blocker for this PR and addressed first.

@armandn
Copy link

armandn commented Sep 27, 2019

screenshots
There may be a problem with the way available space is calculated on Android devices that have notches or punched holes for cameras. With them, the status bar is hidden but the system still keeps a black bar to hide the notch / camera hole.

Tested on two phones, Samsung S8 and S10, same Android version.
On S8, the app uses all available space (left image).
On S10, the status bar is hidden but the system still reserves some black area for the camera and I think this throws off the patch (right image). The original cordova-plugin-statusbar is not affected (middle image)

I received a similar issue from a user with a Doogee P90 phone. I could not test myself, but that phone also has a notch...

@breautek
Copy link
Contributor Author

breautek commented Oct 1, 2019

screenshots
There may be a problem with the way available space is calculated on Android devices that have notches or punched holes for cameras. With them, the status bar is hidden but the system still keeps a black bar to hide the notch / camera hole.

Tested on two phones, Samsung S8 and S10, same Android version.
On S8, the app uses all available space (left image).
On S10, the status bar is hidden but the system still reserves some black area for the camera and I think this throws off the patch (right image). The original cordova-plugin-statusbar is not affected (middle image)

I received a similar issue from a user with a Doogee P90 phone. I could not test myself, but that phone also has a notch...

@armandn I originally thought this was due to the android cutout. I don't have any devices with an actual cutout but I used the developer tools on my phone to simulate a cutout, but I'm unable to reproduce what you're seeing in those screenshots that you have provided. Are you able provide me a reproduction repo that I can use as a test app?

I think that will help me determine if the cutout simulation just doesn't behave the same as devices with an actual cutout, or if I'm just simply not reproducing it right. Thanks.

https://github.com/apache/cordova-contribute/blob/master/create-reproduction.md

@armandn
Copy link

armandn commented Oct 2, 2019 via email

@breautek
Copy link
Contributor Author

breautek commented Oct 2, 2019

Unfortunately the app in question is not open
source and I can't post it.

@armandn Sorry, I didn't mean for you to publish your production app. I mean to create a new app containing the minimal amount of code required to reproduce the issue with the notch and publish that.

If I edit StatusBarViewHelper.java myself, will it be
recompiled automatically when I run cordova build android or do I need
to clear/delete anything to force a recompile?

If you want to experiment with the native code yourself, you can do so by going to:

<project-root>/platforms/android/app/src/main/java/org/apache/cordova/statusbar/

Which is a directory that contains the two java files. You can make edits to those files and simply recompile to see the changes.

If you find a solution to your issue, I'd greatly appreciate it if you could make a PR to my fork so that it can be included in this PR. Obviously you can mark your contribution.

Inside StatusBarViewHelper.java I was picturing changing computeUsableHeight() to something like this:

private int computeUsableHeight() {
        Rect r = new Rect();
        mChildOfContent.getWindowVisibleDisplayFrame(r);
        int uiOptions = activity.getWindow().getDecorView().getSystemUiVisibility();
        boolean isFullscreen = ((uiOptions | View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN) == uiOptions);

        int cutoutTop = 0;
        int cutoutBottom = 0;

        // On devices with screen cutouts, this code will think that screen is available, when in reality it is not.
        // This piece gets the cutout information so that we can correct the computeUsableHeight
        DisplayCutout cutout = mChildOfContent.getDisplayCutout();
        if (cutout != null) {
            cutoutTop = cutout.getSafeInsetTop();
            cutoutBottom = cutout.getSafeInsetBottom();
        }

        int usableHeight = r.bottom - cutoutTop - cutoutBottom;

        //If not fullscreen, then we have to take the status bar into consideration (represented by r.top)
        //r.bottom defines the keyboard, or navigation bar, or both.
        if (isFullscreen) {
            usableHeight = usableHeight - r.top;
        }

        return usableHeight;
    }

But since I don't have a device with notches, and the notch simulation on my devices don't produce the same result as what you're seeing, I don't know if that will fix anything or make it worse, etc.

@armandn
Copy link

armandn commented Oct 2, 2019 via email

@breautek
Copy link
Contributor Author

breautek commented Oct 2, 2019 •
edited
Loading

@armandn Oops, looks like you're right. It should be DisplayCutout cutout = getWindow().getDecorView().getRootWindowInsets().getDisplayCutout();

You can also use Log.v("StatusBar", cutout.toString()) to print a stringified version of the cutout data to the syslog.

If you're on unix, you can log the syslog to a log file by using adb logcat > "android_log.log" (adb obviously needs to be in your path, the executable will be inside your android sdk platform-tools/ directory
If you're on windows, not sure the best way to get the logfile. There is still the adb logcat command, but idk how to redirect it to a file for viewing. adb executable does have a filter, but not sure how to use it off the top of my head.

@armandn
Copy link

armandn commented Oct 3, 2019 via email

@breautek
Copy link
Contributor Author

breautek commented Oct 3, 2019

Good news, inital tests on S10 seem OK. I want to make a few more tests
on various devices to make sure it works everywhere.

Awesome! If you don't mind, please fork my repo and create a PR for me, so that I can include it into this PR. I'll attribute your change.

I also added a BUILD.VERSION test so the cutout is verified only for SKD
28 and above.

Good that you've thought of this. I thought about this before but I forgot to mention it. However starting November 2019 everybody must use API 28+ anyway if they want to publish to the Google Play Store.

Armand Niculescu and others added 2 commits October 3, 2019 16:18
@breautek breautek self-assigned this Oct 4, 2019
@breautek
Copy link
Contributor Author

breautek commented Oct 10, 2019 •
edited
Loading

@armandn FYI I'm not entirely confident that the PR with the cutout is a complete solution. On my devices I don't need the cutout code for getWindowVisibleDisplayFrame to return the correct results. And WITH the cutout code, while simulating a cutout display, it now returns incorrect results. The same issue as you brought up, except in the other direction. My webview is shorter than the available screen space, not above. And this shortage is coming from subtracting the cutout top.

I think the main problem is with the getWindowVisibleDisplayFrame API, I've been reading it returns inconsistent results, and the Google source code even claims that the API is broken...

But it doesn't make much sense that on one device it considers the cutout automatically and on another device it doesn't.

Are you able to provide me a sample reproduction app? Not asking to share your project but to create a new app that produces the original issue. I'm suspecting that perhaps there is another variable at play here.

@breautek breautek changed the title CB-13300: (Android) Fixed keyboard from overlapping content WIP: CB-13300: (Android) Fixed keyboard from overlapping content Oct 10, 2019
@armandn
Copy link

armandn commented Oct 10, 2019

Understood. I'll try to make a sample app. I'll let you know.

@armandn
Copy link

armandn commented Oct 11, 2019

I created a cordova Hello World app and added just the Status Bar plugin. I only added StatusBar.hide() in javascript and placed a big red border on the screen. The issue is still visible.

I created a repo with the full app (including the apk, just in case) and the relevant screenshots (on Samsung S10).

https://github.com/armandn/CordovaStatusBarTests

Sadly, while I have about 7 Android test devices (phones & tablets, Android 6 to 9), they are all Samsung. I wish I had a more varied set.

@breautek
Copy link
Contributor Author

breautek commented Oct 11, 2019 •
edited
Loading

Sadly, while I have about 7 Android test devices (phones & tablets, Android 6 to 9), they are all Samsung. I wish I had a more varied set.

I'm pretty much in the same boat... but with less devices. And none of my devices have a cutout, so I'm relying on the simulated cutout feature... Which might even be the cause of the inconsistent behaviour... 🤷‍♂️

But thanks, hopefully this will produce more consistent results. I'm not sure when I'll get a chance to take a look at this, probably only sometime next week.

@breautek
Copy link
Contributor Author

breautek commented Oct 14, 2019 •
edited
Loading

Just an update, I've reproduced the issue on an emulator, so this is something that I can work with.

The biggest difference is I don't think I was using StatusBar.hide in my testing. So I think what is happening is if we use a cutout while the statusbar is visible, the cutout measurements and getWindowVisibleDisplayFrame will cause the dimensions to double up as getWindowVisibleDisplayFrame includes cutout dimensions, as at least in cutout simulations, the cutout appears to become a part of the statusbar.

However, when you hide the statusbar, then getWindowVisibleDisplayFrame doesn't include that reserved space for the cutout, so the cutout measurement reading becomes necessary in this state.

This also happens on my physical device when using cutout simulations.

@breautek
Copy link
Contributor Author

@armandn In my testing the DisplayCutout always returned 0 results, honestly not quite sure what changed. Which scares me a little... but using your sample app as a test, I managed to update the resizer so that it always produces a good size, at least in the emulator and on my physical device using cutout simulations.

I've tested the statusbar while it is both solid and overlayed (overlaysWebView(true/false)), and when it is visible show() or hidden hide() and it all passed visual inspections on my end.

If you could use update my plugin and retest in your app to confirm on your end, that would be appreciated.

To make sure no old build artefacts are left lingering around you may have to do the following commands:

cordova plugin remove cordova-plugin-statusbar
cordova clean android
cordova plugin add https://github.com/breautek/cordova-plugin-statusbar.git#CB-13300-statusbar-overlay-keyboard-overlap
cordova build android

@breautek breautek force-pushed the CB-13300-statusbar-overlay-keyboard-overlap branch from e5b86a9 to 85a0740 Compare October 16, 2019 02:36
@breautek
Copy link
Contributor Author

PS travis seems to be failing the build for an unrelated reason.

@armandn
Copy link

armandn commented Oct 18, 2019

Following the instructions, I'm getting this error when building:

BUILD FAILED in 27s
[path]\platforms\android\gradlew: Command failed with exit code 1 Error output:
Note: [path]\platforms\android\CordovaLib\src\org\apache\cordova\engine\SystemCookieManager.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
[path]\platforms\android\app\src\main\java\org\apache\cordova\statusbar\StatusBarViewHelper.java:54: error: cannot find symbol
        return statusbar.isVisible();
                        ^
  symbol:   method isVisible()
  location: variable statusbar of type StatusBar
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: [path]\platforms\android\app\src\main\java\org\apache\cordova\file\AssetFilesystem.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error

I don't see any StatusBar class on Android SDK...

@breautek
Copy link
Contributor Author

I don't see any StatusBar class on Android SDK..

It's not from the android sdk, it's the CordovaPlugin reference. It was something that I added, but argh I must have forgotten to copy over the implementation of isVisible from the app I was using to test to the statusbar repo. So sorry for wasting your time. I'll have this corrected later tonight.

@breautek breautek force-pushed the CB-13300-statusbar-overlay-keyboard-overlap branch from 85a0740 to 430e7df Compare October 19, 2019 14:26
@breautek
Copy link
Contributor Author

@armandn that issue should be solved now.

@armandn
Copy link

armandn commented Oct 23, 2019

Sorry for the delay!
Now the plugin compiles and seems to be working great. Thank you so much!

@breautek
Copy link
Contributor Author

@purplecabbage The issue that blocked this has been resolved. This is ready for another review.

@danielehrhardt
Copy link

Please merge!!

@breautek breautek changed the title WIP: CB-13300: (Android) Fixed keyboard from overlapping content CB-13300: (Android) Fixed keyboard from overlapping content Nov 15, 2019
@malinges
Copy link

Any news on this?

@danielehrhardt
Copy link

Any news on this?

https://github.com/codextde/cordova-plugin-statusbar-color-fix

@breautek breautek changed the title CB-13300: (Android) Fixed keyboard from overlapping content WIP: CB-13300: (Android) Fixed keyboard from overlapping content Jan 23, 2020
@breautek
Copy link
Contributor Author

breautek commented Jan 23, 2020 •
edited
Loading

Any news on this?

This PR requires more testing, I notice some weird behaviours particularly with google pixel devices in my apps, but not sure if those issues was actually coming from this change or not. Still troubling and makes me lose confidence.

For the time being I added the WIP, but my time working on this at this time is quite limited, so I can't guarantee when exactly I'll get around to this.

In the meantime, you can fork this plugin and apply my PR into your own fork, others have claimed success, but do so at your own risk.

@purplecabbage
Copy link
Contributor

Is this still valid?

@breautek
Copy link
Contributor Author

Is this still valid?

I think the issue still exists, but this PR isn't a sufficient solution. That is it causes issues in some devices, where the native APIs didn't respond accurately causing the view to be positioned incorrectly.

I had a look at the fork that my company uses and it looks like we have removed this PR so I'm not using this in my production apps anymore either. If I recall correctly we removed it and just updated our UI so that it won't conflict with the keyboard space.

I'm going to close this as abandoned. I don't foresee myself coming back to this unfortunately. My fork will remain available, if anybody wants to either pick it up where add it to their own fork.

@breautek breautek closed this Jul 12, 2022
elisemn added a commit to indie-technologies/cordova-plugin-statusbar that referenced this pull request Dec 7, 2022
@elisemn
Rebase changes from apache#128 onto latest release
16e0247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@purplecabbage purplecabbage Awaiting requested review from purplecabbage

Assignees

@breautek breautek

Labels
bug platform: android
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cordova Status Bar Plugin (with StatusBar.overlaysWebView(true)) hides the input when the keyboard appears
7 participants
@breautek @pengkobe @purplecabbage @armandn @danielehrhardt @malinges @janpio