Give every capture-owning gesture the same capture-loss policy - #348
Merged
Conversation
Pointer capture is scoped to one browsing context. When a canvas drag leaves an embedded iframe and the primary mouse button is released in the parent document, the iframe never receives `pointerup` — Chrome reports the lost capture only once the pointer re-enters. The gesture stayed live across that gap, so the next move continued panning as though the button were still held. Finalize the gesture on `lostpointercapture`: a pan ends at its last in-frame view and emits its single `end` event, while an unfinished selection or box-zoom rolls back, having no release coordinate to complete with. A buttonless mouse move back over the canvas is the backstop for browsers that omit the lost-capture notification. Covered by a headless probe asserting the re-entry move applies no further pan delta and that exactly one `end` event ships with its interaction id intact. Axis-band drags take capture the same way and do not carry this backstop yet; recorded in the pan-and-zoom spec.
📝 WalkthroughWalkthroughCanvas gestures now use shared pointer-capture handling. Lost releases finalize pans or roll back coordinate-dependent gestures, including iframe boundary cases. Specifications and browser tests define and verify the resulting behavior. ChangesPointer capture gesture handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Canvas
participant ChartView
participant InteractionHandlers
participant ViewState
Canvas->>ChartView: pointerdown
ChartView->>InteractionHandlers: capture guard and release handles
Canvas->>ChartView: lostpointercapture or buttonless re-entry
ChartView->>InteractionHandlers: invoke gesture loss handler
InteractionHandlers->>ViewState: finalize pan or restore committed state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_view_state_client.py (1)
148-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the actual fallback and assert interaction identity.
Script-created
PointerEvents are untrusted, so this probe cannot cover the Line 283 buttonless-re-entry guard whenlostpointercaptureis omitted. Also,interaction_id != nulldoes not prove the end event retained the pan’s ID. Record the update ID, require equality, assert the exact result shape, and add a trusted-input probe withoutlostpointercapture.Proposed assertion strengthening
const endEvents = []; + const updateEvents = []; view.root.addEventListener("xy:view_change", (event) => { - if (event.detail.source === "pan_drag" && event.detail.phase === "end") { + if (event.detail.source !== "pan_drag") return; + if (event.detail.phase === "end") { endEvents.push(event.detail); + } else if (event.detail.phase === "update") { + updateEvents.push(event.detail); } }); @@ - finalEndKeptInteraction: endEvents[0]?.interaction_id != null + finalEndKeptInteraction: + endEvents[0]?.interaction_id === + updateEvents[updateEvents.length - 1]?.interaction_id && endEvents[0]?.axes?.length > 0, @@ - assert result == {key: True for key in result} + assert result == { + "reentryDidNotPan": True, + "finalEndEmittedOnce": True, + "finalEndKeptInteraction": True, + }Also applies to: 193-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_view_state_client.py` around lines 148 - 185, Strengthen the view-state probe around the pointer helper and final assertions: record the pan interaction ID from the update event, require the end event’s interaction_id to equal it, and assert the exact expected result shape rather than only checking non-null values. Add a separate trusted-input probe that omits lostpointercapture to exercise the buttonless re-entry fallback guarded near the existing pointer handling logic, while preserving the current lost-capture scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_view_state_client.py`:
- Around line 148-185: Strengthen the view-state probe around the pointer helper
and final assertions: record the pan interaction ID from the update event,
require the end event’s interaction_id to equal it, and assert the exact
expected result shape rather than only checking non-null values. Add a separate
trusted-input probe that omits lostpointercapture to exercise the buttonless
re-entry fallback guarded near the existing pointer handling logic, while
preserving the current lost-capture scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab256b8e-8531-4674-8411-b38ae56ed9af
📒 Files selected for processing (5)
js/src/53_interaction.tsspec/api/interaction.mdspec/design/pan-and-zoom-configuration.mdspec/design/view-state.mdtests/test_view_state_client.py
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
It seems this is the fix we want anyway though. Seems weird to half-fix the issue just for panning. |
The previous commit fixed the stuck pan only for the plot canvas, but pointer capture is acquired in five places, and the axis-band drag, the lasso vertex handles and the modebar drag all had identical exposure: a button released outside an embedding iframe never delivers pointerup, so the gesture stayed live and resumed on re-entry. Acquire capture in exactly one place. `ChartView._captureGesturePointer` owns the detector — `lostpointercapture`, plus a buttonless mouse move as the backstop for browsers that delay or omit it — and each gesture declares its own end policy: a pan finalizes at its last in-frame view, gestures needing a release coordinate roll back to their last committed state, and chrome drags simply release. The helper registers through `_listen`, so its listener is visible to both consumers of that registry: `destroy()` sweeps it, and context-loss recovery re-binds it onto the replacement canvas. `_unlisten` detaches via the record's live target, so a handler the canvas swap moved still detaches from the node it ended up on. Escape now releases before dropping its gesture records, which previously stranded a listener that kept firing on later gestures. The backstop is mouse-only, since it reads the primary-button bit; pen and touch rely on `lostpointercapture` alone. That scope is recorded in the pan-and-zoom spec rather than left implicit, and the API spec now states the observable contract instead of the call-site count. Covered by a headless probe driving all five gestures through capture loss. The source-level guard asserts the single acquisition site; which gestures inherit the policy is asserted behaviorally instead of by grepping for a call count.
FarhanAliRaza
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/53_interaction.ts (1)
380-383: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPan gestures ended via
pointercancelorEscapenever finalize, unlike every other termination path. Both sites bypassendGestureWithoutRelease— the helper that implements this PR's own policy of "pan finalizes, selection/box-zoom rolls back" — and instead run unconditional "abandon" cleanup, so an in-progress pan's final position is never sent to the kernel via apan_drag"end" view-change. Since update-phase events are gated onwantsViewChange(), the kernel'sview_state()cache is left stale after either path.js/src/57_viewstate.ts's axis-bandend()shows the correct pattern: unifypointerup/pointercancel/capture-loss through one function that always finalizes a pan regardless of how it ended.
js/src/53_interaction.ts#L380-L383: changethis._listen(c, "pointercancel", cancelPointerGesture);tothis._listen(c, "pointercancel", endGestureWithoutRelease);.js/src/53_interaction.ts#L423-L437: replace the inlineband?.capture.release(); drag?.capture.release(); ...; band = null; drag = null;block with a call toendGestureWithoutRelease();.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/src/53_interaction.ts` around lines 380 - 383, Pan gestures terminated by pointercancel or Escape bypass endGestureWithoutRelease, leaving the kernel view state stale. In js/src/53_interaction.ts lines 380-383, route pointercancel to endGestureWithoutRelease instead of cancelPointerGesture; in lines 423-437, replace the inline capture-release and state-reset cleanup with endGestureWithoutRelease so pan gestures finalize while selection and box-zoom retain the established rollback behavior.
🧹 Nitpick comments (1)
tests/test_view_state_client.py (1)
156-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThis probe still misses the trusted buttonless-reentry fallback. The
dispatchEvent()-driven pointer events here are untrusted, so themoveEvent.isTrustedbranch in_captureGesturePointernever runs. Add a trusted-input path (for example via CDP/Playwright) or a separate test for that fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_view_state_client.py` around lines 156 - 229, The pointer-event probe only exercises untrusted dispatchEvent events and therefore does not cover the moveEvent.isTrusted fallback in _captureGesturePointer. Add a separate trusted-input test using CDP/Playwright, or otherwise invoke a browser-generated trusted buttonless reentry, and verify that the active gesture does not continue panning after capture loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@js/src/53_interaction.ts`:
- Around line 380-383: Pan gestures terminated by pointercancel or Escape bypass
endGestureWithoutRelease, leaving the kernel view state stale. In
js/src/53_interaction.ts lines 380-383, route pointercancel to
endGestureWithoutRelease instead of cancelPointerGesture; in lines 423-437,
replace the inline capture-release and state-reset cleanup with
endGestureWithoutRelease so pan gestures finalize while selection and box-zoom
retain the established rollback behavior.
---
Nitpick comments:
In `@tests/test_view_state_client.py`:
- Around line 156-229: The pointer-event probe only exercises untrusted
dispatchEvent events and therefore does not cover the moveEvent.isTrusted
fallback in _captureGesturePointer. Add a separate trusted-input test using
CDP/Playwright, or otherwise invoke a browser-generated trusted buttonless
reentry, and verify that the active gesture does not continue panning after
capture loss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: afe5fb77-a839-4b01-8172-d659c16d3c3d
📒 Files selected for processing (8)
js/src/50_chartview.tsjs/src/53_interaction.tsjs/src/57_viewstate.tsspec/api/interaction.mdspec/design/pan-and-zoom-configuration.mdspec/design/view-state.mdtests/test_static_client_security.pytests/test_view_state_client.py
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/design/view-state.md
- spec/api/interaction.md
masenf
approved these changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #212.
The bug
Pan a chart, move the cursor out of the chart area, release the button outside
it — on returning, the chart keeps panning as though the button were still
held.
Pointer capture is scoped to a single browsing context. When a drag leaves an
embedded iframe and the primary mouse button is released in the parent
document, the iframe never receives
pointerup; Chrome reports the lostcapture only once the pointer re-enters. The gesture stayed live across that
gap, so the next
pointermoveresumed it.The plot canvas is where this is reported, but capture is acquired in five
places, and the axis-band drag, the lasso vertex handles and the modebar drag
all had identical exposure. Fixing only the pan would have left the same bug
reachable three other ways.
The fix
Capture is now acquired in exactly one place.
ChartView._captureGesturePointerowns the detector —lostpointercapture,plus a buttonless mouse move as the backstop for browsers that delay or omit it
— and each gesture declares its own end policy:
endevent withinteraction_idintact, so history still coalesces the gesture into a single entryRe-entering the chart never resumes a gesture.
Listener lifecycle
The helper registers through the existing
_listenregistry rather than aprivate
addEventListener, because two subsystems depend on that registrybeing complete:
destroy()sweeps it, and WebGL context-loss recovery walks itto re-bind canvas handlers onto the replacement canvas.
_unlistendetachesvia the record's live target, so a handler the canvas swap moved still
detaches from the node it ended up on.
Escape also now releases before dropping its gesture records. Previously it
nulled them while the capture object was the only handle on its listener,
stranding a listener that stayed armed and re-fired on later gestures.
Scope limit, recorded
The buttonless-move backstop is mouse-only — it reads the primary-button
bit, which has no pen/touch equivalent. Pen and touch rely on
lostpointercapturealone. Recorded inspec/design/pan-and-zoom-configuration.mdrather than left implicit.Coverage
test_capture_loss_policy_covers_every_capture_owning_gesturedrives all fivegestures through a capture loss in headless Chromium and asserts each one's end
policy.
The source-level guard in
test_static_client_security.pyasserts the durableinvariant — exactly one
setPointerCapturesite, so capture-loss handlingcannot be opted out of. Which gestures inherit the policy is asserted
behaviorally by the probe rather than by grepping for a call count.
Specs
Updated per the repo's spec-as-source-of-truth rule:
spec/api/interaction.md(observable contract), and the mechanism plus itsmouse-only scope in
spec/design/pan-and-zoom-configuration.mdandspec/design/view-state.md.Verification
node js/build.mjs— typechecks and rebuildsuv run pytest— 3187 passed, 1 skippedscripts/abi_smoke.py,scripts/render_smoke_nonumpy.py— pass (the rendersmoke exercises context-loss canvas swaps and
destroy(), the paths_unlistentouches)pre-commit run --all-files,ruff check .,ruff format --check .— cleanNote for reviewers
Two things I deliberately did not do, both worth their own change:
_beginGesture(owner, event, {onMove, onEnd(reason)})that ownspointerup/pointercanceltoo. Today the helper centralizes the detector;each gesture still hand-writes its
onLostand its ownrelease()in thenormal end path, so a sixth gesture could still forget one. That would be a
redesign of all five gestures.
_initInteractionclosure into per-gesturebinders.
Separately:
tests/reflex_adapter/test_assets.py::test_client_source_is_the_installed_bundlefails locally because it expects unminified markers in
static/index.jswhilejs/build.mjsemits a minified bundle. It is pre-existing — it failsidentically with this branch's changes stashed — and untouched here.