Skip to content

fix(explorer): stop the canvas growing vertically on every hover #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeminkachhadiya merged 1 commit into from
Aug 25, 2026
Merged

fix(explorer): stop the canvas growing vertically on every hover #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 27 additions & 42 deletions packages/sfm-analysis/src/sfm_analysis/report/explorer_render.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,9 @@
.sfm-explorer .toolbar button:hover { background: var(--gridline); }
.sfm-explorer .sfm-readout { font-variant-numeric: tabular-nums; color: var(--ink-primary); }
.sfm-explorer .sfm-readout b { font-weight: 600; }
/* touch-action: none on both canvas wraps (border/padding included, not
just the canvas element itself) -- a touch-pinch or scroll gesture
starting a few pixels into the border/background is claimed by this
widget's own zoom/pan handling (see EXPLORER_JS), not the browser's
native page zoom, matching the wheel/gesture guards below which are
likewise scoped to the whole widget rather than just the canvases. */
.sfm-explorer .canvas-wrap {
position: relative; border: 1px solid var(--gridline); border-radius: 4px;
background: var(--surface); overflow: hidden; touch-action: none;
background: var(--surface); overflow: hidden;
}
.sfm-explorer canvas { display: block; width: 100%; }
.sfm-explorer .sfm-overview { cursor: grab; }
Expand Down Expand Up @@ -203,10 +197,31 @@ def explorer_bootstrap_js(entries) -> str:

function dpr() { return window.devicePixelRatio || 1; }

