From 0a335464fe61c3445af54e45f74e7787c064a673 Mon Sep 17 00:00:00 2001 From: Jemin Kachhadiya Date: Tue, 25 Aug 2026 12:14:22 -0500 Subject: [PATCH] fix(explorer): stop the canvas growing vertically on every hover The real cause of the reported "hovering zooms the graph vertically" bug, and a genuine defect in the explorer's own code -- not a browser pinch-zoom behavior, which is what the two preceding commits wrongly guessed at. Those guards are reverted here: they addressed a problem that wasn't occurring, and both were actively harmful, stripping the browser's standard Ctrl+scroll and touch-pinch zoom affordance from the widget. canvas.height = N is a *reflected* IDL attribute: assigning it also rewrites the element's "height" content attribute. sizeCanvas() read getAttribute("height") back as its source of truth for the CSS height, so every redraw re-multiplied the already-scaled backing-store height by devicePixelRatio again. mouseleave calls drawMain(), so a single hover in-and-out doubled the canvas. Measured at dpr 2, hover alone, no zoom input: 230 -> 460 -> 920 -> 1840 -> 3680 -> 7360 px. At devicePixelRatio == 1 the multiply is a no-op and the bug cannot be observed at all -- which is why it shipped, and why the earlier headless checks (run at the default scale factor of 1) came back clean and were wrongly read as clearing the chart's own math. Fix: capture each canvas's intended CSS height once into a memoized variable and never re-read the attribute that sizeCanvas itself writes. Verified with headless Chromium at devicePixelRatio 1, 2 and 3: main and overview canvas heights are byte-stable across 20 hover in/out cycles, 15 wheel zooms, reset, and window resizes; the backing store is exactly cssH * dpr at each scale (crisp, not oversized); horizontal zoom still works; the second widget on the page is independent and equally stable. The added regression test asserts the structure (height memoized, never re-read) rather than trying to observe growth, since the bug is invisible at dpr 1 -- confirmed it fails when the old line is reinstated and passes when it isn't. Suite: 268 passed, 1 skipped, 0 failures. --- .../sfm_analysis/report/explorer_render.py | 69 ++++++--------- .../tests/test_report_explorer.py | 88 ++++++++----------- 2 files changed, 64 insertions(+), 93 deletions(-) diff --git a/packages/sfm-analysis/src/sfm_analysis/report/explorer_render.py b/packages/sfm-analysis/src/sfm_analysis/report/explorer_render.py index e2364d8..d10834b 100644 --- a/packages/sfm-analysis/src/sfm_analysis/report/explorer_render.py +++ b/packages/sfm-analysis/src/sfm_analysis/report/explorer_render.py @@ -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; } @@ -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"; @@ -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); @@ -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(); @@ -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) { diff --git a/packages/sfm-analysis/tests/test_report_explorer.py b/packages/sfm-analysis/tests/test_report_explorer.py index 58af890..14d27d4 100644 --- a/packages/sfm-analysis/tests/test_report_explorer.py +++ b/packages/sfm-analysis/tests/test_report_explorer.py @@ -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")