From 279e6b832dd9dfc5aa584ccc285e9ae25b1a6d75 Mon Sep 17 00:00:00 2001 From: "Joshua A. Horton" Date: Mon, 30 Oct 2023 15:58:05 +0700 Subject: [PATCH 1/4] fix(web): better cleanup of cancelled gestures --- .../gestures/matchers/matcherSelector.ts | 31 ++----------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/matcherSelector.ts b/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/matcherSelector.ts index 3a4c0d1a5f..193b0c0348 100644 --- a/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/matcherSelector.ts +++ b/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/matcherSelector.ts @@ -325,22 +325,10 @@ export class MatcherSelector extends EventEmitter> { return async (result: MatchResult) => { // Note: is only called by GestureMatcher Promises that are resolving. - /* - * If we already had a gesture stage match, this will have already been fulfilled; - * bypass all match-handling. Capturing `matchSynchronization` in a closure in this - * manner is important to ensure that the returned handler is "locked" to the - * currently-processing gesture stage. - */ - for(let synchronizer of matchSynchronizers) { - if(synchronizer.isFulfilled) { - return; - } - } + // Do not bypass match handling just because a synchronization promise is fulfilled. + // If a source was force-cancelled, cascading to a call of this handler, we still + // need to perform internal state cleanup. - /* If cancellation was requested but not pre-filtered by the synchronizer setup, replace - * the result object. The matcher's Promise may have resolved simultaneously with the - * winner but 'lost', a scenario that may require careful handling to clean up. - */ if(matcher.isCancelled) { result = { matched: false, @@ -371,11 +359,6 @@ export class MatcherSelector extends EventEmitter> { return; } - if(matcher.isCancelled) { - // Fortunately, the rest of the code will help us recover from the state. - console.warn("Unexpected state: a cancelled GestureMatcher was still listed as a possibility"); - } - this.potentialMatchers.splice(matcherIndex, 1); /* @@ -480,14 +463,6 @@ export class MatcherSelector extends EventEmitter> { /* * Fulfills the contract set by `matchGesture`. - * - * Also, fulfilling the ManagedPromise acts as a synchronizer, partially facilitating the - * guarantee at the start of this closure. It's set synchronously, so other gesture-matchers - * that call into this method will know that a match has already fulfilled for the matched - * source(s). Any further matchers will be silently ignored, effectively cancelling them. - * However, this fails to handle the case where two separate calls to matcherSelectionFilter - * occur for the same matcher due to one source being added at a later point in time; - * this is what the `cancel` */ tracker.matchPromise.resolve({matcher, result}); } From 2e0f6b061503e7fcd1faf9a70468f31010d98b98 Mon Sep 17 00:00:00 2001 From: "Joshua A. Horton" Date: Mon, 30 Oct 2023 16:14:59 +0700 Subject: [PATCH 2/4] fix(web): proper touch-start robustness handling --- common/web/gesture-recognizer/src/engine/touchEventEngine.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/web/gesture-recognizer/src/engine/touchEventEngine.ts b/common/web/gesture-recognizer/src/engine/touchEventEngine.ts index fcf554cb71..9d6ac4995f 100644 --- a/common/web/gesture-recognizer/src/engine/touchEventEngine.ts +++ b/common/web/gesture-recognizer/src/engine/touchEventEngine.ts @@ -105,7 +105,7 @@ export class TouchEventEngine extends InputEv const newTouches = touchListToArray(event.changedTouches); // Maintain all touches in the `.touches` array that are NOT marked as `.changedTouches` (and therefore, new) this.maintainTouchpointsWithIds(allTouches - .filter((touch) => (newTouches.indexOf(touch) == -1)) + .filter((touch1) => newTouches.findIndex(touch2 => touch1.identifier == touch2.identifier) == -1) .map((touch) => touch.identifier) ); From b940e9010dffd0d40d0c353e7978eeca01ccee6c Mon Sep 17 00:00:00 2001 From: "Joshua A. Horton" Date: Mon, 30 Oct 2023 16:22:24 +0700 Subject: [PATCH 3/4] fix(web): gesture-engine null-ref check for doc keyboards --- web/src/engine/osk/src/visualKeyboard.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/web/src/engine/osk/src/visualKeyboard.ts b/web/src/engine/osk/src/visualKeyboard.ts index f0c86b1aad..55b6ef8413 100644 --- a/web/src/engine/osk/src/visualKeyboard.ts +++ b/web/src/engine/osk/src/visualKeyboard.ts @@ -217,7 +217,11 @@ export default class VisualKeyboard extends EventEmitter implements Ke throw new Error(`Keyboard ${this.layoutKeyboard.id} does not have a layer with id ${value}`); } else { this._layerId = value; - this.gestureEngine.stateToken = value; + + // Does not exist for documentation keyboards! + if(this.gestureEngine) { + this.gestureEngine.stateToken = value; + } } if(changedLayer) { From 0fa0520962b0ab139c665ee92becdf21bbd4036e Mon Sep 17 00:00:00 2001 From: "Joshua A. Horton" Date: Tue, 31 Oct 2023 08:46:30 +0700 Subject: [PATCH 4/4] feat(web): further enhances interruption recovery --- .../gestures/matchers/gestureSequence.ts | 36 ++++++++++++------- .../engine/headless/touchpointCoordinator.ts | 11 ++++-- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/gestureSequence.ts b/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/gestureSequence.ts index c4383ac402..790a5febdc 100644 --- a/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/gestureSequence.ts +++ b/common/web/gesture-recognizer/src/engine/headless/gestures/matchers/gestureSequence.ts @@ -96,6 +96,7 @@ export class GestureSequence extends EventEmitter> { private pushedSelector?: MatcherSelector; private gestureConfig: GestureModelDefs; + private markedComplete: boolean = false; // Note: the first stage will be available under `stageReports` after awaiting a simple Promise.resolve(). constructor( @@ -110,6 +111,16 @@ export class GestureSequence extends EventEmitter> { this.selector = selector; this.selector.on('rejectionwithaction', this.modelResetHandler); this.once('complete', () => { + if(this.pushedSelector) { + // The `popSelector` method is responsible for triggering cascading cancellations if + // there are nested GestureSequences. + // + // As this tends to affect which gestures are permitted, it's important this is done + // any time the GestureSequence is cancelled or completed, for any reason. + this.touchpointCoordinator?.popSelector(this.pushedSelector); + this.pushedSelector = null; + } + this.selector.off('rejectionwithaction', this.modelResetHandler); this.selector.dropSourcesWithIds(this.allSourceIds); @@ -193,13 +204,10 @@ export class GestureSequence extends EventEmitter> { }); if(!selection.result.matched) { - if(this.pushedSelector) { - // The `popSelector` method is responsible for triggering cascading cancellations if - // there are nested GestureSequences. - this.touchpointCoordinator?.popSelector(this.pushedSelector); + if(!this.markedComplete) { + this.markedComplete = true; + this.emit('complete'); } - - this.emit('complete'); return; } } @@ -274,13 +282,11 @@ export class GestureSequence extends EventEmitter> { } } } else { - if(this.pushedSelector) { - this.touchpointCoordinator?.popSelector(this.pushedSelector); - this.pushedSelector = null; - } - // Any extra finalization stuff should go here, before the event, if needed. - this.emit('complete'); + if(!this.markedComplete) { + this.markedComplete = true; + this.emit('complete'); + } } } @@ -305,7 +311,11 @@ export class GestureSequence extends EventEmitter> { public cancel() { const sources = this.stageReports[this.stageReports.length - 1].sources; - sources.forEach((src) => src.terminate(true)); + sources.forEach((src) => src.baseSource.isPathComplete || src.baseSource.terminate(true)); + if(!this.markedComplete) { + this.markedComplete = true; + this.emit('complete'); + } } } diff --git a/common/web/gesture-recognizer/src/engine/headless/touchpointCoordinator.ts b/common/web/gesture-recognizer/src/engine/headless/touchpointCoordinator.ts index 312378f8fb..5823777213 100644 --- a/common/web/gesture-recognizer/src/engine/headless/touchpointCoordinator.ts +++ b/common/web/gesture-recognizer/src/engine/headless/touchpointCoordinator.ts @@ -173,6 +173,15 @@ export class TouchpointCoordinator extends Even touchpoint.path.on('invalidated', () => { // GestureSequence _should_ handle any other cleanup internally as fallout // from the path being cancelled. + // + // That said, it's handled asynchronously... but we can give a synchronous signal + // through the next block of code, allowing cleanup to occur earlier during + // recovery states. + + const owningSequence = this.activeGestures.find((entry) => entry.allSourceIds.includes(touchpoint.identifier)); + if(owningSequence) { + owningSequence.cancel(); + } // To consider: should it specially mark if it 'completed' due to cancellation, // or is that safe to infer from the tracked GestureSource(s)? @@ -183,8 +192,6 @@ export class TouchpointCoordinator extends Even this._activeSources = this._activeSources.splice(i, 1); }); touchpoint.path.on('complete', () => { - // TODO: on cancellation, is there any other cleanup to be done? - // Also mark the touchpoint as no longer active. let i = this._activeSources.indexOf(touchpoint); this._activeSources = this._activeSources.splice(i, 1);