Skip to content

fix(core): use correct mjolnir requireFailure key for recognizers#10323

Open
charlieforward9 wants to merge 3 commits into
visgl:masterfrom
NEW-HEAT:fix/recognizer-require-failure-typo
Open

fix(core): use correct mjolnir requireFailure key for recognizers#10323
charlieforward9 wants to merge 3 commits into
visgl:masterfrom
NEW-HEAT:fix/recognizer-require-failure-typo

Conversation

@charlieforward9
Copy link
Copy Markdown
Collaborator

@charlieforward9 charlieforward9 commented May 22, 2026

Summary

Fixes Deck’s mjolnir recognizer setup so requireFailure relationships are actually passed to EventManager.

Problem

Deck normalized recognizer tuples using requestFailure, but mjolnir reads requireFailure. The typo silently dropped the failure dependencies declared in RECOGNIZERS, so gestures such as two-finger pitch could be beaten by pinch recognition on touch devices.

Change

  • Rename the normalized tuple key from requestFailure to requireFailure.
  • Add regression coverage that constructs a Deck and verifies pinch, pan, and click have the expected blocking recognizers in mjolnir’s requireFail arrays.

Validation

  • Hand-tested on iOS WKWebView with GlobeController: two-finger vertical drag now pitches instead of being interpreted as a pinch/pan wobble.
  • CI is green on the PR.

Merge Notes

Approved. Reviewer-requested regression coverage was added in follow-up commit b8f30a09.

@coveralls
Copy link
Copy Markdown

coveralls commented May 22, 2026

Coverage Status

coverage: 83.386% (-0.004%) from 83.39% — NEW-HEAT:fix/recognizer-require-failure-typo into visgl:master

Copy link
Copy Markdown
Collaborator

@chrisgervang chrisgervang left a comment

Choose a reason for hiding this comment

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

I'm surprised this wasn't caught by typescript on the EventManager's recognizers param. Can that type need to be improved?

Comment thread modules/core/src/lib/deck.ts Outdated
The EventManager's RecognizerTupleNormalized expects `requireFailure`,
but Deck was passing `requestFailure`. The key was silently dropped, so
every requireFailure relationship declared in RECOGNIZERS (pinch waiting
for multipan, single-finger pan waiting for multipan, click waiting for
dblclick) never took effect.

On mobile this caused pinch to fire on any 2-finger touch and beat the
multipan recognizer to the gesture: a 2-finger vertical drag would land
in pinch's `controllerState.zoom({pos, scale: ~1})` instead of
`_onMultiPan → rotate({pos})`, so the camera re-anchored its
longitude/latitude (looked like a pan) and picked up tiny bearing
changes from inter-finger rotation deltas — and pitch never engaged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@charlieforward9 charlieforward9 force-pushed the fix/recognizer-require-failure-typo branch from de24093 to 7501827 Compare May 22, 2026 20:29
@charlieforward9
Copy link
Copy Markdown
Collaborator Author

Re: the TypeScript question — agreed, the type RecognizerTuple in mjolnir.js doesn't catch this because:

  1. RecognizerTupleNormalized is part of a union (Recognizer | RecognizerConstructor | RecognizerTupleNormalized | [...]). When TypeScript checks an object literal against a union it only requires one branch to be assignable, and the excess-property check only fires when the literal's required keys are a strict subset of that branch. requireFailure and recognizeWith are both optional, so the object literal {recognizer, recognizeWith, requestFailure} matched RecognizerTupleNormalized (only recognizer was required) and the extra requestFailure was tolerated.
  2. RecognizerTupleNormalized itself isn't exported from mjolnir.js, so consumers can't satisfies-pin it locally either.

Two tightening options, neither in scope here but I'm happy to follow up:

  • Export RecognizerTupleNormalized from mjolnir.js's public types and add satisfies RecognizerTupleNormalized here. Local change, catches typos in this call site only.
  • Make the RecognizerTuple union's normalized branch exact (e.g. require all three keys, with recognizeWith?: undefined / requireFailure?: undefined made non-optional-but-nullable). Catches typos everywhere but is a breaking change to mjolnir's public type.

Want me to open a follow-up PR for either?

@chrisgervang
Copy link
Copy Markdown
Collaborator

I would like for some coverage in this PR so that the regression doesn't come back again. It could be a test if types can't cover it

@charlieforward9
Copy link
Copy Markdown
Collaborator Author

Added in 725e724 — regression test asserts pinch/pan/click each have the right blocking recognizer in their mjolnir requireFail array.

Construct a Deck and verify pinch, pan, and click each have the
expected blocking recognizer in their requireFail array. Catches the
`requestFailure` typo class of bug without depending on TypeScript
flagging it through mjolnir's union type.
@charlieforward9 charlieforward9 force-pushed the fix/recognizer-require-failure-typo branch from 725e724 to b8f30a0 Compare May 22, 2026 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants