Skip to content

fix(web): better cleanup of cancelled gestures 🐵#9893

Merged
jahorton merged 5 commits intofeature-gesturesfrom
fix/web/cancelled-gesture-cleanup
Oct 31, 2023
Merged

fix(web): better cleanup of cancelled gestures 🐵#9893
jahorton merged 5 commits intofeature-gesturesfrom
fix/web/cancelled-gesture-cleanup

Conversation

@jahorton
Copy link
Copy Markdown
Contributor

@jahorton jahorton commented Oct 30, 2023

Fixes a fun issue I've seen on occasion while developing the feature branch; a cancelled longpress would disrupt the next gesture.

Turns out this was because gesture-selection wasn't getting cleaned up properly whenever an underlying source was cancelled... due to some over-optimization I made a while back. Granted, that was before the whole "robustness" strategy was implemented that accounts for when system disruptions prevent a touch interaction from completing normally.

Turns out that getting rid of some code here actually helps things flow more smoothly. I also needed to patch up the logic for touch-start handling.

There is still a minor issue where disruption during the display of a subkey menu blocks the initial touch from doing anything aside from cancelling the subkey menu. This is due to the state of the engine at the time and isn't possible to cleanly resolve without adding in an extra async layer during touch-start handling... which seems not entirely ideal. Even then... after that one blocked touch, all is normal again. As a recovery case that shouldn't be seen often, we should be fine here.

@keymanapp-test-bot skip

@jahorton jahorton requested a review from mcdurdin as a code owner October 30, 2023 09:17
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Oct 30, 2023 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A17S25 milestone Oct 30, 2023
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Oct 30, 2023

Test: Language Modeling Layer (Common) — TeamCity build failed

Still getting frequent failures on this build...

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Oct 30, 2023

There is still a minor issue where disruption during the display of a subkey menu blocks the initial touch from doing anything aside from cancelling the subkey menu. This is due to the state of the engine at the time and isn't possible to cleanly resolve without adding in an extra async layer during touch-start handling... which seems not entirely ideal. Even then... after that one blocked touch, all is normal again. As a recovery case that shouldn't be seen often, we should be fine here.

I don't really understand this scenario. Can you describe in more detail?

@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Oct 31, 2023

Test: Language Modeling Layer (Common) — TeamCity build failed

Still getting frequent failures on this build...

Just realized the base branch wasn't up to date... that may be related.

@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Oct 31, 2023 •
edited
Loading

There is still a minor issue where disruption during the display of a subkey menu blocks the initial touch from doing anything aside from cancelling the subkey menu. This is due to the state of the engine at the time and isn't possible to cleanly resolve without adding in an extra async layer during touch-start handling... which seems not entirely ideal. Even then... after that one blocked touch, all is normal again. As a recovery case that shouldn't be seen often, we should be fine here.

I don't really understand this scenario. Can you describe in more detail?

The most direct effect:

  1. Whenever a subkey menu is active, all new incoming gestures are blocked until the menu completes.
    • The precise mechanism: the set of legal gestures is changed into the empty set for the duration of the subkey-select gesture stage.
  2. If an interruption prevents a longpress interaction from having its path completed through standard DOM events, that mode is sustained.
  3. So, when a new incoming touch occurs... it was able to signal "hey, we should end the subkey-select mode" - but that only resolved asynchronously.
    • The set of legal gestures was restored once the subkey-select gesture stage is cleared - once that async resolution completes.
  4. Therefore, while the subkey mode's was "cancelled" - that cancellation was still pending when it came time to process the incoming GestureSource as input.
    • Thus, the subkey mode would still be active... preventing the incoming GestureSource from being able to pair up with any gestures.

You might notice a little "past tense" there, though - after work yesterday I realized there was a way to fix even this, and I just pushed the commit for it. Recovery is now instant, and the touch that triggers recovery will function as expected for normal scenarios in addition to triggering said recovery.

The trick was to turn point 3 above "synchronous" by leveraging a few gesture-engine internal mechanisms to signal the cancellation early. The same async mechanisms still process afterward, but they've been prevented from breaking relevant code / interface contracts.

@jahorton jahorton mentioned this pull request Oct 31, 2023
Merged
@jahorton jahorton merged commit 40c67c1 into feature-gestures Oct 31, 2023
@jahorton jahorton deleted the fix/web/cancelled-gesture-cleanup branch October 31, 2023 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

Assignees

No one assigned

Labels

common/web/ common/ epic-gestures fix web/

Projects

None yet

Milestone

A17S25

Development

Successfully merging this pull request may close these issues.

2 participants

@jahorton @mcdurdin