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): Allow external keyboard to work when OSK hidden #12381

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

darcywong00
Copy link
Contributor

Fixes #12366
Reverts part of #7612 which had changed where the Javascript handler gets attached

User Testing

Setup - Install the PR build of Keyman for Android on a physical device. Also pair an external keyboard (USB/bluetooth) to the device

  • TEST_KEYBOARD - Verifies external keyboard works when virtual keyboard hidden
  1. Launch Keyman
  2. From "Get Started", enable Keyman as the default system keyboard
  3. Launch a separate app where you can type in (ideally a note taking app like Keep or OneNote as reported in the issue). If those aren't available, Chrome browser works
  4. Select a text area with Keyman sil_euro_latin selected as the system keyboard
  5. Verify the system keyboard types fine (longpresses, switching, keys, etc).
  6. With the virtual keyboard displayed, type on the physical keyboard ` a
  7. Verify the text becomes à.
  8. From the Android system preferences, select "Language and Keyboard" --> Physical keyboard --> Toggle "show on-screen keyboard" so that the OSK is hidden
  9. Return to the note-taking / Chrome app and select a text field
  10. Type on the physical keyboard ` a
  11. Verify the text becomes à

@darcywong00 darcywong00 added this to the A18S10 milestone Sep 10, 2024
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Sep 10, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Sep 10, 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.

I am uncertain about the ramifications of this change. This means the Runnable runs on a new thread, not the UI thread, which seems to go against the discussion in the previously linked SO post. Can you elucidate?

@darcywong00
Copy link
Contributor Author

I am uncertain about the ramifications of this change. This means the Runnable runs on a new thread, not the UI thread, which seems to go against the discussion in the previously linked SO post. Can you elucidate?

I was just returning the call to how it had been for stable-15.0

public void callJavascriptAfterLoad() {
if(this.javascriptAfterLoad.size() > 0) {
Handler handler = new Handler();
handler.postDelayed(new Runnable() {
@Override
public void run() {
if(javascriptAfterLoad.size() > 0) {
loadUrl("javascript:" + javascriptAfterLoad.get(0));
javascriptAfterLoad.remove(0);
// Make sure we didn't reset the page in the middle of the queue!
if(keyboardSet) {
if (javascriptAfterLoad.size() > 0) {
callJavascriptAfterLoad();
}
}
}
}
}, 1);
}
}

@mcdurdin
Copy link
Member

I was just returning the call to how it had been for stable-15.0

OK, so this was changed because of reported errors -- see #7612 (comment):

True. Note that I placed it here because I saw a corresponding error occur once during development. Thought that was interesting, since "shouldn't the code be doing this already?"

I would like to understand why this is causing the event handlers not to attach -- is it because there is no UI thread when the keyboard is hidden?

@dinakaranr
Copy link

Test Results

I tested this issue with the attached "Keyman 18.0.108-alpha-test-12381" build on the Android 14 mobile (physical device and keyboard connected with USB cable). I am sharing my observation.

TEST_KEYBOARD (Passed):

  1. Installed the "Keyman-18.0.108.apk" file from the PR build. and gave all permissions to the application.
  2. Checked the "Enable Keyman as system-wide keyboard" and set the keyboard as the default keyboard box on the settings page.
  3. Open the Keyman app. Enable the "Predictions" to install the "Dictionary."
  4. Launch the Google Keep application.
  5. Create a new note. Keyman "sil_euro_latin" was selected as the system keyboard.
  6. Typed some text using the "sil_euro_latin" keyboard.
  7. Verified the system keyboard types fine when doing long presses, switching, keys, and pull-down. 
  8. Type `a in the physical keyboard, and OSK appears at the same time. 
  9. Verified the text appeared as à.
  10. Disabled the OSK by selecting "Language and Keyboard" --> Physical keyboard --> Toggle "show the on-screen keyboard."
  11. Again, launch the Google Keep application.
  12. Create a new note.
  13. Verified that the OSK does not appear in the Android mobile app.
  14. Type `a in the physical keyboard.
  15. Verified the text appeared as à.
    It works well using the physical keyboard if OSK is enabled or disabled.
    I tested this same scenario in the "https://editpad.org/" on the Chrome browser. It works great for me.

@darcywong00
Copy link
Contributor Author

is it because there is no UI thread when the keyboard is hidden?

That's what seems to be the behavior
Screenshot from 2024-09-12 14-37-34

When the OSK is hidden, executing hardware keystrokes never gets into this handler on the right. And I see the javascriptAfterLoad queue keeps accumulating the Javascript calls.

When the OSK is shown again, all the Javascript calls execute and the text updates.

@mcdurdin
Copy link
Member

This reversion may appear to work but it is not thread safe -- which is probably why it was changed in the first place. Calling this.postDelayed() is scheduling it to run on the UI thread (per the documentation). But calling new Handler().postDelayed() schedules it to run on a new thread.

  1. I guess this could cause the error "All WebView methods must be called on the same thread." (per the Stack Overflow discussion). Because now we are calling WebView methods from the wrong thread, right? (We'll only see it happen if javascriptAfterLoad.size() > 0.
  2. In the runner, we are referencing parent object member variables that could be being modified by another thread, e.g. javascriptAfterLoad, keyboardSet. This will likely lead to thread races -- access to these members needs to be sychronized.
  3. Also all calls to member functions are on the wrong thread, as they expect to be called on the UI thread, e.g. loadUrl, callJavascriptAfterLoad.
  4. It is possible, albeit probably unlikely, that the runner could execute after the parent object is destroyed, resulting in a crash.

So we need to find a better way to solve this.

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.

See discussion

@darcywong00 darcywong00 modified the milestones: A18S10, A18S11 Sep 14, 2024
@darcywong00 darcywong00 modified the milestones: A18S11, A18S12 Sep 28, 2024
@darcywong00 darcywong00 modified the milestones: A18S12, A18S13 Oct 11, 2024
…/android/external-keyboard

# Keyman Conventional Commit suggestions:
#
# - Link to a Sentry issue with git trailer:
#     Fixes: _MODULE_-_ID_
# - Give credit to co-authors:
#     Co-authored-by: _Name_ <_email_>
# - Use imperative, present tense ('attach' not 'attaches', 'attached' etc)
# - Don't include a period at the end of the title
# - Always include a blank line before trailers
# - More: https://github.com/keymanapp/keyman/wiki/Pull-Request-and-Commit-workflow-notes
@darcywong00 darcywong00 marked this pull request as draft October 22, 2024 01:21
@darcywong00 darcywong00 modified the milestones: A18S13, A18S14 Oct 26, 2024
@darcywong00 darcywong00 removed this from the A18S14 milestone Nov 9, 2024
@darcywong00 darcywong00 added this to the A18S15 milestone Nov 9, 2024
@darcywong00 darcywong00 modified the milestones: A18S15, A18S16 Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

bug(android): External physical keyboard doesn't type when virtual keyboard hidden
3 participants