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): fixes up-flick shortcut issue for longpress when keys support downflicks #11216

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

jahorton
Copy link
Contributor

I happened to notice this one yesterday while using sil_euro_latin - our up-flick shortcut for quick longpresses wasn't working.

Rather than make things convoluted with special-case handling this close to release, I figured the simplest way forward is to simply parameterize longpresses and flicks in a way that allows the longpress-flick to win when a key supports both. The old parameterization locked in the start of a full-fledged flick gesture before the longpress up-flick shortcut had a chance to trigger.

User Testing

TEST_LONGPRESSES_AND_FLICKS: Using the sil_euro_latin keyboard, verify the following:

  • A simple longpress on a key.
  • Holding a key, then moving upward quickly causes the subkey menu to display early.
  • Holding a key, then moving downward quickly triggers the key's flick.

@jahorton jahorton requested a review from ermshiperete as a code owner April 12, 2024 01:43
@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Apr 12, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Apr 12, 2024

User Test Results

Test specification and instructions

  • TEST_LONGPRESSES_AND_FLICKS (PASSED): 1. Verified that a long press on a key displays the subkey menu on the keyboard. 2. Verified that holding a key and quickly moving it upward causes the subkey menu to display early. 3. Verified that holding a key and quickly moving it downward causes the key's flick.

@keymanapp-test-bot keymanapp-test-bot bot added this to the B17S5 milestone Apr 12, 2024
Comment on lines +635 to +636
// Needs to beat flick-start priority.
resolutionPriority: 4,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, flick-start is specified at priority 3.

export function flickStartModel(params: GestureParams): GestureModel<any> {
return {
id: 'flick-start',
resolutionPriority: 3,

this.gestureParams.longpress.flickDist = 0.25 * this.currentLayer.rowHeight;
this.gestureParams.flick.startDist = 0.15 * this.currentLayer.rowHeight;
this.gestureParams.flick.startDist = 0.25 * this.currentLayer.rowHeight;
Copy link
Contributor Author

@jahorton jahorton Apr 12, 2024

Choose a reason for hiding this comment

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

I opted to increase the minimum distance for a flick start due to #10647 - if the existing threshold for quick longpress triggers is already too permissive, reducing it would only make things worse. Thus, increasing the flick-start threshold to match is the safer way forward.

Additionally, #10647 probably indicates that the flick-start threshold needed to increase anyway - if 25% of key height is too permissive for one gesture, it likely is too permissive (as a minimum) for all of them.

@jahorton jahorton changed the title fix(web): fixes up-flick shortcut issue for longpress when keys suppot downflicks fix(web): fixes up-flick shortcut issue for longpress when keys support downflicks Apr 12, 2024
@darcywong00 darcywong00 modified the milestones: B17S5, B17S6 Apr 12, 2024
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.

Looks like a nice and easy fix 😄

@bharanidharanj
Copy link

bharanidharanj commented Apr 12, 2024

Test Results

  • TEST_LONGPRESSES_AND_FLICKS (PASSED): 1. Verified that a long press on a key displays the subkey menu on the keyboard. 2. Verified that holding a key and quickly moving it upward causes the subkey menu to display early. 3. Verified that holding a key and quickly moving it downward causes the key's flick.

@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 Apr 12, 2024
@jahorton jahorton merged commit 8052134 into beta Apr 12, 2024
15 of 16 checks passed
@jahorton jahorton deleted the fix/web/longpress-up-flick-priority branch April 12, 2024 14:43
@keyman-server
Copy link
Collaborator

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

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.

LGTM

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.

6 participants