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): handle surrogate pairs in selection range indexing #10885

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Feb 29, 2024

Fixes an issue noted while validating #10873.

This change prevents backspaces and context-deletions from keyboard rules from mishandling the context after moving the text insertion point and/or selecting text.

Recent changes to the codebase have resulted in a need to consistently specify selection ranges with non-BMP indexing. #10662 recognized the need to keep non-BMP processing enabled as part of a different Android bug fix, and #10728 took things a bit further (for iOS) while further validating this position. Until now... we missed that Android was actually specifying the selection range with code unit (BMP) indexing, which does not account for surrogate pairs.

User Testing

TEST_POST_EMOJI_ROTATION:

  1. Go to https://jahorton.github.io/ and download the test_robust_deletions.kmp package.
    Alternatively, scan this QR code for the package:

    QR code

  2. Enable Keyman as system keyboard.

  3. Open the device's primary mail app (gmail). If prompted, attempt to sign in with a Gmail account.

    • Note: you don't need to actually sign in, though. The "Sign in" prompt itself is a perfect test target!
  4. Type just one of the emoji keys 4 times. (Do not exceed 7.)

  5. Type ggg.

  6. Select ggg and hit backspace to delete it.

  7. Type p once, then continue typing it to verify that 'p' correctly rotates as follows, without affecting any other pre-existing text:
    - 'p' will rotate through: p, , , p̂p̂, p, ...
    - The number of code points are: 1, 2, 2, 4, 1, ...
    - One backspace should delete one codepoint; therefore, to delete p̂p̂ completely should take 4 backspaces.

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

keymanapp-test-bot bot commented Feb 29, 2024

User Test Results

Test specification and instructions

  • TEST_POST_EMOJI_ROTATION (PASSED): Tested with the attached PR build (Keyman 17.0.279-beta-test-10885) on an Android 13 Mobile device and here is my observation: 1. Installed test_robust_deletions.kmp package. 2. Enabled Keyman as the system-wide keyboard. 3. Opened an Gmail account. 4. Typed an emoji keys 4 times. 5. Typed ggg. 6. Selected ggg then hit the backspace key to delete it. Deleted. 7. Typed the 'p' key and entered the letter p on the text field. 8. Verified that the 'p' letter changes with each press of the 'p' key. ie., ṗ, p̂, p̂p̂ 9. On the fourth press of the 'p' key, it deleted the p̂p̂ character and displayed the standard 'p' key. Seems to be working as expected. (notes)

Test Artifacts

@mcdurdin
Copy link
Member

Fixes an issue noted while validating #10873.

Just a process thing, it's better to split that out into a separate issue for tracking in the future.

@mcdurdin mcdurdin changed the title fix(android): selection range indexing fix(android): handle surrogate pairs in selection range indexing Feb 29, 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.

Changes LGTM

Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

Should check icText != null before getting the text.

Copy link
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

@bharanidharanj
Copy link

bharanidharanj commented Feb 29, 2024

Test Results

  • TEST_POST_EMOJI_ROTATION (PASSED): Tested with the attached PR build (Keyman 17.0.279-beta-test-10885) on an Android 13 Mobile device and here is my observation: 1. Installed test_robust_deletions.kmp package. 2. Enabled Keyman as the system-wide keyboard. 3. Opened an Gmail account. 4. Typed an emoji keys 4 times. 5. Typed ggg. 6. Selected ggg then hit the backspace key to delete it. Deleted. 7. Typed the 'p' key and entered the letter p on the text field. 8. Verified that the 'p' letter changes with each press of the 'p' key. ie., ṗ, p̂, p̂p̂ 9. On the fourth press of the 'p' key, it deleted the p̂p̂ character and displayed the standard 'p' key. Seems to be working as expected.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 29, 2024
@jahorton jahorton merged commit e9c56bb into beta Mar 1, 2024
5 checks passed
@jahorton jahorton deleted the fix/android/selection-range-indexing branch March 1, 2024 03:40
@keyman-server
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants