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 invalid longpress shortcut triggers #10641

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

jahorton
Copy link
Contributor

@jahorton jahorton commented Feb 6, 2024

Fixes #10640.

When I put together #10172, I rightly considered how the changes would be affected for standard longpress timeouts... but wrongly forgot to apply the same sort of filtering whenever the longpress up-flick shortcut is executed.

By making the item-validator a full-on path-model property, rather than a path-model timer-only property, we can validate all finalizations, rather than only timer-based ones. The same filtering should apply for both cases, after all, so it makes sense as a configurable parameter on its new level.

User Testing

TEST_SIMPLE_KEY_UP_FLICK:

  1. Load up the standard "Test unminified Keymanweb" test page while on (or emulating) a mobile device. (Chrome emulation is fine.)
  2. Using the default keyboard ("English")...
  3. Press any non-special key. (For example, the k key.)
  4. Drag up while keeping the key pressed.
  5. FAIL the test if the key-preview disappears almost immediately without moving to a different key.
  6. FAIL the test if any stuck-highlighting effects occur after lifting your finger.

There was a temporary extra test here at one point - see #10641 (comment) - which was used to gauge how related #10640 is to #10592 and whether or not the latter would also be fixed by this PR. Alas, this PR's changes are insufficient for the latter.

@keymanapp-test-bot keymanapp-test-bot bot added this to the B17S1 milestone Feb 6, 2024
@bharanidharanj
Copy link

Test Results

  • TEST_SIMPLE_KEY_UP_FLICK (FAILED): Tested with the "Test unminified Keymanweb" test page in a Chrome emulator and here is my observation: 1. Long pressed the 'k' key to see its Key preview. 2. Dragged up while keeping the key pressed. 3. Noticed that the key preview disappeared immediately after if I tried to touch the 'k' letter using the mouse-pointer. Seems to be an issue.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Feb 6, 2024
@jahorton
Copy link
Contributor Author

jahorton commented Feb 7, 2024

Test Results

  • TEST_SIMPLE_KEY_UP_FLICK (FAILED): Tested with the "Test unminified Keymanweb" test page in a Chrome emulator and here is my observation: 1. Long pressed the 'k' key to see its Key preview. 2. Dragged up while keeping the key pressed. 3. Noticed that the key preview disappeared immediately after if I tried to touch the 'k' letter using the mouse-pointer. Seems to be an issue.

@bharanidharanj It disappeared, rather than just moving to a different key? Could you record a video of some sort for what you're seeing? If it's not moving to a different key, I'm not getting the same behavior when I use the test artifact.

@bharanidharanj
Copy link

Test Results

  • TEST_SIMPLE_KEY_UP_FLICK (FAILED): Tested with the "Test unminified Keymanweb" test page in a Chrome emulator and here is my observation: 1. Long pressed the 'k' key to see its Key preview. 2. Dragged up while keeping the key pressed. 3. Noticed that the key preview disappeared immediately after if I tried to touch the 'k' letter using the mouse-pointer. Seems to be an issue.

@bharanidharanj It disappeared, rather than just moving to a different key? Could you record a video of some sort for what you're seeing? If it's not moving to a different key, I'm not getting the same behavior when I use the test artifact.

@jahorton Yes . It disappeared, rather than moving to a different key. I have recorded a video file for reference. Here, the problem is If I move the mouse pointer (while long pressing the 'k' key) towards the label name 'k' in the key preview, then it (key preview) suddenly disappears from the keyboard.

keypreview.mp4

@jahorton
Copy link
Contributor Author

jahorton commented Feb 7, 2024

@jahorton Yes . It disappeared, rather than moving to a different key. I have recorded a video file for reference. Here, the problem is If I move the mouse pointer (while long pressing the 'k' key) towards the label name 'k' in the key preview, then it (key preview) suddenly disappears from the keyboard.

TEST_SIMPLE_KEY_UP_FLICK: PASSED

All I see is the 'k' key preview being replaced with an 'i' key preview - the key preview has "moved to a different key." Assuming the actual disappearance of the key preview at timestamp 0:15 was you releasing the held mouse-button / touch, I see no errors here. There's no stuck highlighting afterward, either.

@bharanidharanj Would there be a better/clearer way to describe this intended behavior for any future user-tests to be written? It's pretty clear that you interpreted my original description of this behavior differently than I anticipated, so hopefully I can find something clearer for next time.

@bharanidharanj
Copy link

Test Results

  • TEST_RAPID_TYPING_STABILITY (PASSED): Retested as per Josh's suggestion and here is my observation: 1. Opened the testing page in the Chrome emulator. 2. Pressed a non-special key (eg., 'k' key). I dragged up a little while keeping the key pressed. Verified that the key preview still appeared. Released the key and verified that the key preview disappeared. Seems to be working fine.

@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-required User tests have not been completed label Feb 8, 2024
@jahorton
Copy link
Contributor Author

jahorton commented Feb 9, 2024

@keymanapp-test-bot retest TEST_RAPID_TYPING_STABILITY

Your description of the test is from the other specified unit test, not the intended rapid-typing one. Easy enough mistake to make.

@keymanapp-test-bot keymanapp-test-bot bot added the user-test-required User tests have not been completed label Feb 9, 2024
@bharanidharanj
Copy link

bharanidharanj commented Feb 9, 2024

Test Results

  • TEST_RAPID_TYPING_STABILITY (FAILED): Retested with this attached PR build (ver 17.0.261-alpha-test-10641) in the Android mobile device (ver 13) and here is my observation: 1. Rapidly typing and utilizing functions like the globe key, space bar and predictive texts intermittently leads to show an error message on the screen. 2. Subsequently, If I press any single key, the key gets immediately stuck on the keyboard.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed user-test-failed labels Feb 9, 2024
@jahorton jahorton merged commit c246993 into master Feb 13, 2024
20 checks passed
@jahorton jahorton deleted the fix/web/invalid-longpress-shortcut branch February 13, 2024 02:47
@jahorton
Copy link
Contributor Author

I've gone ahead and recorded that result to its related issue. As previously noted, I was hoping that this PR might fix #10592 too, but that was only speculative... and it wasn't the original point of this PR anyway.

@keyman-server
Copy link
Collaborator

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

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): invalid subkey triggers
5 participants