jahorton commented 1 week ago

Fixes #10647.

In retrospect, I did find it odd, once or twice before, how the longpress contact-model wasn't extracting the vertical component of the traveled distance and was instead using the whole, unprojected distance. Turns out... it had noticeable cross-effects and was the primary underlying contributor to #10647. I just... didn't think that part of the math completely through for some reason. (git blame -> me)

As noted in a recording and repro of two such occurrences, the user's touchpoint traveled just past the up-flick threshold distance... at a nearly sixty degree angle from it! We should only be considering vertical distance here.

User Testing

TEST_UP_FLICKS: Using the Keyman for Android app, use the default EuroLatin keyboard to verify that an up-flick on base-layer keys quickly displays the subkey menu, without a need to wait half a second.

TEST_QUICK_TYPING: Using the Keyman for Android app, use the default EuroLatin keyboard and type rapidly, without trying to use flicks or longpresses. If a character appears with a diacritic when not intended, FAIL this test.

keymanapp-test-bot[bot] commented 1 week ago

User Test Results

Test specification and instructions

✅ TEST_UP_FLICKS (PASSED): Hammered on the keyboard with lots of upflicks and normal taps on various keys, particularly the period key. No errors encountered and all looking good.
✅ TEST_QUICK_TYPING (PASSED): This is looking good. Not really experiencing issues with longpress shortcuts any more, even with rapid typing.

Test Artifacts

Android
Developer
iOS
- Keyman for iOS (simulator image) - build failure
- FirstVoices Keyboards for iOS (simulator image) - build failure
Keyboards
- Test Keyboards
Web
- KeymanWeb Test Home
Windows

mcdurdin commented 1 week ago

Test Results