function sizeCanvas(canvas) {
// The canvas's intended *CSS* height, captured once and then held in a
// JS variable -- never re-read from the element after the first call.
//
// This matters because `canvas.height = N` (in sizeCanvas below) is a
// reflected IDL attribute: assigning it also rewrites the element's
// "height" *content attribute*. Reading getAttribute("height") back as
// the source of truth on the next redraw would therefore return the
// already-scaled backing-store height and multiply it by
// devicePixelRatio again, compounding on every single redraw -- the
// canvas doubles on each hover (mouseleave calls drawMain) on any
// HiDPI display. It's exactly invisible at devicePixelRatio == 1,
// where the multiply is a no-op, which is what makes it easy to miss.
var baseCssH = {};
function baseCssHeight(canvas, key) {
if (baseCssH[key] === undefined) {
baseCssH[key] = parseFloat(canvas.getAttribute("height")) ||
canvas.getBoundingClientRect().height || 1;
}
return baseCssH[key];
}

function sizeCanvas(canvas, key) {
var rect = canvas.getBoundingClientRect();
var ratio = dpr();
var cssH = parseFloat(canvas.getAttribute("height")) || rect.height;
var cssH = baseCssHeight(canvas, key);
canvas.width = Math.max(1, Math.round(rect.width * ratio));
canvas.height = Math.max(1, Math.round(cssH * ratio));
canvas.style.height = cssH + "px";
Expand Down Expand Up @@ -363,13 +378,13 @@ def explorer_bootstrap_js(entries) -> str:
}

function drawMain() {
var dims = sizeCanvas(mainCanvas);
var dims = sizeCanvas(mainCanvas, "main");
drawFrame(mainCtx, dims, view.t0, view.t1, { showLabels: true, showAxis: true });
if (brush) drawBrush(dims);
}

function drawOverview() {
var dims = sizeCanvas(overviewCanvas);
var dims = sizeCanvas(overviewCanvas, "overview");
drawFrame(overviewCtx, dims, 0, duration, { showLabels: false, showAxis: false, laneH: Math.max(2, laneGap(dims.h) * 0.6) });
var sc = xScale(0, duration, dims.w);
var x0 = sc.toPx(view.t0), x1 = sc.toPx(view.t1);
Expand Down Expand Up @@ -466,11 +481,7 @@ def explorer_bootstrap_js(entries) -> str:
window.addEventListener("mouseup", mainMouseUp);
mainCanvas.addEventListener("mouseleave", mainMouseLeave);

// wheel: zoom at cursor -- horizontal (time) only, never vertical. Every
// wheel/trackpad delta over the canvas (including a pinch or Ctrl+wheel,
// which arrives as a wheel event with ctrlKey/metaKey set) maps to the
// same one-dimensional (view.t0, view.t1) change; there is no vertical
// view-state field anywhere in this module for a "vertical zoom" to mean.
// wheel: zoom at cursor
mainCanvas.addEventListener("wheel", function (e) {
e.preventDefault();
var rect = mainCanvas.getBoundingClientRect();
Expand All @@ -484,32 +495,6 @@ def explorer_bootstrap_js(entries) -> str:
setView(t0, t0 + newSpan);
}, { passive: false });

// A pinch gesture or Ctrl+wheel that lands a few pixels off the canvas
// -- on the toolbar, the legend, the hint text, anywhere else in this
// widget -- has no listener above to stop it, so the browser's own
// page zoom fires instead: the whole page (this chart included) scales
// in both directions at once, which reads as "the graph zoomed
// vertically too" even though this module never implements a vertical
// zoom. Block that specifically (ctrlKey/metaKey wheel = the browser's
// pinch/Ctrl+scroll zoom signal) anywhere within the widget, without
// touching plain vertical page-scroll wheel events elsewhere in it.
root.addEventListener("wheel", function (e) {
if (e.ctrlKey || e.metaKey) e.preventDefault();
}, { passive: false });

// Safari/WebKit's trackpad pinch does not go through "wheel" at all --
// it fires its own proprietary, non-standard gesturestart/gesturechange/
// gestureend events instead (still absent from every other browser's
// event model), so the ctrlKey-wheel guard above cannot see or stop it.
// A user describing this as "just hovering" triggering an unwanted
// vertical zoom is the classic symptom of unguarded Safari trackpad
// pinch: resting/moving fingers on the trackpad while the cursor sits
// over the widget can register as a pinch even without a deliberate
// scroll gesture. Harmless no-op on every non-WebKit browser.
["gesturestart", "gesturechange", "gestureend"].forEach(function (name) {
root.addEventListener(name, function (e) { e.preventDefault(); }, { passive: false });
});

// ---- overview: drag to pan ----
var panDrag = null;
overviewCanvas.addEventListener("mousedown", function (e) {
Expand Down
88 changes: 37 additions & 51 deletions packages/sfm-analysis/tests/test_report_explorer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,59 +166,45 @@ def test_css_scoped_under_sfm_explorer(self):
continue
assert ".sfm-explorer" in rule, f"unscoped rule: {rule!r}"

def test_wheel_zoom_is_horizontal_only(self):
"""Regression test for a real bug: a pinch/Ctrl+wheel gesture that
lands a few pixels off the canvas -- on the toolbar, the legend,
the hint text -- has no handler to stop it there, so the browser's
own page zoom fires and scales the whole page (this chart
included) in both directions, which reads as "the graph zoomed
vertically too" even though nothing in EXPLORER_JS's view state
(view.t0/view.t1 only) can represent a vertical zoom at all.

No Node available in this environment to execute the JS, so this
is a structural check on the source -- confirmed against a real
headless-Chromium repro during development: before the fix, a
synthetic ctrlKey wheel event over the toolbar/hint was NOT
defaultPrevented (native zoom would fire there); after, it is,
everywhere in the widget, while the canvas's own horizontal-only
zoom is unaffected.
def test_canvas_css_height_is_never_re_read_from_the_element(self):
"""Regression test for a real bug: the canvas grew vertically on
every hover, on HiDPI displays only.

``canvas.height = N`` is a *reflected* IDL attribute -- assigning
it also rewrites the element's "height" content attribute. The
original sizeCanvas() read ``getAttribute("height")`` back as its
source of truth for the CSS height, so each redraw re-multiplied
the already-scaled backing-store height by devicePixelRatio
again. mouseleave calls drawMain(), so one hover in-and-out
doubled the canvas: measured 230 -> 460 -> 920 -> 1840 -> 3680 ->
7360 px over five hovers at dpr 2, with no zoom input at all.

At devicePixelRatio == 1 the multiply is a no-op and the bug is
perfectly invisible -- which is exactly why it shipped, and why
this test asserts the *structure* (height is captured once into a
variable, never re-read) rather than trying to observe growth.
"""
assert "var view = { t0: 0, t1: duration };" in EXPLORER_JS
# No vertical view-state field exists anywhere for a wheel/pinch
# handler to zoom.
assert re.search(r"\bview\.(y0|y1|scaleY|zoomY)\b", EXPLORER_JS) is None

# The canvas's own zoom handler changes only t0/t1.
main_wheel = re.search(
r'mainCanvas\.addEventListener\("wheel".*?\}, \{ passive: false \}\);',
EXPLORER_JS, re.DOTALL,
assert "function sizeCanvas" in EXPLORER_JS
body = re.search(r"function sizeCanvas\([^)]*\)\s*\{(.*?)\n \}", EXPLORER_JS, re.DOTALL)
assert body is not None
# The CSS height must come from the memoized helper, not a live
# read of the attribute that this same function writes to.
assert 'getAttribute("height")' not in body.group(1), (
"sizeCanvas reads back the height attribute it also writes -- "
"this re-scales by devicePixelRatio on every redraw"
)
assert main_wheel is not None
assert "setView(t0, t0 + newSpan)" in main_wheel.group(0)

# A second, container-level handler blocks the browser's native
# pinch/Ctrl+scroll zoom signal (ctrlKey/metaKey wheel) anywhere
# in the widget -- not just exactly over the canvas.
root_wheel = re.search(
r'root\.addEventListener\("wheel".*?\}, \{ passive: false \}\);',
EXPLORER_JS, re.DOTALL,
)
assert root_wheel is not None
assert "e.ctrlKey" in root_wheel.group(0) and "e.metaKey" in root_wheel.group(0)
assert "e.preventDefault()" in root_wheel.group(0)

# Safari/WebKit's trackpad pinch doesn't go through "wheel" at
# all -- it fires its own gesturestart/gesturechange/gestureend
# events, invisible to the ctrlKey-wheel guard above. A user
# describing this as "just hovering" triggering an unwanted zoom
# is the classic symptom of unguarded Safari trackpad pinch.
for name in ("gesturestart", "gesturechange", "gestureend"):
assert f'"{name}"' in EXPLORER_JS, f"no guard registered for {name}"

def test_canvas_wrap_blocks_native_touch_gestures(self):
rule = re.search(r"\.sfm-explorer \.canvas-wrap\s*\{([^}]*)\}", EXPLORER_CSS)
assert rule is not None
assert "touch-action: none" in rule.group(1)
assert "baseCssHeight(" in body.group(1)

# The one permitted read lives in the memoizing helper, guarded so
# it can only ever run before the first write.
helper = re.search(r"function baseCssHeight\([^)]*\)\s*\{(.*?)\n \}", EXPLORER_JS, re.DOTALL)
assert helper is not None
assert "undefined" in helper.group(1), "baseCssHeight must memoize, not re-read"
# Exactly one *executable* read in the whole module (the one in
# baseCssHeight above). Strip // comments first -- the explanation
# of this very bug mentions getAttribute("height") in prose.
code_only = re.sub(r"//[^\n]*", "", EXPLORER_JS)
assert code_only.count('getAttribute("height")') == 1

def test_bootstrap_js_round_trips_entries(self, tmp_path):
run, m = _run_and_metrics(tmp_path, session="RoundTrip")
Expand Down
Loading