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(web): prevent layer switch key from erasing selection #11032

Merged
merged 5 commits into from
Mar 29, 2024

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Mar 21, 2024

Fixes #7866.

When the transform generated by a key event results in no changes to the text, this will no longer trigger a change event in the apps. This means that layer switch keys will no longer erase the selection.

Fix proposed by @jahorton.

User Testing

GROUP_IOS: Run these tests in Keyman for iPhone and iPad
GROUP_ANDROID: Run these tests in Keyman for Android

TEST_BASIC_INPUT: Verify that basic input continues to function correctly.
TEST_BACKSPACE: Type some text, then verify that backspace continues to work as normal.
TEST_CONTEXT_SYNC: Using Khmer Angkor keyboard, type . Select the numeric/symbol layer, and return to the default layer. Press ្ម (longpress on ). Press Backspace and verify that the output on screen is ខ្ម (and not ខេ).
TEST_SELECTION_DELETION: Type two words, select the second word, and press backspace on the touch keyboard to delete it. Verify that the word is deleted as expected.
TEST_SELECTION_REPLACEMENT: Type two words, select the second word, and press a letter on the touch keybaord to replace it. Verify that the word is deleted as expected, and the new letter is left in its place.
TEST_SELECTION_LAYER_SWITCH: Type two words, select the second word, and press any layer switch key on the touch keyboard. The active layer should change, but the text should not be modified.

Fixes #7866.

When the transform generated by a key event results in no changes to the
text, this will no longer trigger a change event in the apps. This means
that layer switch keys will no longer erase the selection.

Fix proposed by @jahorton.
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Mar 21, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Mar 21, 2024

User Test Results

Test specification and instructions

  • ✅ GROUP_IOS: Run these tests in Keyman for iPhone and iPad

    6 tests PASSED
    • TEST_BASIC_INPUT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Installed sil_ipa keyboard. 2. Verified that switching between keyboards using the globe key is working fine. 3. Longpress menu options are working as expected. 4. Shift layer and Symbol layer are working as expected. 5. Tested the same in iPad (10th generation) Simulator and it works fine.
    • TEST_BACKSPACE (PASSED): 1. Entered some texts on the Notes app then pressing the backspace key deletes the characters one by one. Verified that the backspace key functionality is working as expected. 2. Tested the same in iPad (10th generation) Simulator and it is working fine.
    • TEST_CONTEXT_SYNC (PASSED): Retested with the updated instructions and here is my observation: 1. Installed Khmer Angkor keyboard. Switched to a Khmer Angkor keyboard. Typed ខ េ. 2. Clicked the number / symbol layer then returned to the default layer. 3. Selected ្ from the long-press menu. 4. Pressed the Backspace key and verified that the output showed ខ្ on the Screen. (notes)
    • TEST_SELECTION_DELETION (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Typed two words on the Notes app. 2. Selected the second word. 3. Pressed the backspace key on the OSK and verified that it deletes the selected word. 4. Tested the same on iPad (10 generation) Simulator and it is working fine.
    • TEST_SELECTION_REPLACEMENT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Typed two words on the Notes app. 2. Selected the second word. 3. Typed a letter on the touch keyboard to replace it. Verified the selected word is deleted and new letter is left in its place. 4. Tested the same on iPad (10 generation) Simulator and it is working fine.
    • TEST_SELECTION_LAYER_SWITCH (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Typed two words on the Notes app. 2. Selected the second word. 3. Pressed the '123' number key and layer changed to another layer. Pressed the Symbol key and the the layer changed to the Symbol layer. Verified that the selected word didn't change. 4. Tested the same on the iPad (10 generation) Simulator and it is working fine.
  • ✅ GROUP_ANDROID: Run these tests in Keyman for Android

    6 tests PASSED
    • TEST_BASIC_INPUT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android 13 Mobile device and here is my observation: 1. Installed sil_ipa keyboard. 2. Verified that switching between keyboards using the globe key is working fine. 3. Longpress menu options are working as expected. 4. Shift layer and Symbol layer are working as expected. 5. Tested the same in iPad (10th generation) Simulator and it works fine.
    • TEST_BACKSPACE (PASSED): 1. Entered some texts on the text input screen then pressing the backspace key, deletes the characters one by one. Verified that the backspace key functionality is working as expected.
    • TEST_CONTEXT_SYNC (PASSED): 1. Installed Khmer Angkor keyboard. Switched to a Khmer Angkor keyboard. Typed ខ េ. 2. Clicked the number / symbol layer then returned to the default layer. 3. Selected ្ from the long-press menu. 4. Pressed the Backspace key and verified that the output showed ខ្ on the Screen. (notes)
    • TEST_SELECTION_DELETION (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android Mobile device and here is my observation: 1. Typed two words on the text input screen. 2. Selected the second word. 3. Pressed the backspace key on the OSK and verified that it deletes the selected word.
    • TEST_SELECTION_REPLACEMENT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android 13 mobile device and here is my observation: 1. Typed two words on the text input screen. 2. Selected the second word. 3. Typed a letter on the touch keyboard to replace it. Verified the selected word is deleted and new letter is left in its place.
    • TEST_SELECTION_LAYER_SWITCH (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android 13 Mobile device and here is my observation: 1. Typed two words on the text input screen. 2. Selected the second word. 3. Pressed the '123' number key and layer changed to another layer. Pressed the Symbol key and the the layer changed to the Symbol layer. Verified that the selected word didn't change.

Test Artifacts

Copy link
Contributor

@ermshiperete ermshiperete left a comment

Choose a reason for hiding this comment

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

LGTM

@jahorton
Copy link
Contributor

Note: need @jahorton's input on how best to structure these user tests to ensure that we cover the necessary potential context desync scenarios.

What you have here seems fine to me.

@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 Mar 27, 2024
Co-authored-by: Joshua Horton <joshua_horton@sil.org>
@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_BASIC_INPUT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Installed sil_ipa keyboard. 2. Verified that switching between keyboards using the globe key is working fine. 3. Longpress menu options are working as expected. 4. Shift layer and Symbol layer are working as expected. 5. Tested the same in iPad (10th generation) Simulator and it works fine.

@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_BACKSPACE (PASSED): 1. Entered some texts on the Notes app then pressing the backspace key deletes the characters one by one. Verified that the backspace key functionality is working as expected. 2. Tested the same in iPad (10th generation) Simulator and it is working fine.

@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_CONTEXT_SYNC (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Opened https://build.palaso.org/repository/download/Keymanweb_TestPullRequests/451458:id/src/test/manual/web/unminified.html test page in the Chrome browser. 2. Run the web_context_tests keyboard on the test page. 3. Verified that the context changes were correctly synchronized between the the app and keymanweb. 4. Tested this in iPad (10th generation) simulator, and it works fine. Did I miss any other test related to context_sync? Please let me know. Thank you.

@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_SELECTION_DELETION (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Typed two words on the Notes app. 2. Selected the second word. 3. Pressed the backspace key on the OSK and verified that it deletes the selected word. 4. Tested the same on iPad (10 generation) Simulator and it is working fine.

@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_SELECTION_REPLACEMENT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Typed two words on the Notes app. 2. Selected the second word. 3. Typed a letter on the touch keyboard to replace it. Verified the selected word is deleted and new letter is left in its place. 4. Tested the same on iPad (10 generation) Simulator and it is working fine.

@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_SELECTION_LAYER_SWITCH (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Typed two words on the Notes app. 2. Selected the second word. 3. Pressed the '123' number key and layer changed to another layer. Pressed the Symbol key and the the layer changed to the Symbol layer. Verified that the selected word didn't change. 4. Tested the same on the iPad (10 generation) Simulator and it is working fine.

@mcdurdin
Copy link
Member Author

  • TEST_CONTEXT_SYNC (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Opened https://build.palaso.org/repository/download/Keymanweb_TestPullRequests/451458:id/src/test/manual/web/unminified.html test page in the Chrome browser. 2. Run the web_context_tests keyboard on the test page. 3. Verified that the context changes were correctly synchronized between the the app and keymanweb. 4. Tested this in iPad (10th generation) simulator, and it works fine. Did I miss any other test related to context_sync? Please let me know. Thank you.

My apologies, that test was very badly specified by me. I was going to rewrite it and then I missed it. I have rewritten the test, which should be run with the app -- not with KeymanWeb!

@keymanapp-test-bot retest group_ios TEST_CONTEXT_SYNC

@bharanidharanj
Copy link

  • TEST_CONTEXT_SYNC (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on iPhone 13 Mobile device and here is my observation: 1. Opened https://build.palaso.org/repository/download/Keymanweb_TestPullRequests/451458:id/src/test/manual/web/unminified.html test page in the Chrome browser. 2. Run the web_context_tests keyboard on the test page. 3. Verified that the context changes were correctly synchronized between the the app and keymanweb. 4. Tested this in iPad (10th generation) simulator, and it works fine. Did I miss any other test related to context_sync? Please let me know. Thank you.

My apologies, that test was very badly specified by me. I was going to rewrite it and then I missed it. I have rewritten the test, which should be run with the app -- not with KeymanWeb!

@keymanapp-test-bot retest group_ios TEST_CONTEXT_SYNC

@mcdurdin Thanks for the clarification!

@bharanidharanj
Copy link

Test Results

GROUP_IOS: Run these tests in Keyman for iPhone and iPad

  • TEST_CONTEXT_SYNC (PASSED): Retested with the updated instructions and here is my observation: 1. Installed Khmer Angkor keyboard. Switched to a Khmer Angkor keyboard. Typed ខ េ. 2. Clicked the number / symbol layer then returned to the default layer. 3. Selected ្ from the long-press menu. 4. Pressed the Backspace key and verified that the output showed ខ្ on the Screen.

@bharanidharanj
Copy link

Test Results

GROUP_ANDROID: Run these tests in Keyman for Android

  • TEST_BASIC_INPUT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android 13 Mobile device and here is my observation: 1. Installed sil_ipa keyboard. 2. Verified that switching between keyboards using the globe key is working fine. 3. Longpress menu options are working as expected. 4. Shift layer and Symbol layer are working as expected. 5. Tested the same in iPad (10th generation) Simulator and it works fine.

  • TEST_BACKSPACE (PASSED): 1. Entered some texts on the text input screen then pressing the backspace key, deletes the characters one by one. Verified that the backspace key functionality is working as expected.

  • TEST_CONTEXT_SYNC (PASSED): 1. Installed Khmer Angkor keyboard. Switched to a Khmer Angkor keyboard. Typed ខ េ. 2. Clicked the number / symbol layer then returned to the default layer. 3. Selected ្ from the long-press menu. 4. Pressed the Backspace key and verified that the output showed ខ្ on the Screen.

  • TEST_SELECTION_DELETION (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android Mobile device and here is my observation: 1. Typed two words on the text input screen. 2. Selected the second word. 3. Pressed the backspace key on the OSK and verified that it deletes the selected word.

  • TEST_SELECTION_REPLACEMENT (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android 13 mobile device and here is my observation: 1. Typed two words on the text input screen. 2. Selected the second word. 3. Typed a letter on the touch keyboard to replace it. Verified the selected word is deleted and new letter is left in its place.

  • TEST_SELECTION_LAYER_SWITCH (PASSED): Tested with the attached PR build (17.0.292-beta-test-11032) on an Android 13 Mobile device and here is my observation: 1. Typed two words on the text input screen. 2. Selected the second word. 3. Pressed the '123' number key and layer changed to another layer. Pressed the Symbol key and the the layer changed to the Symbol layer. Verified that the selected word didn't change.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Mar 28, 2024
@mcdurdin mcdurdin merged commit 4e403e9 into beta Mar 29, 2024
19 checks passed
@mcdurdin mcdurdin deleted the fix/web/7866-layer-switch-key-deletes-selection branch March 29, 2024 04:22
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.298-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(web): pressing layer switch key with selected text would delete the selection
5 participants