Keep wheel zoom and double-click reset alive under the box-zoom tool - #344
Conversation
Whole-program LTO plus strip cuts the cdylib ~15% (1.51 -> 1.29 MB); the native scatter bench shows no runtime change beyond +/-15% run-to-run noise. .dynsym survives stripping, so ctypes binding is unaffected (abi_smoke passes 140/140 against the stripped build). panic stays unwinding because lib.rs catches panics at the C ABI; the wasm target keeps its explicit -C panic=abort in release.yml. Profile rationale and the not-adopted PGO lever are recorded in spec/design/rust-engine.md.
The dragMode guard added with the toolbar restyle (#198) blocked the plot wheel and dblclick handlers for every tool except pan, so activating the modebar Box Zoom tool killed cursor-anchored wheel zoom and double-click reset. Per the pan/zoom design spec (§5.5), the drag tool selects the drag gesture only; guard on the deliberate exception instead — 'none', the modebar escape hatch that releases page scroll for embedded charts. Records the contract in spec/api/interaction.md and spec/design/pan-and-zoom-configuration.md (whose 'none' + wheel_zoom example had drifted since #198), and adds a headless-Chromium regression probe covering both the box-zoom tool and the 'none' carve-out.
📝 WalkthroughWalkthroughThe PR preserves wheel zoom and double-click reset across active drag tools except ChangesInteraction behavior
Rust release profile
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing Footnotes
|
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)
371-385: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWheel can now fire mid box-zoom/select drag.
Broadening the guard to
dragMode === "none"means wheel zoom now runs whilebandis active (box-zoom or select-* drag in progress) — previously blocked since band only exists whendragModeisn't"pan".band.d0is captured under the pre-wheel view; if the wheel changes the view mid-drag, the release-timed1is computed under a different mapping while the on-screen rubber-band overlay (tracked in raw screen coordinates) visually desyncs from the repainted chart, producing a box-zoom target that doesn't match what was visually dragged.🐛 Proposed guard
this._listen(c, "wheel", (e) => { // The drag tool never disables the wheel (box-zoom/select drags are // drag-only tools) — except `none`, the modebar's escape hatch that // releases page scroll for embedded charts. if (this.dragMode === "none") return; + // Don't let wheel zoom mutate the view out from under an + // in-progress box-zoom/select rectangle; band.d0 was captured + // under the pre-zoom view. + if (band) return; if (!this._interactionFlag("navigation", true)) return;🤖 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 371 - 385, Prevent the wheel handler from processing events while a box-zoom or select drag is active by adding the active band state to the early-return guard alongside dragMode "none". Keep wheel zoom available when no drag band is active, and preserve the existing navigation, zoom, and wheel_zoom checks and _queueWheelZoom flow.
🧹 Nitpick comments (1)
Cargo.toml (1)
20-21: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin release panic to unwind.
Cargo.tomlleaves the strategy implicit; addpanic = "unwind"in[profile.release]so the C-ABI panic-catching path stays explicit.🤖 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 `@Cargo.toml` around lines 20 - 21, Add panic = "unwind" to the [profile.release] section in Cargo.toml, keeping the release profile’s panic strategy explicit and preserving lib.rs C-ABI panic catching behavior.Source: MCP tools
🤖 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 371-385: Prevent the wheel handler from processing events while a
box-zoom or select drag is active by adding the active band state to the
early-return guard alongside dragMode "none". Keep wheel zoom available when no
drag band is active, and preserve the existing navigation, zoom, and wheel_zoom
checks and _queueWheelZoom flow.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 20-21: Add panic = "unwind" to the [profile.release] section in
Cargo.toml, keeping the release profile’s panic strategy explicit and preserving
lib.rs C-ABI panic catching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 75bdda14-6578-46b5-bf08-531b5cc5913a
📒 Files selected for processing (7)
CHANGELOG.mdCargo.tomljs/src/53_interaction.tsspec/api/interaction.mdspec/design/pan-and-zoom-configuration.mdspec/design/rust-engine.mdtests/test_boxzoom_tool_keeps_wheel_and_reset.py
Problem
After selecting the modebar Box Zoom tool (drag mode
"zoom"), cursor-anchored wheel zoom and double-click view reset both stop working — the chart can only be navigated back out via the toolbar.Cause
The toolbar restyle (#198) added
if (this.dragMode !== "pan") return;to the plot-canvaswheelanddblclickhandlers injs/src/53_interaction.ts. The intent was the deliberatepan ↔ nonemodebar escape hatch (embedded charts release page scroll), but the guard also blocked thezoomand select tools — contradicting the pan/zoom design spec (§5.5: "zoommeans box zoom for drag only. It does not control wheel zoom…") and the axis-band wheel handler, which has no drag-tool guard.Fix
dragMode === "none", instead of!== "pan". Wheel zoom now works under the pan, box-zoom, and select tools; double-click reset works under pan and box-zoom."none"stays fully inert so the modebar pan toggle still hands wheel scroll back to the page.spec/api/interaction.md(flag table + gesture map) andspec/design/pan-and-zoom-configuration.md§5.5, including fixing the staledefault_drag_action="none", wheel_zoom=Trueexample that had promised behavior ENG-10514: Update toolbar style #198 removed.tests/test_boxzoom_tool_keeps_wheel_and_reset.py: a headless-Chromium probe that activates the box-zoom tool, completes a box zoom, then asserts wheel still zooms (andpreventDefaults) and double-click still resets — plus the"none"carve-out in both directions. Verified it fails against the old guard.Testing
node js/build.mjs(typecheck + bundle) cleantest_wheel_gesture_end,test_modebar_select_drill,test_view_state_client, …)pre-commit run --all-files,ruff check .,ruff format --check .all passSummary by CodeRabbit
New Features
Bug Fixes
none.Documentation
Performance