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/engine): Skip updating selection range if invalid #11384

Merged
merged 1 commit into from
May 9, 2024

Conversation

darcywong00
Copy link
Contributor

@darcywong00 darcywong00 commented May 7, 2024

Fixes #11360

@mcdurdin went spelunking in the Android code and found getSelectionStart() and getSelectionEnd() return -1 if there's no selection or cursor.
Reference https://developer.android.com/reference/android/text/Selection#getSelectionEnd(java.lang.CharSequence

In those cases (likely a race condition), we should no-op on updateSelectionRange() since there's no valid selection to pass to KeymanWeb.

User Testing (from #11360)

Setup - Install PR build of Keyman for Android and enable Keyman as the default system keyboard

  • TEST_INVERTED_SELECTION_RANGE: Using Keyman for Android as a system keyboard, ensure that backwards selection ranges do not crash the keyboard.
  1. Select text, then drag one of the end points through and past the other one.
  2. If the keyboard immediately disappears, or you see an error notification, FAIL this test.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels May 7, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented May 7, 2024

User Test Results

Test specification and instructions

  • TEST_INVERTED_SELECTION_RANGE (PASSED): I tested this issue with the attached "Keyman 17.0.320-beta-test-11384" build on an Android 14 environment: Here is my observation. (notes)

Test Artifacts

Comment on lines 185 to 194
int selStart = icText.selectionStart;
int selEnd = icText.selectionEnd;

if (selStart < 0 || selEnd < 0) {
// There is no selection or cursor
// Reference https://developer.android.com/reference/android/text/Selection#getSelectionEnd(java.lang.CharSequence)
return false;
}

int selMin = selStart, selMax = selEnd;
Copy link
Member

@mcdurdin mcdurdin May 7, 2024

Choose a reason for hiding this comment

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

Personally, I'd like to eliminate the selStart and selEnd variables. They just add potential for error.

Suggested change
int selStart = icText.selectionStart;
int selEnd = icText.selectionEnd;
if (selStart < 0 || selEnd < 0) {
// There is no selection or cursor
// Reference https://developer.android.com/reference/android/text/Selection#getSelectionEnd(java.lang.CharSequence)
return false;
}
int selMin = selStart, selMax = selEnd;
int selMin = icText.selectionStart, selMax = icText.selectionEnd;
if (selMin < 0 || selMax < 0) {
// There is no selection or cursor
// Reference https://developer.android.com/reference/android/text/Selection#getSelectionEnd(java.lang.CharSequence)
return false;
}
if(selMin > selMax) {
selMin = icText.selectionEnd;
selMax = icText.selectionStart;
}

I can't do this suggestion across the whole range of this code, so you'll need to delete the subsequent if clause.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we should eliminate the result variable, and short-circuit the input conditions:

    InputConnection ic = KMManager.getInputConnection(this.keyboardType);
    if (ic == null) {
      // TODO: please explain
      return true;
    }

    ExtractedText icText = ic.getExtractedText(new ExtractedTextRequest(), 0);
    if (icText == null) {
      // TODO: please explain
      return false;
    }

    // ... the rest of the function goes here ...

    return true;

Why do we return true when ic == null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring addressed in #11389. Also fixes to return false if ic == null.

@dinakaranr
Copy link

Test Results

  • TEST_INVERTED_SELECTION_RANGE (PASSED): I tested this issue with the attached "Keyman 17.0.320-beta-test-11384" build on an Android 14 environment: Here is my observation.
  1. Installed the "keyman-17.0.320.apk" file 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 Chrome browser and then enter some text in the address bar.
  4. The keyboard did not disappear when selecting some text from right to left and left to right. The keyboard stays on the screen while dragging the text selection.

Note: The reverse text selection does not happen from one point to another. It's restricted to the last letter of the word. I observed this behavior in Android 13 (Physical Device) and emulator devices in Android 9 and 12 versions.
However, it worked well, and I passed this test case. Thanks.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label May 7, 2024
@darcywong00 darcywong00 merged commit 58f169e into beta May 9, 2024
6 checks passed
@darcywong00 darcywong00 deleted the fix/android/invalid-selection branch May 9, 2024 01:43
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.321-beta

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.

bug(android): selected-text handling crashes, continued
5 participants