fix(android): check current orientation when redisplaying system keyboard#11604
fix(android): check current orientation when redisplaying system keyboard#11604
Conversation
User Test ResultsTest specification and instructions
Test Artifacts |
|
ref #10054 |
Test Results
If the above problem does not appear, then repeat any other app on the landscape view. Please refer to the video file. |
|
Following up on @dinakaranr's previous comment:
See timestamp 2:01 - the keyboard launches far too tall (portrait height) in landscape mode. (Upon opening a different app than was previously hosting the system keyboard.) That's the only (related) erroneous timestamp I recognize in that video - did I miss anything? I did notice that at earlier timestamps, re-entering the main app didn't relaunch the keyboard, but that's unrelated - I didn't touch any main-app code in this PR.
See timestamp 2:09-2:22 - (triggers at 2:22) after force-killing all apps that were hosting a system keyboard instance, launching a new one in landscape had the same issue - the keyboard was far too tall. Also see timestamp 1:34 - 1:47 - an earlier point in the test sequence where the same force-kill was applied, but things worked normally afterward. The behavior should have been the same for both timestamps, I'd think. |
|
Finally got one locally, after setting up an identical emulated device target to what was in those videos + also setting a very tall portrait height. I decided to locally insert the following logging line within For clarity,
It's also worth noting some observations here:
I was originally testing with a "Nexus 5 API 32" target. With that device, I never saw a skip from Also note that such a skip does not guarantee that the wrong layout will happen. I've seen similar "skipping" transitions on the emulated Nexus 5 where the keyboard still showed up properly in the intended orientation. Finally... note that the keyboard did at least detect the correct orientation - it just erroneously thought it was already in that orientation and so didn't bother updating. |
|
Got another one, this time with landscape height in portrait (via portrait-locked app):
I added in temp-logging in the I kept the timestamps this time, since that highlights how close in succession the last three logging messages were. No guarantees that it's significant, but I figured that highlighting the info helps here more than it hurts (log simplicity/readability). |
|
Ooh, managed to retrigger that one fairly quickly this time... and I added in one extra bit into the logs. Same pattern as before, but now we're also logging the JS calls being passed to KMW. The concatenated JS line is a bit of a beast, so here's a prettification of its contents: setBannerHTML("<!-- Custom banner theme --><div style=\"background: white; width: 100%; height: 100%; position: absolute; left: 0; top: 0; display: flex\"> <!-- svg path is relative to assets\/ folder --> <img src=\"banner\/keyman_banner.svg\" style=\"height:60%; background: white; display: block; margin: auto 2%;\"> <!-- Tri-color line --> <div style='height: 7%; left: 0; position: absolute; bottom: 0; width: 56%; background: #F68924'><\/div> <div style='height: 7%; left: 56%; position: absolute; bottom: 0; width: 23%; background: #CC3846'><\/div> <div style='height: 7%; left: 79%; position: absolute; bottom: 0; width: 21%; background: #79C3DA'><\/div><\/div>");
showBanner(true);
resetContext();
setBannerOptions(true);
setBannerHTML("<!-- Custom banner theme --><div style=\"background: white; width: 100%; height: 100%; position: absolute; left: 0; top: 0; display: flex\"> <!-- svg path is relative to assets\/ folder --> <img src=\"banner\/keyman_banner.svg\" style=\"height:60%; background: white; display: block; margin: auto 2%;\"> <!-- Tri-color line --> <div style='height: 7%; left: 0; position: absolute; bottom: 0; width: 56%; background: #F68924'><\/div> <div style='height: 7%; left: 56%; position: absolute; bottom: 0; width: 23%; background: #CC3846'><\/div> <div style='height: 7%; left: 79%; position: absolute; bottom: 0; width: 21%; background: #79C3DA'><\/div><\/div>");
setBannerHeight(110);
setOskWidth(393);
setOskHeight(275);
keyman.hideGlobeHint();
;
resetContext();
setBannerOptions(true);
updateKMText({"text":""});
updateKMText({"text":""});
updateKMSelectionRange(0,0);The values do line up within the WebView when accounting for |
|
Well, I went and added even more logging before this next trigger: (newlines added for emphasis) That's... a surprising number of Also, the calls just before the JS stuff passes through, before the first newline... is interestingly placed. The one spot where that's referred to outside of the orientation flow I've been tracing otherwise... is in |
…of onStartInput version
|
@keymanapp-test-bot retest all Noting the system-keyboard order of operations, it seems that the best approach may be to nix the The latest commit makes this change, so it's time for another round of testing. |
|
I've got the latest build installed. Will report back as I use it. |
Test ResultsTEST_KEYBOARD_REPEATED_RESTORE (PASSED): I tested this issue with the attached "Keyman 18.0.45-alpha-test-11604" build on an Android 13.0 (emulator and physical) environment: Here is my observation.
|
|
So far I have not experienced any keyboard sizing issues with this build. Continuing to use it on my phone. |
|
If this works as well as it seems to be so far, we need to update KMSample2 in the same way, pass this knowledge on to the Keyboard App Builder team, and publish this as a release note for other Keyman Engine for Android integrators. |
| // This method (likely) includes the IME equivalent to `onResume` for `Activity`-based classes, | ||
| // making it an important time to detect orientation changes. |
There was a problem hiding this comment.
| // This method (likely) includes the IME equivalent to `onResume` for `Activity`-based classes, | |
| // making it an important time to detect orientation changes. | |
| // This method (likely) includes the IME equivalent to `onResume` for `Activity`-based classes, | |
| // making it an important time to detect orientation changes. | |
| // | |
| // In fact, it appears to be called whenever the view / activity layout changes, after any | |
| // orientation changes have fully resolved. |
|
well it happened again... Restoring the keyboard after not using it for a while. Only this time, height is good and width is bad... I was never seeing wrong width on 17.0.325 |
| public void onConfigurationChanged(Configuration newConfig) { | ||
| super.onConfigurationChanged(newConfig); | ||
| if (newConfig.orientation != lastOrientation) { | ||
| lastOrientation = newConfig.orientation; | ||
| KMManager.onConfigurationChanged(newConfig); | ||
| } | ||
| } |
There was a problem hiding this comment.
It sounds as if this part of the code may be needed for getting proper landscape width, based upon #11604 (comment). But, with it here, it's possible for an update to lastOrientation here to block proper height-setting in onStartInput.
There was a problem hiding this comment.
Note:
keyman/android/KMEA/app/src/main/java/com/keyman/engine/KMKeyboard.java
Lines 446 to 459 in c85f774
The Configuration object we get here sources the keyboard width as seen in the code block linked above.
Meanwhile, the height values there are determined via our orientation-detection code:
keyman/android/KMEA/app/src/main/java/com/keyman/engine/KMManager.java
Lines 2045 to 2054 in c85f774
|
Changes in this pull request will be available for download in Keyman version 18.0.53-alpha |
Replaces #11484.
Noting two different comments from #11484, I now suspect the issue is that the system keyboard simply lacked "on resume"-style orientation checking. After some investigation, I think I've identified the correct place for it.
The new code essentially duplicates the following in-app block for handling orientation changes when the app is restored:
keyman/android/KMAPro/kMAPro/src/main/java/com/tavultesoft/kmapro/MainActivity.java
Lines 243 to 251 in c8d8e41
While a bit WET, note that a special state variable is needed for tracking. Both would lie within "app" space, so I suppose I could DRY it out if this approach is in fact the solution.
User Testing
TEST_KEYBOARD_REPEATED_RESTORE
Sadly, just this isn't enough to get a repro with the current state of master for me within Android Studio's emulator.