TEST_UP_FLICKS (PASSED): Tested up flicks in various contexts and all appeared to work correctly.
TEST_QUICK_TYPING (FAILED): I think this is somewhat better than before, but I am still triggering up-flicks when typing rapidly in normal use with this build. Here is one instance with video and gesture data (per #11277 instructions). The issue is with the letter 'c' in the second typing of the word 'excellent' in the video (only instance of 'c' in the JSON data). I know that there probably is some movement in my touches as I type, but it really does feel incidental enough that it shouldn't trigger the up-flick, from a usability perspective. That is, I'd prefer the up-flick to be recognized only as a really deliberate action as it will generally be the less common outcome -- whether that is via timing or distance threshold tweaks I am not sure. Not sure if this relates also to #11312?

https://github.com/keymanapp/keyman/assets/4498365/9ebdb620-b495-4023-ab78-effa9705a655

[
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_ENTER",
      "sources": [
        {
          "identifier": "touch:195",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 314.3333435058594,
                "targetY": 163,
                "t": 1232883.5999999046,
                "item": "default-K_ENTER"
              },
              {
                "targetX": 313.7109375,
                "targetY": 162.33334350585938,
                "t": 1232998.4000000954,
                "item": "default-K_ENTER"
              },
              {
                "targetX": 313.66668701171875,
                "targetY": 162.33334350585938,
                "t": 1233004.9000000954,
                "item": "default-K_ENTER"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0.9427946554577716,
              "duration": 121.30000019073486,
              "sampleCount": 3,
              "rawDistance": 0.9562922183077271
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:195"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_E",
      "sources": [
        {
          "identifier": "touch:196",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 77,
                "targetY": 21,
                "t": 1234036.9000000954,
                "item": "default-K_E"
              },
              {
                "targetX": 77,
                "targetY": 21,
                "t": 1234078.5999999046,
                "item": "default-K_E"
              },
              {
                "targetX": 74.36003112792969,
                "targetY": 29.586593627929688,
                "t": 1234129.2000000477,
                "item": "default-K_E"
              },
              {
                "targetX": 74.04427337646484,
                "targetY": 29.666671752929688,
                "t": 1234137.0999999046,
                "item": "default-K_E"
              },
              {
                "targetX": 74,
                "targetY": 29.666671752929688,
                "t": 1234144.9000000954,
                "item": "default-K_E"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "angle": 3.4748367732892733,
              "cardinal": "s",
              "netDistance": 9.171215801246275,
              "duration": 108,
              "sampleCount": 5,
              "rawDistance": 9.353290710515564
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:196"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_D",
      "sources": [
        {
          "identifier": "touch:197",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 81.33333587646484,
                "targetY": 98,
                "t": 1234230.7999999523,
                "item": "default-K_D"
              },
              {
                "targetX": 81.33333587646484,
                "targetY": 98.08822631835938,
                "t": 1234252.7000000477,
                "item": "default-K_D"
              },
              {
                "targetX": 81.33333587646484,
                "targetY": 98,
                "t": 1234255.2999999523,
                "item": "default-K_D"
              },
              {
                "targetX": 81.33333587646484,
                "targetY": 98,
                "t": 1234270.5,
                "item": "default-K_D"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 39.700000047683716,
              "sampleCount": 4,
              "rawDistance": 0.17645263671875
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:197"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "longpress",
      "linkType": "chain",
      "sources": [
        {
          "identifier": "touch:198",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 123.66667175292969,
                "targetY": 129.33334350585938,
                "t": 1234428.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 123.62239837646484,
                "targetY": 129.33334350585938,
                "t": 1234444.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 123.66667175292969,
                "targetY": 129.1666717529297,
                "t": 1234473.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 122.37760925292969,
                "targetY": 119.39096069335938,
                "t": 1234479.7999999523,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 112.1376953125,
                "t": 1234489.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 112.1376953125,
                "t": 1234492.4000000954,
                "item": "default-K_C"
              }
            ],
            "stats": {
              "angle": 6.112537534595476,
              "cardinal": "n",
              "netDistance": 17.44909687601092,
              "duration": 64.30000019073486,
              "sampleCount": 6,
              "rawDistance": 17.521028120934968
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:198"
      ]
    },
    {
      "gestureSetId": "none",
      "matchedId": "subkey-select",
      "linkType": "complete",
      "item": "popup-default-U_010B",
      "sources": [
        {
          "identifier": "touch:198",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 123.66667175292969,
                "targetY": 83.33334350585938,
                "t": 1234428.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 123.62239837646484,
                "targetY": 83.33334350585938,
                "t": 1234444.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 123.66667175292969,
                "targetY": 83.16667175292969,
                "t": 1234473.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 122.37760925292969,
                "targetY": 73.39096069335938,
                "t": 1234479.7999999523,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 66.1376953125,
                "t": 1234489.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 66.1376953125,
                "t": 1234492.4000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 120.33333587646484,
                "targetY": 65,
                "t": 1234537.4000000954,
                "item": "popup-default-U_010B"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "angle": 6.103331770766034,
              "cardinal": "n",
              "netDistance": 18.633910275869738,
              "duration": 109.30000019073486,
              "sampleCount": 7,
              "rawDistance": 18.717413241440646
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:198"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_E",
      "sources": [
        {
          "identifier": "touch:199",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 82,
                "targetY": 27.333335876464844,
                "t": 1234638,
                "item": "default-K_E"
              },
              {
                "targetX": 82.08821868896484,
                "targetY": 27.333335876464844,
                "t": 1234668.2000000477,
                "item": "default-K_E"
              },
              {
                "targetX": 82,
                "targetY": 27.333335876464844,
                "t": 1234670.5,
                "item": "default-K_E"
              },
              {
                "targetX": 82,
                "targetY": 27.333335876464844,
                "t": 1234681.5,
                "item": "default-K_E"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 43.5,
              "sampleCount": 4,
              "rawDistance": 0.1764373779296875
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:199"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_L",
      "sources": [
        {
          "identifier": "touch:200",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 314.3333435058594,
                "targetY": 75,
                "t": 1234729.7999999523,
                "item": "default-K_L"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 0,
              "sampleCount": 1,
              "rawDistance": 0
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:200"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_L",
      "sources": [
        {
          "identifier": "touch:201",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 314,
                "targetY": 77,
                "t": 1234843.7999999523,
                "item": "default-K_L"
              },
              {
                "targetX": 314.0882263183594,
                "targetY": 76.95573425292969,
                "t": 1234876.7999999523,
                "item": "default-K_L"
              },
              {
                "targetX": 314,
                "targetY": 77,
                "t": 1234888.0999999046,
                "item": "default-K_L"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 44.299999952316284,
              "sampleCount": 3,
              "rawDistance": 0.19741671271645336
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:201"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_E",
      "sources": [
        {
          "identifier": "touch:202",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 84.33333587646484,
                "targetY": 39.333335876464844,
                "t": 1234956.5,
                "item": "default-K_E"
              },
              {
                "targetX": 84.33333587646484,
                "targetY": 39.333335876464844,
                "t": 1234978.5999999046,
                "item": "default-K_E"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 22.09999990463257,
              "sampleCount": 2,
              "rawDistance": 0
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:202"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_N",
      "sources": [
        {
          "identifier": "touch:203",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 225.33334350585938,
                "targetY": 133.6666717529297,
                "t": 1235109.4000000954,
                "item": "default-K_N"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 0,
              "sampleCount": 1,
              "rawDistance": 0
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:203"
      ]
    }
  ],
  [
    {
      "gestureSetId": "default",
      "matchedId": "initial-tap",
      "linkType": "chain",
      "item": "default-K_T",
      "sources": [
        {
          "identifier": "touch:204",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 150.33334350585938,
                "targetY": 30.333335876464844,
                "t": 1235222.5999999046,
                "item": "default-K_T"
              },
              {
                "targetX": 150.33334350585938,
                "targetY": 30.333335876464844,
                "t": 1235295.5999999046,
                "item": "default-K_T"
              },
              {
                "targetX": 150.33334350585938,
                "targetY": 30.333335876464844,
                "t": 1235305,
                "item": "default-K_T"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "netDistance": 0,
              "duration": 82.40000009536743,
              "sampleCount": 3,
              "rawDistance": 0
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:204"
      ]
    }
  ]
]

jahorton commented 1 week ago

[...] I think this is somewhat better than before, but I am still triggering up-flicks when typing rapidly in normal use with this build. Here is one instance with video and gesture data (per feat(web): add recent-history log to gesture engine #11277 instructions). The issue is with the letter 'c' in the second typing of the word 'excellent' in the video (only instance of 'c' in the JSON data). I know that there probably is some movement in my touches as I type, but it really does feel incidental enough that it shouldn't trigger the up-flick, from a usability perspective. That is, I'd prefer the up-flick to be recognized only as a really deliberate action as it will generally be the less common outcome -- whether that is via timing or distance threshold tweaks I am not sure. Not sure if this relates also to bug(android): No default key selected on longpress menu #11312?

I hear you, but the case you ran into here looks pretty hard to distinguish. Here's where your tap started:

start of 'c' tap

And here's where the tap pretty much stopped moving:

'c' movement end point

I personally have similar issues when using joystick-based controller inputs while playing video games; my input direction is often not in the direction I believe it to be.

[...]

  [
    {
      "gestureSetId": "default",
      "matchedId": "longpress",
      "linkType": "chain",
      "sources": [
        {
          "identifier": "touch:198",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 123.66667175292969,
                "targetY": 129.33334350585938,
                "t": 1234428.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 123.62239837646484,
                "targetY": 129.33334350585938,
                "t": 1234444.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 123.66667175292969,
                "targetY": 129.1666717529297,
                "t": 1234473.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 122.37760925292969,
                "targetY": 119.39096069335938,
                "t": 1234479.7999999523,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 112.1376953125,
                "t": 1234489.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 112.1376953125,
                "t": 1234492.4000000954,
                "item": "default-K_C"
              }
            ],
            "stats": {
              "angle": 6.112537534595476,
              "cardinal": "n",
              "netDistance": 17.44909687601092,
              "duration": 64.30000019073486,
              "sampleCount": 6,
              "rawDistance": 17.521028120934968
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:198"
      ]
    },
    {
      "gestureSetId": "none",
      "matchedId": "subkey-select",
      "linkType": "complete",
      "item": "popup-default-U_010B",
      "sources": [
        {
          "identifier": "touch:198",
          "isFromTouch": true,
          "path": {
            "coords": [
              {
                "targetX": 123.66667175292969,
                "targetY": 83.33334350585938,
                "t": 1234428.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 123.62239837646484,
                "targetY": 83.33334350585938,
                "t": 1234444.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 123.66667175292969,
                "targetY": 83.16667175292969,
                "t": 1234473.0999999046,
                "item": "default-K_C"
              },
              {
                "targetX": 122.37760925292969,
                "targetY": 73.39096069335938,
                "t": 1234479.7999999523,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 66.1376953125,
                "t": 1234489.9000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 120.70345306396484,
                "targetY": 66.1376953125,
                "t": 1234492.4000000954,
                "item": "default-K_C"
              },
              {
                "targetX": 120.33333587646484,
                "targetY": 65,
                "t": 1234537.4000000954,
                "item": "popup-default-U_010B"
              }
            ],
            "wasCancelled": false,
            "stats": {
              "angle": 6.103331770766034,
              "cardinal": "n",
              "netDistance": 18.633910275869738,
              "duration": 109.30000019073486,
              "sampleCount": 7,
              "rawDistance": 18.717413241440646
            }
          },
          "stateToken": "default"
        }
      ],
      "allSourceIds": [
        "touch:198"
      ]
    }
  ]

At the point where the engine "locked in" the longpress:

"stats": {
  "angle": 6.112537534595476,
  "cardinal": "n",
  "netDistance": 17.44909687601092,
  "duration": 64.30000019073486,
  "sampleCount": 6,
  "rawDistance": 17.521028120934968
}

The angle's value is in radians: that's 350.2 degree angle, only 10 degrees off of pure north. 98.5% of the net distance for such an angle is vertically aligned. I believe that this is well within any reasonable tolerance for up-flick detection, partly due to the aforementioned "angle alignment" issues I indicated I personally have when making inputs.

jahorton commented 1 week ago

After a brief in-person discussion... part of the reason for

but it really does feel incidental enough that it shouldn't trigger the up-flick, from a usability perspective.

is that we auto-cancel flicks that don't reach the final required distance. We aren't doing so for the longpress up-flick, so it feels like it triggers far more easily. That's a fair point.

To help rectify this difference, I've adjusted the behavior a bit more.

flick-start will now block further, uh, "true flick" processing if there is a northward flick input without any northward flick subkeys AND the key has defined longpresses.
- It does so with the original, relatively small threshold; after all, we don't want flick processing to block the shortcut outright.
- If no longpresses, it'll still enter flick mode in order to facilitate flick discovery (should a user be trying to see what flicks are available).
longpress will now require the same distance as true flicks in order to trigger its subkey-menu quick-display shortcut, putting the feel much closer to that of flicks.
- If the distance never reaches the 'final' threshold, the longpress never triggers, leaving it a "simple tap" instead.
- We don't want to reach subkey mode and then cancel, as that could outright block any output from the key if no default subkey is defined.

I've added a new parameter to match, though I suppose we could just use the same param as is defined for flick finalization.

This makes things a tad bit more complex, but it allows us to avoid adjusting the internal gesture state model graph - that would be a far riskier change to make at this stage of the game.

Given the changes made...

@keymanapp-test-bot retest all

bharanidharanj commented 1 week ago

Test Results

TEST_UP_FLICKS (FAILED): Retested with the attached PR build (Keyman 17.0.315-beta-test-11306) on an Android 13 mobile device and here is my observation: 1. Using the default Eurolatin keyboard, quickly flick upwards on the base-layer keys to show the subkey menu. 2. I observed that when typing quickly, some keys do not display the subkey menu; instead, only the key preview is shown. 3. Got an error message while quickly long-pressing the base keys to bring up the subkey menu. Seems to be an issue.

bharanidharanj commented 1 week ago

Test Results

TEST_QUICK_TYPING (FAILED): Retested with the attached PR build (Keyman 17.0.315-beta-test-11306) on an Android 13 mobile device and here is my observation: 1. Opened Keyman In-app. 2. Typed rapidly, without trying to use flicks or longpress. However, I got an error message (keyboard error) during the process. Seems to be an issue.

jahorton commented 1 week ago

Took a while to get it, but I got a local reproduction:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'nw')
    at Object.evaluate (specsForLayout.ts:371)
    at PathMatcher.update (pathMatcher.ts:161)
    at GestureMatcher.addContactInternal (gestureMatcher.ts:491)
    at new GestureMatcher (gestureMatcher.ts:167)

The 'flick-start' model shouldn't even be evaluated if the key doesn't support flicks... yet... somehow we ended up with a case where it mattered.

Interestingly, when it triggered, the keyboard locked up on my end; the asyncDispatchQueue aiming to help sort out rapid-typing got locked up due to the error. I'll need to inspect where I missed a try...finally, since I thought those bases were covered. This may be related to #11186.

Edit: it is! And it's actually stupidly easy to reproduce once I investigate it - just tap the period key. It's a non-flick key on a flick-enabled keyboard. Will document more details on a related issue.

jahorton commented 1 week ago

@keymanapp-test-bot retest all

I found the cause of the reported errors and have added a fix with #11321. This PR is now based upon that PR, so the fixes are merged in - the errors should no longer occur.

mcdurdin commented 6 days ago

I have been testing this build, and it is looking really solid -- not really experiencing issues with longpress shortcuts any more, even with rapid typing.

mcdurdin commented 5 days ago

Test Results

TEST_UP_FLICKS (PASS): Hammered on the keyboard with lots of upflicks and normal taps on various keys, particularly the period key. No errors encountered and all looking good.
TEST_QUICK_TYPING (PASS): This is looking good. Not really experiencing issues with longpress shortcuts any more, even with rapid typing.

keyman-server commented 4 days ago

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

keymanapp / keyman

fix(web): longpress shortcut activation should only consider northward part #11306

User Testing

User Test Results

Test Artifacts

Test Results

Test Results

Test Results

Test Results