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")