fix(core): disable doubleClickZoom by default to eliminate 300ms click delay#10422
fix(core): disable doubleClickZoom by default to eliminate 300ms click delay#10422chrisgervang wants to merge 6 commits into
Conversation
|
Coverage is 83.139% — chr/deckgl-controller-gesture-delays into x/mjolnir-up. No base build found for |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f567030. Configure here.
…k delay The click recognizer's requireFailure dependency on dblclick causes mjolnir.js to defer click events by 300ms. Disabling doubleClickZoom by default removes this delay while keeping doubleClickDragZoom available for zoom-by-drag gestures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Same principle — once #10416 lands, dblclickdrag should be in click's requireFailure list to prevent spurious clicks before a drag-zoom. That would reintroduce the 300ms delay, so disable by default too. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents spurious click events from firing before a double-click-drag zoom gesture is recognized. Without this, a click would emit on the first tap before the recognizer determines the user intends to drag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f567030 to
e250fc9
Compare
Tests verify through the full EventManager → recognizer pipeline that: - Click fires immediately when doubleClickZoom and doubleClickDragZoom are both disabled (the new default) - Click is delayed ~300ms when doubleClickZoom is enabled - Click is delayed ~300ms when doubleClickDragZoom is enabled Also reorders RECOGNIZERS so dblclickdrag is registered before click, which is required for click's requireFailure reference to resolve. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…m is false The dblclick/dblclickdrag recognizers start with enable:false so they don't cause the 300ms click delay out of the box. Deck's picking system uses watch() (passive) for the dblclick event so it doesn't force-enable the recognizer. Only the controller's doubleClickZoom option enables it. Also adds integration tests that verify click latency at the Deck level using real pointer events dispatched to the canvas. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if (eventType === 'dblclick') { | ||
| // Use watch (passive) so the dblclick recognizer is only enabled by the | ||
| // controller's doubleClickZoom option — not by the picking system. | ||
| eventManager.watch(eventType, this._onEvent); |
There was a problem hiding this comment.
@Pessimistress while trying to unit test this, the LLM gathered some interesting context and proposed this change
The
watchchange atdeck.ts:1090is necessary to makedoubleClickZoom: falseactually eliminate the click delay.The problem: Deck registers handlers for both
clickanddblclickevents (for theonClickprop — it fires on both single and double clicks for picking). UsingeventManager.on('dblclick', ...)enables thedblclickrecognizer. The click recognizer hasdblclickin itsrequireFailurelist, so whendblclickis enabled,hasRequireFailures()returns true and the TapRecognizer adds a 300mssetTimeoutbefore emitting click.Even with
controller: {doubleClickZoom: false}, the controller callseventManager.off('dblclick', handler)— but the Deck-levelon('dblclick', ...)handler still exists, keeping the recognizer enabled and the delay in place.The fix:
watch()registers a passive handler that still receives dblclick events (soonClickstill fires on double-clicks whendoubleClickZoom: true), but passive handlers don't count towardisEmpty()— so they don't force-enable the recognizer. Only the controller'son/off(driven bydoubleClickZoom) controls whether the dblclick recognizer is active.Without this change, the
enable: falsedefault on the dblclick recognizer gets immediately overridden by Deck'son('dblclick', ...)call, and the 300ms click delay persists regardless of the controller setting.
I'm still investigating and learning about this area of deck..
The dblclick tests were broken because page.mouse.dblclick() doesn't properly target the canvas inside vitest's browser iframe. Fixed by dispatching PointerEvents directly on the canvas element, matching how mjolnir's PointerEventInput listener works (pointerdown on element, pointerup on window). Each test uses a fresh Deck instance for isolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
doubleClickZoomby default (falseinstead oftrue) to eliminate the 300ms click delay caused by mjolnir.js's TapRecognizerrequireFailuremechanismdoubleClickDragZoomby default for the same reason — once feat(core): upgrade mjolnir.js for DoubleClickDrag recognizer #10416 lands,dblclickdragshould be inclick'srequireFailurelist to prevent spurious click events before a drag-zoom begins, which would reintroduce the 300ms delayBackground
When
doubleClickZoomis enabled, theclickrecognizer has arequireFailure: ['dblclick']dependency. This causes mjolnir.js's TapRecognizer to defer click emission by 300ms (theintervaltimeout) while waiting to see if a double-click follows. Disabling thedblclickrecognizer causeshasRequireFailures()to returnfalse(it only counts enabled recognizers), allowing clicks to fire immediately.The same principle applies to
doubleClickDragZoom— a single click is ambiguous with the first tap of a double-click-drag, so thedblclickdragrecognizer should also be inclick'srequireFailurelist (to be added with #10416). That means enabling it would also reintroduce the 300ms delay.See discussion in #10388 and the revert in #10420.
Relates to #10384
Test plan
controller: { doubleClickZoom: true }controller: { doubleClickDragZoom: true }🤖 Generated with Claude Code
Note
Medium Risk
Changes default controller behavior for all consumers (double-click zoom off unless opted in); interaction timing and gesture disambiguation are user-facing and easy to miss in manual testing.
Overview
Default map interaction behavior changes:
doubleClickZoomanddoubleClickDragZoomnow default tofalseinstead oftrue, so single clicks on the deck canvas fire immediately instead of waiting ~300ms for the tap recognizer to rule out double-click gestures.The controller defaults and API docs now call out that turning either option back on reintroduces that click latency. Recognizer wiring in
RECOGNIZERSis adjusted soclickcoordinates withdblclick/dblclickdragviarequireFailurewhen those features are enabled.Tests add EventManager-level coverage: immediate clicks with both zoom options off, and ~300ms delay when either is on.
Reviewed by Cursor Bugbot for commit b2b9627. Bugbot is set up for automated code reviews on this repo. Configure here.