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

fix(android): Resize keyboard on orientation change #11484

Closed
wants to merge 4 commits into from

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented May 20, 2024

This PR attempts to resolve the tricky wrong-keyboard-size bug that continues to occur within our Android app, despite our best efforts.

After analyzing the codebase, I feel that there are two different types of orientation-change triggers within the code:

  1. onResume, when restoring the app/keyboard from the background. We just need to set whatever the current orientation is in place, even if it changed since being backgrounded.
  2. onConfigurationChanged, during an actual orientation-change event.

The solution for point 1 - reliant on the Display-based pathway, seems to work quite well for reading the current orientation when it's not being mutated.

For point 2, we currently ignore the provided parameter for onConfigurationChange in favor of reusing the same solution as is used for onResume. (This seems to have been established in either #10373 or #10442.) We used to read it from activity.getResources().getConfiguration().orientation instead... until we found that sometimes reading it specifically from that consistent source could be prone to a known bug on certain devices. Interestingly, the external references I could find to that bug... require accessing it in that manner, rather than from onConfigurationChange's parameter.

  • Example reference: https://stackoverflow.com/q/48777084
  • Cross-reference with:
    // onConfigurationChanged() only triggers when device is rotated while app is in foreground
    // This handles when device is rotated while app is in background
    // using KMManager.getOrientation() since getConfiguration().orientation is unreliable #10241
    Configuration newConfig = this.getResources().getConfiguration();
    int newOrientation = KMManager.getOrientation(context);
    if (newOrientation != lastOrientation) {
    lastOrientation = newOrientation;
    KMManager.onConfigurationChanged(newConfig);
    }

If we instead use the value directly from the onConfigurationChanged parameter when possible, perhaps that may allow us to dodge that particular bug issue entirely. It's worth noting that this is the "standard solution" I found, repeatedly, when searching for how to detect device orientation - using onConfigurationChanged and the .orientation field of its parameter.

Meanwhile, my current "best guess" for the current problem is that the onResume pathway, reliant upon Display - isn't intended for use during an orientation change. If it's updated in a manner susceptible to race-conditions, we could be getting the old orientation via the Display-based pathway when the "race" is "lost". It's hard to say for sure without thorough testing and/or a solid repro that could be used as proof, though. I'm not seeing much external mention of this as a possibility, but this is the possibility that makes most sense to me via process of elimination.

User Testing

TEST_OSK - Verifies OSK is full width after using keyboard picker

  1. Launch Keyman for Android in landscape
  2. Type some letters on the input text screen.
  3. Longpress the globe key to open the Keyboard picker menu.
  4. Click the default keyboard.
  5. Verify the app shows the OSK full-width and full height.

TEST_LANDSCAPE_KEYBOARD_PICKER - Verifies menu to adjust keyboard height works

  1. Install the PR build of Keyman for Android on an Android device/emulator
  2. with the device in Landscape mode, Launch Keyman and dismiss the Get Started menu
  3. In the Keyman app, type some letter using the OSK
  4. Longpress the globe key to display the keyboard picker menu
  5. Select the same keyboard from the picker menu
  6. Verify OSK is still full width and full height

TEST_OSK_ROTATION_ANDROID_12

  1. Install the PR build of Keyman for Android on an Android device/emulator Android 12 (SDK 32)
  2. Open Keyman In-App.
  3. Open the Settings menu.
  4. Open Adjust keyboard height menu.
  5. Set the height to the maximum size.
  6. Revert to Keyman Home screen.
  7. Change the Orientation from Portrait to Landscape.
  8. Verify OSK displays and is full width and full height.
  9. Change the orientation from Landscape to Portrait.
  10. Verify OSK displays and is full width and full height.
  11. Repeat steps 7 through 10 at least ten times, verifying that the OSK always displays with the correct width.

TEST_OSK_ROTATION_ANDROID_13

  1. Install the PR build of Keyman for Android on an Android device/emulator Android 13 (SDK 33)
  2. Open Keyman In-App.
  3. Open the Settings menu.
  4. Open Adjust keyboard height menu.
  5. Set the height to the maximum size.
  6. Revert to Keyman Home screen.
  7. Change the Orientation from Portrait to Landscape.
  8. Verify OSK displays and is full width and full height.
  9. Change the orientation from Landscape to Portrait.
  10. Verify OSK displays and is full width and full height.
  11. Repeat steps 7 through 10 at least ten times, verifying that the OSK always displays with the correct width.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label May 20, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented May 20, 2024

User Test Results

Test specification and instructions

  • TEST_OSK (PASSED): - I have followed acceptance steps. OSK works after selecting a default keyboard on the "Keyboard Picker" menu. Hence OSK appears in full width on OSK.
  • TEST_LANDSCAPE_KEYBOARD_PICKER (PASSED): - I have followed acceptance steps. OSK works after changing the keyboard height. OSK appeared in full width & height after selecting a default keyboard on the "Keyboard picker" menu.
  • TEST_OSK_ROTATION_ANDROID_12 (PASSED): - I have followed acceptance steps. OSK appeared in full width & height after selecting a default keyboard on the "Keyboard picker" menu. I tried to change the Portrait to Landscape and Landscape to Portrait view and OSK always displays with the correct width & height. It works correctly after repeating steps more than 10 times.
  • TEST_OSK_ROTATION_ANDROID_13 (PASSED): - I have followed acceptance steps. I tried to change the view from Portrait to Landscape and Landscape to Portrait and OSK always displays with the correct width & height. It works correctly after repeating steps more than 10 times.

Test Artifacts


this.dismissHelpBubble();
DisplayMetrics dms = context.getResources().getDisplayMetrics();
int kbWidth = (int) (dms.widthPixels / dms.density);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific line - kbWidth - has two other instances in the codebase already. This simply makes a third.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed and removed user-test-missing User tests have not yet been defined for the PR labels May 23, 2024
@jahorton jahorton marked this pull request as ready for review May 23, 2024 08:31
@jahorton jahorton requested a review from sgschantz as a code owner May 23, 2024 08:31
@darcywong00 darcywong00 changed the title fix(android): keyboard sizing on orientation change fix(android): Resize keyboard on orientation change May 24, 2024
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than approving these changes, I think I will install and use this PR build on my phone for a few days. I experience the OSK height issues most days, so that should be enough to tell us if the patch improves the situation.

@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman 18.0.39-alpha-test-11484" build on an Android 12 & 13 & 14 emulator & physical environment: Here is my observation.
Installed the "Keyman 18.0.39.apk" file and gave all permissions to the application.
Checked the "Enable Keyman as system-wide keyboard" and set the keyboard as the default keyboard box on the settings page.

  • TEST_OSK (PASSED): - I have followed acceptance steps. OSK works after selecting a default keyboard on the "Keyboard Picker" menu. Hence OSK appears in full width on OSK.

  • TEST_LANDSCAPE_KEYBOARD_PICKER (PASSED): - I have followed acceptance steps. OSK works after changing the keyboard height. OSK appeared in full width & height after selecting a default keyboard on the "Keyboard picker" menu.

  • TEST_OSK_ROTATION_ANDROID_12 (PASSED): - I have followed acceptance steps. OSK appeared in full width & height after selecting a default keyboard on the "Keyboard picker" menu. I tried to change the Portrait to Landscape and Landscape to Portrait view and OSK always displays with the correct width & height. It works correctly after repeating steps more than 10 times.

  • TEST_OSK_ROTATION_ANDROID_13 (PASSED): - I have followed acceptance steps. I tried to change the view from Portrait to Landscape and Landscape to Portrait and OSK always displays with the correct width & height. It works correctly after repeating steps more than 10 times.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label May 24, 2024
@mcdurdin
Copy link
Member

Screenshot_20240524_164517_WhatsApp.jpg

Screenshot_20240524_164525_WhatsApp.jpg

sadly... Probably worse than before.

@mcdurdin mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024
@mcdurdin
Copy link
Member

Screenshot_20240524_164517_WhatsApp.jpg

Screenshot_20240524_164525_WhatsApp.jpg

sadly... definitely worse than before.

This update doesn't come good if I switch to another keyboard and back, unlike 17.0.325.

Rotation and restore are both broken.

In 17.0.325, rotation seems fine, and it is only restore that seems to sometimes go wrong. So I think we need to leave the rotation handler alone and look at the restore handler.

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as 'request changes' to take it out of the review queue -- given the issues noted in my earlier comments 😁

@jahorton
Copy link
Contributor Author

In 17.0.325, rotation seems fine, and it is only restore that seems to sometimes go wrong. So I think we need to leave the rotation handler alone and look at the restore handler.

This information looks to be very useful; that's more description than I think I had before. Upon investigating the current code for stable, I have a question - does this ever occur in-app for you, or only with the system keyboard?

I happened to notice there's specific handling for this in-app that lacks a clear parallel in the system keyboard, and your answer would likely help clarify that and point me in the right direction.

@mcdurdin
Copy link
Member

Upon investigating the current code for stable, I have a question - does this ever occur in-app for you, or only with the system keyboard?

I almost never use the in-app keyboard so don't have data to answer this question sorry.

This information looks to be very useful; that's more description than I think I had before.

I've now linked this PR and #11604 to #10054 (the original issue I opened on this problem) for posterity. #10054 said this:

I have not yet been able to figure out exactly what causes this, but sometimes when the OSK pops up on Android, the keyboard shows at the wrong size. Rotating the device seems to be the only way to address this.

@jahorton
Copy link
Contributor Author

We're having far better success with #11604, so I'm closing this PR out now.

@jahorton jahorton closed this May 31, 2024
@jahorton jahorton deleted the fix/android/keyboard-orientation-sizing branch May 31, 2024 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants