fix(web): Fix clearing of deadkeys by ermshiperete · Pull Request #9944 · keymanapp/keyman

ermshiperete · 2023-11-06T05:37:07Z

don't clear deadkeys after pressing backspace
add unit test
also some small refactorings

Fixes #8568.

User Testing

TEST_8568: Verify that #8568 is fixed

Type phaa saa laaw ko then press bksp, bksp
Verify you get the following output: ພາສາລາວ

Since things are working in both cases it seems that the problem got fixed and the comments are obsolete.

keymanapp-test-bot · 2023-11-06T05:37:12Z

User Test Results

Test specification and instructions

✅ TEST_8568 (PASSED): Tested with the attached PR build (Keyman Developer 17.0.206-alpha-test-9944) on Windows 10 OS and here is my observation: 1. Installed Keyman 17.0.206-alpha build in the system. 2. Installed Lao Phonetic keyboard. 3. Opened the text editor. 3. Verified that it outputs the expected result. 4. Opened Lao Phonetic project in Keyman developer. Repeat step 1 as it mentioned in the User Testing. I have attached the screenshot for your reference. 5. Open the keyboard in Chrome mobile emulator mode. (notes)

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
- TestFlight internal PR build version - 17.0.206 (0.9944.9675)
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

Also adds a similar testcase to the baseline keyboard tests.

Fixes #8568.

mcdurdin

This looks okay, but I have some questions around the testing of the default backspace handling with deadkeys -- are all the scenarios in k_020__deadkeys_and_backspace being covered? Are they being tested automatically at present or did you manually test?

Also can you split out the change you made to k_020 because it is a different test case?

common/web/input-processor/tests/cases/inputProcessorTests.js

common/test/keyboards/baseline/k_020___deadkeys_and_backspace.kmn

jahorton · 2023-11-06T08:38:42Z

common/test/keyboards/baseline/k_014___groups_and_virtual_keys.kmn

While I'm guessing this test keyboard was under consideration for use, it looks like that got cancelled. Not sure about the removal of a few comments from lines 25 and 26, so I'll leave that aspect of the review to others.

common/test/keyboards/baseline/k_020___deadkeys_and_backspace.kmn

Addresses code review comments.

mcdurdin · 2023-11-06T09:00:24Z

common/test/keyboards/baseline/k_020___deadkeys_and_backspace.kmn

-c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP]
-c expected: 78aa
+c 7. Two chars and one deadkey in between in context: 'c' (dk7) 'd' + BKSP = 'ok'
+c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP][K_7][K_BKSP] [K_BKSP]


Suggested change

c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP][K_7][K_BKSP] [K_BKSP]

c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP][K_7][K_BKSP][K_BKSP]

mcdurdin · 2023-11-06T09:01:14Z

common/test/keyboards/baseline/k_020___deadkeys_and_backspace.kmn

 c 6. One char and two deadkeys and one char and two deadkeys in context     'a' dk(6a) dk(6b) 'b' dk(6c) dk(6d) + BKSP = 'a'
-c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP]
-c expected: 78aa
+c 7. Two chars and one deadkey in between in context: 'c' (dk7) 'd' + BKSP = 'ok'


Suggested change

c 7. Two chars and one deadkey in between in context: 'c' (dk7) 'd' + BKSP = 'ok'

c 7. Tests behaviour when deleting two characters with deadkey prior to first character,

c verifying that deadkeys are preserved with the first backspace event:

c 'c' (dk7) 'de' + BKSP + BKSP = 'ok'

mcdurdin · 2023-11-06T09:02:41Z

common/test/resources/keyboards/test_8568_deadkeys.kmn

+c 4. Two deadkeys in a row in context         dk(4a) dk(4b) + BKSP = nul
+c 5. One char and two deadkeys in context     'a' dk(5a) dk(5b) 'b' + BKSP = 'a'
+c 6. One char and two deadkeys and one char and two deadkeys in context     'a' dk(6a) dk(6b) 'b' dk(6c) dk(6d) + BKSP = 'a'
+c 7. Two chars and one deadkey in between in context: 'c' (dk7) 'd' + BKSP = 'ok'


Need to update this test to match k_020

bharanidharanj · 2023-11-07T12:59:07Z

Test Results

TEST_8568 (PASSED): Tested with the attached PR build (Keyman Developer 17.0.206-alpha-test-9944) on Windows 10 OS and here is my observation: 1. Installed Keyman 17.0.206-alpha build in the system. 2. Installed Lao Phonetic keyboard. 3. Opened the text editor. 3. Verified that it outputs the expected result. 4. Opened Lao Phonetic project in Keyman developer. Repeat step 1 as it mentioned in the User Testing. I have attached the screenshot for your reference. 5. Open the keyboard in Chrome mobile emulator mode.

..in Windows

..in the Debugger window

..in Web

keyman-server · 2023-11-08T18:03:03Z

Changes in this pull request will be available for download in Keyman version 17.0.208-alpha

ermshiperete added 2 commits November 6, 2023 12:20

refactor(core): Remove obsolete comments

fa0b3e8

Since things are working in both cases it seems that the problem got fixed and the comments are obsolete.

chore(web): Improve debug output of nodeProctor

84dd0c6

ermshiperete requested review from jahorton and mcdurdin as code owners November 6, 2023 05:37

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Nov 6, 2023

keymanapp-test-bot bot added this to the A17S25 milestone Nov 6, 2023

github-actions bot added common/ common/web/ fix web/ labels Nov 6, 2023

ermshiperete marked this pull request as draft November 6, 2023 05:39

ermshiperete added 2 commits November 6, 2023 13:28

chore(web): Add failing test for deadkey bug

2e71ec9

Also adds a similar testcase to the baseline keyboard tests.

fix(web): Don't clear deadkeys after backspace

a43fad4

Fixes #8568.

ermshiperete force-pushed the fix/web/8568_deadkeys branch from 56be76b to a43fad4 Compare November 6, 2023 06:36

github-actions bot added web/ and removed web/ labels Nov 6, 2023

ermshiperete marked this pull request as ready for review November 6, 2023 06:38

mcdurdin requested changes Nov 6, 2023

View reviewed changes

common/web/input-processor/tests/cases/inputProcessorTests.js Outdated Show resolved Hide resolved

common/test/keyboards/baseline/k_020___deadkeys_and_backspace.kmn Outdated Show resolved Hide resolved

jahorton reviewed Nov 6, 2023

View reviewed changes

ermshiperete added 2 commits November 6, 2023 15:51

chore(web): Additional tests for backspace

abe6cee

Addresses code review comments.

chore(core): Enhance core test

4851c54

github-actions bot added web/ and removed web/ labels Nov 6, 2023

ermshiperete force-pushed the fix/web/8568_deadkeys branch from 53d95e5 to 4851c54 Compare November 6, 2023 08:57

github-actions bot added web/ and removed web/ labels Nov 6, 2023

ermshiperete requested a review from mcdurdin November 6, 2023 08:58

mcdurdin approved these changes Nov 6, 2023

View reviewed changes

chore(web): Address code review comments

0984363

github-actions bot added web/ and removed web/ labels Nov 6, 2023

chore(core): Fix test keyboard

eb64ca4

github-actions bot added web/ and removed web/ labels Nov 7, 2023

chore(core): Recompile keyboard with kmcomp

fde460e

github-actions bot added web/ and removed web/ labels Nov 7, 2023

keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Nov 7, 2023

ermshiperete merged commit cbec636 into master Nov 8, 2023

ermshiperete deleted the fix/web/8568_deadkeys branch November 8, 2023 01:13

jahorton mentioned this pull request Nov 22, 2023

feat(web): delayed modipress completion 🐵 #9973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(web): Fix clearing of deadkeys#9944

fix(web): Fix clearing of deadkeys#9944
ermshiperete merged 9 commits intomasterfrom
fix/web/8568_deadkeys

ermshiperete commented Nov 6, 2023 •

edited

Loading

Uh oh!

keymanapp-test-bot bot commented Nov 6, 2023 •

edited

Loading

Uh oh!

mcdurdin left a comment

Uh oh!

Uh oh!

Uh oh!

jahorton Nov 6, 2023

Uh oh!

Uh oh!

mcdurdin Nov 6, 2023

Uh oh!

mcdurdin Nov 6, 2023

Uh oh!

mcdurdin Nov 6, 2023

Uh oh!

bharanidharanj commented Nov 7, 2023

Uh oh!

keyman-server commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP][K_7][K_BKSP] [K_BKSP]
	c keys: [K_1][K_BKSP][K_2][K_BKSP][K_3][K_BKSP][K_4][K_BKSP][K_5][K_BKSP][K_6][K_BKSP][K_7][K_BKSP][K_BKSP]

-c 7. Two chars and one deadkey in between in context: 'c' (dk7) 'd' + BKSP = 'ok'
+c 7. Tests behaviour when deleting two characters with deadkey prior to first character,
+c    verifying that deadkeys are preserved with the first backspace event:
+c    'c' (dk7) 'de' + BKSP + BKSP = 'ok'

Uh oh!

Conversation

ermshiperete commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Testing

Uh oh!

keymanapp-test-bot bot commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User Test Results

Test Artifacts

Uh oh!

mcdurdin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jahorton Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcdurdin Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

mcdurdin Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

bharanidharanj commented Nov 7, 2023

Test Results

Uh oh!

keyman-server commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ermshiperete commented Nov 6, 2023 •

edited

Loading

keymanapp-test-bot bot commented Nov 6, 2023 •

edited

Loading