From 483148ed1f0ec09566e20182e347b35bb718be83 Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Thu, 30 Jul 2026 04:50:01 +0000 Subject: [PATCH 1/6] Give the polar coordinate system CodSpeed coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodSpeed on #370 reported "103 untouched benchmarks" for a change that added a whole coordinate system, rewrote wedge geometry in three renderers, and shipped the most expensive mark in the engine. Nothing in the suite could see any of it, which is why a ~50k-polar-bar performance cliff was found by hand instead. `benchmarks/test_codspeed_polar.py` — six rows, all Python cost (what simulation mode measures): - payload prep for the three shapes with materially different validation and emit paths: a 100k polar line, a 16-sector / 50k-observation wind rose (Python-side binning plus stacked wedges), and a 24-slice pie, whose unequal widths take the four-edge column path rather than the compact scalar-width one; - SVG and native-PNG export of the same rose, bracketing the arc-flattening term. SVG draws real `A` arcs and needs no subdivision count, so it is the control; PNG flattens every wedge at `config.polar_bar_segments(span, turn)` vertices, so a regression back to a flat full-turn count lands here as an arc-flattening step change rather than as a bug report; - a polar heatmap's bounded screen-space inverse raster, which has no Cartesian twin. The payload rows assert bounds rather than sizes — the rose's bytes must stay bounded by sector and band count, never by observation count — so a row cannot get cheaper by shipping a different chart. The report's other item was 2 skipped rows falling back to baseline results. Those are orphans in the dashboard, not skipped tests: the suite collects exactly 103 rows and CodSpeed measured 103, so the 2 extra rows have no code behind them and need archiving there by hand. What the repo can do is stop it recurring — `test_codspeed_row_count_matches_the_methodology_spec` collects the modules by AST and gates the total against `spec/benchmarks/methodology.md` §8, whose count was itself already one row stale. Deleting a benchmark stays allowed; deleting one silently does not. Also adds a `polar_coordinate_system` benchmark category, documents the module in §8 and `benchmarks/README.md`, and repoints the triangle-mesh cleanup assertion at the shared `TRACE_GPU_BUFFERS` list the buffer names moved into. --- CHANGELOG.md | 8 + benchmarks/README.md | 14 ++ benchmarks/categories.py | 9 ++ benchmarks/test_codspeed_polar.py | 228 ++++++++++++++++++++++++++++ spec/benchmarks/methodology.md | 40 ++++- spec/benchmarks/results.md | 1 + tests/test_benchmark_environment.py | 65 +++++++- 7 files changed, 359 insertions(+), 6 deletions(-) create mode 100644 benchmarks/test_codspeed_polar.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ce25fec..5568798e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,14 @@ in the README). `set/get_thetamin`, `set/get_thetamax`, and radial `set/get_rorigin`. Generic segment/mesh marks, polar rule/band annotations, LOD, facets/animation, and angular navigation/selection remain deferred. +- CodSpeed coverage for the polar coordinate system + (`benchmarks/test_codspeed_polar.py`, a new `polar_coordinate_system` benchmark + category): payload prep for a polar line, a wind rose and a pie, plus SVG, + native-PNG and polar-heatmap export. The polar increment previously moved no + benchmark at all, so the wedge-flattening cost and the polar payload path were + invisible to CI. The collected row count is now gated against + `spec/benchmarks/methodology.md` §8, so a renamed or deleted benchmark cannot + silently leave a stale row in the CodSpeed dashboard. ### Fixed - Repeated data updates no longer leak GPU buffers. Trace teardown walked a diff --git a/benchmarks/README.md b/benchmarks/README.md index 3b99e41d..091228ed 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -382,6 +382,20 @@ production split transport. Run `bench_animation.py` for real-Chrome previous+next scene bound; browser clocks and GPU work do not belong in CodSpeed simulation. +`test_codspeed_polar.py` attributes the polar coordinate system, which shipped +without a CodSpeed row of its own: three payload rows for the shapes with +materially different validation and emit paths (a 100k polar line, a 16-sector / +50k-observation wind rose, a 24-slice pie whose unequal widths take the four-edge +column path), and three export rows. SVG draws real `A` arcs and needs no +subdivision count, so it is the control for the native-PNG row, which flattens +every wedge at `config.polar_bar_segments(span, turn)` vertices — six segments +for a 22.5-degree sector rather than the full-turn 96, so a regression back to a +flat count appears as an arc-flattening step change. The last row is a polar +heatmap's bounded screen-space inverse raster, which has no Cartesian twin. +Browser wedge vertex counts, GPU buffer lifetime, and radial-zoom frame pacing +are wall-clock/WebGL measurements and stay in `bench_interaction.py` and the +polar smokes. + `test_codspeed_selection.py` covers the backend handlers the client's gesture messages resolve to: hover pick readout with a categorical channel, zone-pruned and full-scan box select at 1M points, and the cross-filter diff --git a/benchmarks/categories.py b/benchmarks/categories.py index 6db43d9f..186baa86 100644 --- a/benchmarks/categories.py +++ b/benchmarks/categories.py @@ -126,6 +126,15 @@ "status": "tracked", "goal": "Compute correct positive log domains from zone statistics with cost proportional to chunks, not points.", }, + { + "id": "polar_coordinate_system", + "name": "Polar coordinate system", + "why": "Radar, wind rose, pie/donut and gauge views are a whole chart family, and a polar wedge is the most expensive mark in the engine — one annular sector per bar instead of one quad.", + "metrics": "payload-prep time, wedge flattening cost, SVG/PNG export latency, inverse-raster latency", + "harness": "benchmarks/test_codspeed_polar.py", + "status": "tracked", + "goal": "Keep polar payload prep bounded by composition size rather than observation count, and keep wedge subdivision proportional to each wedge's own angular span.", + }, { "id": "static_export", "name": "Static export", diff --git a/benchmarks/test_codspeed_polar.py b/benchmarks/test_codspeed_polar.py new file mode 100644 index 00000000..30903e6e --- /dev/null +++ b/benchmarks/test_codspeed_polar.py @@ -0,0 +1,228 @@ +"""CodSpeed attribution for the polar coordinate system. + +The polar increment shipped a whole coordinate system — a new payload-build +validation pass, and the most expensive mark in the codebase — with no CodSpeed +row anywhere near it, so the report read "103 untouched benchmarks" for a change +that rewrote wedge geometry in three renderers. A performance cliff at ~50k +polar bars was found by hand, not by CI, precisely because nothing here could +see it. + +These rows isolate the *Python* cost, which is what simulation mode measures: + +- payload build for the three shapes with materially different validation and + emit paths (a plain polar line, a stacked wind rose, an unequal-width pie); +- static SVG and native-PNG export of wedges, where `polar_wedge_points` / + `_polar_wedge_path` flatten one arc per wedge. This is the row that tracks + the span-proportional subdivision in `config.polar_bar_segments`: a + 16-sector rose flattens six segments per wedge rather than the full-turn + worst case of 96, and a regression back to a flat count shows up here as + roughly a 10x arc-flattening increase rather than as a bug report; +- static export of a polar heatmap, whose bounded inverse raster resolves + screen pixels back through the transform and has no Cartesian twin. + +Browser-side wedge vertex counts, GPU buffer lifetime, and radial-zoom frame +pacing are wall-clock/WebGL measurements and stay out of simulation mode — +`benchmarks/bench_interaction.py` and the polar smokes cover those. +""" + +from __future__ import annotations + +import math + +import numpy as np +import pytest + +import xy +from xy import kernels as k + +#: Angular samples for the line row: enough to make the projection and the +#: per-vertex cull the dominant term rather than figure setup. +LINE_N = 100_000 + +#: Sector count for the rose. Sixteen compass points is the dense end of what +#: real wind roses use, and it is the case the subdivision formula was sized +#: against (22.5 degrees per wedge). +ROSE_SECTORS = 16 +ROSE_OBSERVATIONS = 50_000 + +#: Pie slices. Unequal widths ship four edge columns rather than one scalar +#: width, which is a different emit path and a different flattening call. +PIE_SLICES = 24 + +#: Polar heatmap grid. Cell count, not point count, drives the inverse raster. +HEATMAP_THETA = 96 +HEATMAP_R = 48 + +N_BUCKETS = 2048 + + +@pytest.fixture(scope="session", autouse=True) +def require_native_backend() -> None: + assert k.BACKEND == "native", ( + "CodSpeed benchmarks must run against the native Rust backend; " + f"got {k.BACKEND!r}. Build the native core before running them." + ) + + +@pytest.fixture(scope="session", autouse=True) +def warm_lazy_modules() -> None: + """Warm the polar build and export stacks before any measured region. + + Same phantom-regression guard the other modules carry: without it the first + row pays lazy submodule import for the payload and export stacks and tracks + package source size instead of its own workload. Warmed through the polar + paths specifically, so `_validate_coords`, the wedge emitters and the + projection are all resident. + """ + theta = np.array([0.0, 90.0, 180.0, 270.0]) + radius = np.array([1.0, 2.0, 3.0, 2.0]) + figure = xy.polar_chart( + xy.line(theta, radius), + xy.theta_axis(unit="degrees"), + width=240, + height=240, + ).figure() + figure.build_payload_split(N_BUCKETS) + figure.to_svg(width=240, height=240) + xy.polar_bar_chart( + xy.bar(theta, radius, width=22.5), + xy.theta_axis(unit="degrees"), + width=240, + height=240, + ).figure().to_png(engine=xy.Engine.default, scale=1.0) + + +@pytest.fixture(scope="module") +def polar_data() -> dict[str, object]: + rng = np.random.default_rng(19) + theta = np.linspace(0.0, 360.0, LINE_N, dtype=np.float64) + # A five-lobe rose: the radius varies over the whole turn, so no vertex run + # is culled wholesale and the projection runs on every point. + radius = (1.0 + 0.5 * np.sin(np.radians(5.0 * theta))).astype(np.float64, copy=False) + return { + "theta": theta, + "radius": radius, + "directions": rng.uniform(0.0, 360.0, ROSE_OBSERVATIONS), + "speeds": rng.gamma(2.0, 3.0, ROSE_OBSERVATIONS), + "pie_labels": [f"S{index:02d}" for index in range(PIE_SLICES)], + "pie_values": (10.0 + 6.0 * np.cos(np.linspace(0.0, 9.0, PIE_SLICES))).astype( + np.float64, copy=False + ), + "grid_theta": np.linspace(0.0, 2.0 * math.pi, HEATMAP_THETA, dtype=np.float64), + "grid_r": np.linspace(0.5, 4.0, HEATMAP_R, dtype=np.float64), + } + + +def _polar_line_payload(theta: np.ndarray, radius: np.ndarray) -> int: + figure = xy.polar_chart( + xy.line(theta, radius), + xy.theta_axis(unit="degrees"), + ).figure() + _spec, buffers = figure.build_payload_split(N_BUCKETS) + return sum(b.nbytes for b in buffers) + + +def _wind_rose_payload(directions: np.ndarray, speeds: np.ndarray) -> int: + figure = xy.wind_rose(directions, speeds, sectors=ROSE_SECTORS).figure() + _spec, buffers = figure.build_payload_split(N_BUCKETS) + return sum(b.nbytes for b in buffers) + + +def _pie_payload(labels: list[str], values: np.ndarray) -> int: + figure = xy.pie_chart(labels, values).figure() + _spec, buffers = figure.build_payload_split(N_BUCKETS) + return sum(b.nbytes for b in buffers) + + +def test_first_payload_polar_line(benchmark, polar_data): + """Polar payload prep: coordinate validation plus the angular axis contract. + + A polar figure carries the same raw f32 geometry a cartesian one does — the + projection happens in the renderer — so the payload must stay bounded by the + f32 encoding of two columns, never grow a pre-projected third. + """ + theta = polar_data["theta"] + radius = polar_data["radius"] + assert isinstance(theta, np.ndarray) + assert isinstance(radius, np.ndarray) + payload_bytes = benchmark(_polar_line_payload, theta, radius) + assert 0 < payload_bytes <= (theta.nbytes + radius.nbytes) // 2 + + +def test_first_payload_wind_rose(benchmark, polar_data): + """Wind rose payload prep: Python-side binning plus stacked wedge columns. + + Binning happens in Python, exactly as `hist` does it, so the shipped bytes + must be bounded by sector count and band count — never by observation count. + That bound is the whole reason a rose over 50k observations is cheap. + """ + directions = polar_data["directions"] + speeds = polar_data["speeds"] + assert isinstance(directions, np.ndarray) + payload_bytes = benchmark(_wind_rose_payload, directions, speeds) + assert 0 < payload_bytes < directions.nbytes // 8 + + +def test_first_payload_pie(benchmark, polar_data): + """Pie payload prep: one wedge bar per slice, each with its own width. + + Unequal widths take the four-edge column path rather than the compact + scalar-width one, so this row tracks per-slice emit cost — the thing that + grows when a composition gains a slice, not when it gains a data point. + """ + labels = polar_data["pie_labels"] + values = polar_data["pie_values"] + assert isinstance(labels, list) + payload_bytes = benchmark(_pie_payload, labels, values) + assert payload_bytes > 0 + + +def test_svg_export_polar_wedges(benchmark, polar_data): + """Static SVG export of a dense rose: one real `A` arc pair per wedge. + + SVG needs no flattening count, so this row is the arc-emission and chrome + cost with the subdivision term removed — the control for the PNG row below. + """ + directions = polar_data["directions"] + speeds = polar_data["speeds"] + figure = xy.wind_rose(directions, speeds, sectors=ROSE_SECTORS).figure() + document = benchmark(figure.to_svg, width=720, height=720) + assert document.startswith(" None: def test_triangle_mesh_resource_cleanup_deletes_every_coordinate_buffer() -> None: + # The buffer names moved out of `_destroyTraceResources` into the shared + # `TRACE_GPU_BUFFERS` list (js/src/00_header.ts) that all three teardown paths + # read, so assert against the list and that the teardown really uses it. + # `tests/test_trace_buffer_lifecycle.py` pins the list against every buffer + # any build path creates; this row keeps the triangle-mesh six explicit. + header = (ROOT / "js" / "src" / "00_header.ts").read_text(encoding="utf-8") + listed = header.split("export const TRACE_GPU_BUFFERS = [", 1)[1].split("];", 1)[0] + for name in ("x0Buf", "x1Buf", "x2Buf", "y0Buf", "y1Buf", "y2Buf"): + assert f'"{name}"' in listed + client = (ROOT / "js" / "src" / "50_chartview.ts").read_text(encoding="utf-8") cleanup = client[client.index("_destroyTraceResources(g, texSeen)") :] cleanup = cleanup[: cleanup.index("_destroyGlResources()")] - for name in ("x0Buf", "x1Buf", "x2Buf", "y0Buf", "y1Buf", "y2Buf"): - assert f'"{name}"' in cleanup + assert "this._deleteBuffers(g, TRACE_GPU_BUFFERS);" in cleanup + + +def _codspeed_row_count() -> int: + """CodSpeed rows the workflow's glob collects, counting parametrized expansion. + + Parsed rather than imported: the benchmark modules assert a native backend at + import time, and this only needs their shape. + """ + total = 0 + for path in sorted((ROOT / "benchmarks").glob("test_codspeed_*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in tree.body: + if not isinstance(node, ast.FunctionDef) or not node.name.startswith("test_"): + continue + rows = 1 + for decorator in node.decorator_list: + if not isinstance(decorator, ast.Call) or len(decorator.args) != 2: + continue + target = decorator.func + name = ( + target.attr if isinstance(target, ast.Attribute) else getattr(target, "id", "") + ) + if name != "parametrize": + continue + argvalues = decorator.args[1] + assert isinstance(argvalues, (ast.List, ast.Tuple)), ( + f"{path.name}::{node.name} parametrizes with a non-literal argvalues; " + "the row count can no longer be counted statically" + ) + rows *= len(argvalues.elts) + total += rows + return total + + +def test_codspeed_row_count_matches_the_methodology_spec() -> None: + """A benchmark cannot be added or removed without saying so in the spec. + + A row that is renamed or deleted silently stays in CodSpeed's stored + baseline, where the dashboard keeps reporting it as "skipped, using the + baseline result" — indistinguishable from a flaky measurement rather than a + row that no longer exists. Deleting a benchmark is fine; deleting one without + updating §8 (and archiving the stale row in the dashboard) is not. + """ + methodology = (ROOT / "spec/benchmarks/methodology.md").read_text(encoding="utf-8") + declared = re.search(r"for \*\*(\d+) rows\*\* total", methodology) + assert declared is not None, "spec/benchmarks/methodology.md §8 no longer states a row count" + assert _codspeed_row_count() == int(declared.group(1)), ( + "CodSpeed row count drifted from spec/benchmarks/methodology.md §8. Update the " + "count and the module list there, and archive any deleted row in the CodSpeed " + "dashboard so it stops being reported as skipped." + ) def test_benchmark_categories_track_core_hardening_metrics() -> None: From ba36a7ae65524e07548af894e3b21818e2a9afba Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Thu, 30 Jul 2026 17:46:28 +0000 Subject: [PATCH 2/6] Fix the pie legend: no sideways scrollbar, no repeated percentage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects reported against the pie chart on #370, with one shared cause. **The legend scrolled horizontally.** The box is capped at `--xy-legend-max-width`, but its grid columns were `max-content` and so refused to shrink below their content: an over-wide row overflowed and `overflow:auto` answered with a sideways scrollbar, hiding the label it was meant to be showing. Columns are now `minmax(0, max-content)` and the box scrolls on the block axis only. Overflow on the inline axis ellipsizes per row instead, which is what the static exporters already do — and the full text stays reachable in `title`/ARIA, the same rule categorical tick labels use. Only the label clips: the swatch is `flex:none` and keeps `overflow:visible`, so an authored oversized marker still draws outside its 18x14 box. **The legend repeated the percentage.** `pie_chart` printed the value and the share, and for values that already sum to 100 — how most pie data arrives — those are the same digits: `[40, 30, 20, 10]` rendered `Direct 40 (40%)`. The share keeps the unit, so the bare value is dropped when it says nothing new. Decided once for the whole pie rather than per slice, because a legend where one row carries a bare value and the next does not is harder to read than either consistent shape; zero slices draw no wedge and get no row, so they cannot veto it. The doubled label was also what overflowed the box, so the two reports share a root cause. The polar legend gutter added earlier in this branch was a flat 96 px, which ellipsized `Partner (30%)` — an ordinary slice's default name — while being a fifth of a phone canvas and a fifteenth of a wide one. It is now 22% of the canvas width clamped to 120-200 px: still derived from the canvas rather than measured from the label set, so all three renderers reserve the identical box instead of drifting with their font metrics, and `floor` rather than `round` so Python and JavaScript land on the same integer pixel. `legend_item` and `legend_label` join the `:where()` chrome layer, so an author's utility class still wins, and `test_static_client_security.py` now requires both rules to stay defeatable. --- CHANGELOG.md | 19 +++++++++- js/src/20_theme.ts | 4 +- js/src/50_chartview.ts | 56 +++++++++++++++++++++++----- python/xy/_svg.py | 47 ++++++++++++++++------- python/xy/components.py | 18 ++++++++- spec/design/polar-axes.md | 12 +++--- tests/test_polar_audit_fixes.py | 36 +++++++++++++++++- tests/test_polar_charts.py | 48 +++++++++++++++++++++++- tests/test_static_client_security.py | 5 +++ 9 files changed, 208 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5568798e..5b55a73e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,8 +44,11 @@ in the README). - A polar figure with a legend reserves a gutter for it and places it there, instead of overlaying the disc. A default `upper right` legend covered a wind rose's north-east sectors and its outer radial tick label; a disc inscribed in - its rect has no free corner to overlay. Compact widths take a band beneath the - disc instead. An authored `anchor` or four-tuple `padding` still wins. + its rect has no free corner to overlay. The gutter is 22% of the canvas width, + clamped to 120-200 px — derived from the canvas so all three renderers reserve + the identical box, and wide enough to hold an ordinary row rather than + ellipsize it. Compact widths take a 64 px band beneath the disc instead. An + authored `anchor` or four-tuple `padding` still wins. - Compact vertical colorbars keep their two extreme tick labels and their title. Collapsing them hid every number and the scale name, leaving an unlabelled gradient; only the interior tick ladder and the text-free minor ticks drop now. @@ -67,6 +70,18 @@ in the README). and painting twice. - `xy.pie_chart` appears in the generated chart-factory API reference alongside the other polar compositions. +- A legend row too wide for its box ellipsizes instead of growing a horizontal + scrollbar. The box is capped at `--xy-legend-max-width`, but its grid columns + were `max-content` and refused to shrink, so an over-wide row overflowed and + `overflow:auto` answered sideways — hiding the label it was meant to show. + Vertical scrolling is unchanged; the full text stays available in + `title`/ARIA, matching how categorical tick labels already ellipsize. +- `xy.pie_chart` no longer prints the same number twice. Values that already sum + to 100 — how most pie data arrives — made `show_values` and `show_percent` + collide, so `[40, 30, 20, 10]` rendered `Direct 40 (40%)`: a legend row that + reads as repeated text, and long enough to overflow the box. The share keeps + the unit and the bare value is dropped, decided once for the whole pie so rows + stay uniform. ### Changed - Polar wedge subdivision is span-proportional: `segments(span) = diff --git a/js/src/20_theme.ts b/js/src/20_theme.ts index 5d7b523b..9bfa9036 100644 --- a/js/src/20_theme.ts +++ b/js/src/20_theme.ts @@ -127,7 +127,9 @@ export const XY_CHROME_CSS = ` :where(.xy [data-xy-slot="tooltip_label"])::after{content:": "} :where(.xy [data-xy-slot="legend"]){left:var(--xy-legend-left,auto);right:var(--xy-legend-right,auto);top:var(--xy-legend-top,auto);bottom:var(--xy-legend-bottom,auto);transform:var(--xy-legend-transform,none);max-width:var(--xy-legend-max-width);max-height:var(--xy-legend-max-height);gap:2px;font-size:11px;background:var(--chart-legend-bg,rgba(128,128,128,.08));border-radius:4px;padding:4px 8px;color:var(--chart-text,inherit)} :where(.xy [data-xy-slot="legend_title"]){font-weight:400;text-align:center} -:where(.xy [data-xy-slot="legend_swatch"]){display:inline-block;width:var(--xy-legend-swatch-width,12px);height:var(--xy-legend-swatch-height,10px);vertical-align:-1px;background:var(--xy-legend-swatch-paint,transparent);border-radius:2px;margin-right:var(--xy-legend-swatch-margin-right,5px);fill:var(--xy-legend-swatch-fill);stroke:var(--xy-legend-swatch-stroke);stroke-width:var(--xy-legend-swatch-stroke-width);stroke-dasharray:var(--xy-legend-swatch-dasharray)} +:where(.xy [data-xy-slot="legend_item"]){display:flex;align-items:center;min-width:0} +:where(.xy [data-xy-slot="legend_label"]){min-width:0;overflow:hidden;text-overflow:ellipsis;white-space:nowrap} +:where(.xy [data-xy-slot="legend_swatch"]){display:inline-block;flex:none;width:var(--xy-legend-swatch-width,12px);height:var(--xy-legend-swatch-height,10px);vertical-align:-1px;background:var(--xy-legend-swatch-paint,transparent);border-radius:2px;margin-right:var(--xy-legend-swatch-margin-right,5px);fill:var(--xy-legend-swatch-fill);stroke:var(--xy-legend-swatch-stroke);stroke-width:var(--xy-legend-swatch-stroke-width);stroke-dasharray:var(--xy-legend-swatch-dasharray)} :where(.xy [data-xy-slot="legend_swatch"] > svg){display:block;width:100%;height:100%} :where(.xy [data-xy-slot="colorbar"]){color:var(--chart-text,inherit);font-size:10px} :where(.xy [data-xy-slot="colorbar_bar"]){background:var(--xy-colorbar-gradient);border:1px solid currentColor;box-sizing:border-box} diff --git a/js/src/50_chartview.ts b/js/src/50_chartview.ts index 5841c5e9..845b800c 100644 --- a/js/src/50_chartview.ts +++ b/js/src/50_chartview.ts @@ -67,13 +67,28 @@ const THETA_ZERO = { E: 0, N: Math.PI / 2, W: Math.PI, S: -Math.PI / 2 }; // plot because data rarely reaches a corner; a disc inscribed in its rect leaves // no corner at all, so an inside legend lands on the marks — an `upper right` box // covered a wind rose's whole north-east quadrant and the outer radial label -// under it. Mirrored by _POLAR_LEGEND_ROOM / _POLAR_LEGEND_BAND in -// python/xy/_svg.py; fixed rather than measured so all three renderers reserve -// the same box regardless of their font metrics. -const POLAR_LEGEND_ROOM = 96; -// Compact widths take a band under the disc instead: a 96 px side gutter out of a -// 380 px phone canvas leaves a disc too small to read, while vertical room is the -// one thing a phone viewport has. +// under it. +// +// A FRACTION OF THE CANVAS, clamped, rather than a measurement of the label set: +// every renderer knows the canvas width to the pixel, so all three reserve the +// identical box, while a measured reservation would drift with each renderer's +// font metrics (system-ui here, DejaVu in the exporters). A flat constant was +// tried first and is the wrong shape — 96 px ellipsized `Partner (30%)`, an +// ordinary pie slice's default name, while being a fifth of a phone canvas and a +// fifteenth of a wide one. A label still wider than the gutter ellipsizes with +// its full text in `title`/ARIA. +// Mirrored by `_polar_legend_room` in python/xy/_svg.py. +const POLAR_LEGEND_ROOM_FRACTION = 0.22; +const POLAR_LEGEND_ROOM_MIN = 120; +const POLAR_LEGEND_ROOM_MAX = 200; + +// `Math.floor`, not `Math.round`: Python and JavaScript disagree about half-way +// cases, and the two must land on the same integer pixel. +function xyPolarLegendRoom(width) { + const scaled = Math.floor(Number(width) * POLAR_LEGEND_ROOM_FRACTION); + return Math.min(POLAR_LEGEND_ROOM_MAX, Math.max(POLAR_LEGEND_ROOM_MIN, scaled)); +} + const POLAR_LEGEND_BAND = 64; // DejaVu Sans advances at 16 px, generated beside python/xy/_fontmetrics.py // and the native rasterizer. Layout must retain proportional glyph metrics: @@ -761,7 +776,10 @@ export class ChartView { if (!hasRows) return null; if (compact) return { side: "bottom", room: POLAR_LEGEND_BAND }; const loc = String(options.loc || "upper right"); - return { side: loc.includes("left") ? "left" : "right", room: POLAR_LEGEND_ROOM }; + return { + side: loc.includes("left") ? "left" : "right", + room: xyPolarLegendRoom(this.size.w), + }; } // Re-cut the plot rect for a disc. Mirrors `_recut_polar_plot` in @@ -2582,9 +2600,16 @@ export class ChartView { const handleTextPad = Number.isFinite(Number(options.handletextpad)) ? Math.max(0, Number(options.handletextpad)) : 0.8; + // `minmax(0, max-content)`, not bare `max-content`: the box is capped at + // `--xy-legend-max-width`, and a column that refuses to shrink below its + // content made a long row overflow horizontally — a legend with a horizontal + // SCROLLBAR, which hides the label it is meant to be showing. Vertical + // overflow still scrolls (that is the browser legend's advantage over the + // static exporters, which can only ellipsize); horizontal overflow + // ellipsizes per row instead, with the full text in `title`/ARIA. lg.style.cssText = "position:absolute;" + - `display:grid;grid-template-columns:repeat(${horizontal ? ncols : 1},max-content);` + - "column-gap:2em;row-gap:.5em;overflow:auto;"; + `display:grid;grid-template-columns:repeat(${horizontal ? ncols : 1},minmax(0,max-content));` + + "column-gap:2em;row-gap:.5em;overflow-x:hidden;overflow-y:auto;"; lg.dataset.xyLegendLoc = loc; if (Array.isArray(options.anchor)) { lg.dataset.xyLegendAnchor = JSON.stringify(options.anchor); @@ -2732,6 +2757,17 @@ export class ChartView { label.textContent = it.name; this._applySlot(label, "legend_label"); row.appendChild(label); + // A row too wide for the capped box ellipsizes (the `legend_item` / + // `legend_label` rules in 20_theme.ts) rather than pushing a horizontal + // scrollbar onto the legend. Only the LABEL clips: the swatch is + // `flex:none` and keeps `overflow:visible`, so an authored oversized + // marker still draws outside its 18x14 box. Same full-text-in-title/ARIA + // rule categorical tick labels use, so nothing an ellipsis hides becomes + // unreachable. + if (it.name) { + row.title = String(it.name); + row.setAttribute("aria-label", String(it.name)); + } // Hover emphasis (interaction spec §9): rows backed by live traces dim the rest // of the chart while hovered. Manually-added Legend artists carry no // trace linkage, so extra_legends rows stay inert. diff --git a/python/xy/_svg.py b/python/xy/_svg.py index 7225b259..5bd17d5a 100644 --- a/python/xy/_svg.py +++ b/python/xy/_svg.py @@ -2658,21 +2658,40 @@ def layout(spec: dict[str, Any]) -> tuple[int, int, bool, dict[str, float]]: # under it. Both incumbents' answer is to move it out (Plotly puts polar legends # in the figure margin), which needs room the disc gives back. # -# Fixed rather than measured from the label set, for the same reason -# POLAR_BAR_SEGMENTS is fixed: the reservation has to be identical in all three -# renderers, and a measured one would drift with each renderer's font metrics. -# The legend still sizes and ellipsizes to the gutter, so a long label is bounded -# rather than clipped. -# Mirrored by POLAR_LEGEND_ROOM / POLAR_LEGEND_BAND in js/src/50_chartview.ts. -_POLAR_LEGEND_ROOM = 96.0 -# Compact widths take a band under the disc instead: a 96 px side gutter out of a -# 380 px phone canvas leaves a disc too small to read, while vertical room is the -# one thing a phone viewport has. +# A FRACTION OF THE CANVAS, clamped, rather than a measurement of the label set: +# every renderer knows the canvas width to the pixel, so all three reserve the +# identical box, while a measured reservation would drift with each renderer's +# font metrics (DejaVu here, system-ui in the browser). A flat constant was tried +# first and is the wrong shape — 96 px ellipsized `Partner (30%)`, an ordinary +# pie slice's default name, while being a fifth of a phone canvas and a +# fifteenth of a wide one. +# +# The floor keeps a narrow chart's legend readable; the ceiling stops a wide one +# from spending 300 px on four short rows. A label still wider than the gutter +# ellipsizes with its full text in `title`/ARIA, exactly as the static exporters +# already ellipsize against the plot width. +# Mirrored by xyPolarLegendRoom in js/src/50_chartview.ts. +_POLAR_LEGEND_ROOM_FRACTION = 0.22 +_POLAR_LEGEND_ROOM_MIN = 120.0 +_POLAR_LEGEND_ROOM_MAX = 200.0 + + +def _polar_legend_room(width: float) -> float: + """Side-gutter width for a polar legend on a `width`-px canvas. + + `floor`, not `round`: Python and JavaScript disagree about half-way cases + (banker's rounding versus round-half-up) and the two must land on the same + integer pixel. + """ + scaled = math.floor(float(width) * _POLAR_LEGEND_ROOM_FRACTION) + return min(_POLAR_LEGEND_ROOM_MAX, max(_POLAR_LEGEND_ROOM_MIN, float(scaled))) + + _POLAR_LEGEND_BAND = 64.0 -def _polar_legend_reserve(spec: dict[str, Any], compact: bool) -> tuple[str, float]: - """Side and px a polar legend gutter claims: ``("right", 96.0)`` etc. +def _polar_legend_reserve(spec: dict[str, Any], compact: bool, width: float) -> tuple[str, float]: + """Side and px a polar legend gutter claims: ``("right", 158.0)`` etc. ``("", 0.0)`` when nothing is reserved — a non-polar figure, no legend rows, an authored ``anchor`` (an explicit plot-relative placement the author owns), @@ -2696,7 +2715,7 @@ def _polar_legend_reserve(spec: dict[str, Any], compact: bool) -> tuple[str, flo if compact: return "bottom", _POLAR_LEGEND_BAND loc = str(options.get("loc") or "upper right") - return ("left" if "left" in loc else "right"), _POLAR_LEGEND_ROOM + return ("left" if "left" in loc else "right"), _polar_legend_room(width) def _polar_label_room(theta_axis: dict[str, Any]) -> float: @@ -2761,7 +2780,7 @@ def _recut_polar_plot( # legend never occupies the disc. Recorded as four floats rather than a # nested rect so `plot` stays a flat float map. canvas_x0 = 0.0 - legend_side, legend_room = _polar_legend_reserve(spec, compact) + legend_side, legend_room = _polar_legend_reserve(spec, compact, width) if legend_room: if legend_side == "left": box = (0.0, plot["y"], legend_room, plot["h"]) diff --git a/python/xy/components.py b/python/xy/components.py index a1037490..6d06949d 100644 --- a/python/xy/components.py +++ b/python/xy/components.py @@ -6436,6 +6436,8 @@ def pie_chart( colors: One CSS colour per slice. Defaults to the palette cycle. corner_radius: Rounded slice corners, in px. show_values: Include the value in the slice's name (legend + tooltip). + Dropped for a slice whose value renders identically to its share — + percentage-shaped input would otherwise print the same number twice. show_percent: Include the share in the slice's name (legend + tooltip). **props: Any `polar_chart` keyword (`width`, `height`, `title`, …). """ @@ -6460,6 +6462,20 @@ def pie_chart( f"pie_chart colors must have one entry per slice ({len(names)}); got {len(colors)}" ) + # Never print the same number twice. Percentage-shaped input — values that + # already sum to 100, which is how most pie data arrives — made the two + # defaults collide: `[40, 30, 20, 10]` rendered "Direct 40 (40%)", so the + # legend read as repeated text and the doubled label was what overflowed the + # box. The share keeps the unit, so it is the one that survives. + # + # Decided once for the whole pie rather than per slice: a mixed legend, where + # one row carries a bare value and the next does not, is harder to read than + # either consistent choice. Zero slices are excluded because they draw no + # wedge and get no row. + values_are_shares = show_percent and all( + f"{value:g}" == f"{value / total * 100:.0f}" for value in amounts if value > 0.0 + ) + slices: list[Component] = [] cursor = 0.0 for index, (label, value) in enumerate(zip(names, amounts, strict=True)): @@ -6473,7 +6489,7 @@ def pie_chart( if span <= 0.0: continue display = label - if show_values: + if show_values and not values_are_shares: display += f" {value:g}" if show_percent: display += f" ({value / total * 100:.0f}%)" diff --git a/spec/design/polar-axes.md b/spec/design/polar-axes.md index 593f1071..8803cf31 100644 --- a/spec/design/polar-axes.md +++ b/spec/design/polar-axes.md @@ -130,12 +130,14 @@ Five properties this pins down, each of which has matching coverage: under it. `_polar_legend_reserve` (`_svg.py`, mirrored by `_polarLegendReserve`) therefore takes a gutter off the canvas edge **before** the disc is fitted, and records it as `plot["legend_box_*"]` / `view._legendBox`; the legend places and - bounds itself in that box, and `loc` chooses where within it. `_POLAR_LEGEND_ROOM` - (96 px) on the side `loc` names, or `_POLAR_LEGEND_BAND` (64 px) beneath the + bounds itself in that box, and `loc` chooses where within it. `_polar_legend_room` + (22% of the canvas width, clamped to 120–200 px) on the side `loc` names, or `_POLAR_LEGEND_BAND` (64 px) beneath the disc at compact widths, where a side gutter would leave a disc too small to - read. Fixed rather than measured from the label set, for the same reason the - subdivision count is a shared formula: a measured reservation would drift with - each renderer's font metrics. Nothing is reserved when the author supplied an + read. Derived from the canvas width rather than measured from the label set, for the + same reason the subdivision count is a shared formula: every renderer knows the + canvas to the pixel, while a measured reservation would drift with each + renderer's font metrics. A label wider than the gutter ellipsizes with its full + text in `title`/ARIA, as the static exporters already do against the plot width. Nothing is reserved when the author supplied an `anchor` (an explicit plot-relative placement they own, still resolved against the plot) or a four-tuple `padding` (which already states the box the plot should occupy, and remains the way to hand-reserve a caption band), and nothing diff --git a/tests/test_polar_audit_fixes.py b/tests/test_polar_audit_fixes.py index 9c075d0b..769b7dc5 100644 --- a/tests/test_polar_audit_fixes.py +++ b/tests/test_polar_audit_fixes.py @@ -369,6 +369,36 @@ def test_client_caps_the_title_box_at_the_measured_wrap_width() -> None: assert "function xyWrapLines(lines, advance, maxWidth)" in CHARTVIEW +# -- legend overflow -------------------------------------------------------- + + +def test_a_long_legend_row_ellipsizes_instead_of_scrolling_sideways() -> None: + """A pie legend grew a horizontal scrollbar, hiding the label it was showing. + + The box is capped at `--xy-legend-max-width`, but its grid columns were + `max-content` — they refused to shrink — so an over-wide row overflowed and + `overflow:auto` answered with a sideways scrollbar. Vertical scrolling stays + (it is what the browser legend has over the static exporters, which can only + ellipsize); horizontal overflow now ellipsizes per row. + """ + theme = (ROOT / "js/src/20_theme.ts").read_text(encoding="utf-8") + # Columns that can shrink, and no horizontal scroll axis. + assert "minmax(0,max-content)" in CHARTVIEW + assert "overflow-x:hidden;overflow-y:auto;" in CHARTVIEW + # Only the LABEL clips. The swatch is `flex:none` and keeps overflow visible, + # so an authored oversized marker still draws outside its 18x14 box. + assert 'data-xy-slot="legend_item"]){display:flex;align-items:center;min-width:0}' in theme + assert ( + 'data-xy-slot="legend_label"]){min-width:0;overflow:hidden;' + "text-overflow:ellipsis;white-space:nowrap}" in theme + ) + assert 'data-xy-slot="legend_swatch"]){display:inline-block;flex:none;' in theme + # An ellipsis must never make text unreachable: same full-text-in-title/ARIA + # rule the categorical tick labels use. + assert "row.title = String(it.name);" in CHARTVIEW + assert 'row.setAttribute("aria-label", String(it.name));' in CHARTVIEW + + # -- polar legend gutter ---------------------------------------------------- @@ -380,7 +410,7 @@ def test_a_polar_legend_gets_a_gutter_beside_the_disc() -> None: # The box is outside the plot rect, on the right, and the disc no longer # reaches into it. assert plot["legend_box_x"] >= plot["x"] + plot["w"] - assert plot["legend_box_w"] == pytest.approx(_svg._POLAR_LEGEND_ROOM) + assert plot["legend_box_w"] == pytest.approx(_svg._polar_legend_room(720)) def test_a_compact_polar_legend_takes_a_band_under_the_disc() -> None: @@ -437,7 +467,9 @@ def test_a_cartesian_legend_still_overlays_its_plot() -> None: def test_client_legend_places_in_the_reserved_box() -> None: assert "_polarLegendReserve(compact)" in CHARTVIEW - assert "const POLAR_LEGEND_ROOM = 96;" in CHARTVIEW + assert "function xyPolarLegendRoom(width)" in CHARTVIEW + assert "const POLAR_LEGEND_ROOM_FRACTION = 0.22;" in CHARTVIEW + assert "room: xyPolarLegendRoom(this.size.w)," in CHARTVIEW assert "const POLAR_LEGEND_BAND = 64;" in CHARTVIEW # Placement and the responsive max-width both read the legend box, and an # authored anchor still resolves against the plot. diff --git a/tests/test_polar_charts.py b/tests/test_polar_charts.py index 4ef77ce5..40ebc099 100644 --- a/tests/test_polar_charts.py +++ b/tests/test_polar_charts.py @@ -1176,10 +1176,12 @@ def spy_grad(self, pts, g0, g1, stops): def test_pie_chart_slices_carry_category_value_and_share() -> None: - chart = xy.pie_chart(["a", "b", "c"], [50.0, 30.0, 20.0], width=300, height=300) + # Counts, not shares: the value and the percentage are different numbers, so + # both earn their place in the row. + chart = xy.pie_chart(["a", "b", "c"], [27.0, 21.0, 13.0], width=300, height=300) spec, _ = chart.figure().build_payload() names = [t["name"] for t in spec["traces"]] - assert names == ["a 50 (50%)", "b 30 (30%)", "c 20 (20%)"] + assert names == ["a 27 (44%)", "b 21 (34%)", "c 13 (21%)"] # The composition owns its readout: the tooltip is the slice name alone, # never theta (layout) or the constant rim radius. assert spec["tooltip"] == {"title": "{name}"} @@ -1189,6 +1191,48 @@ def test_pie_chart_slices_carry_category_value_and_share() -> None: assert sum(widths) == pytest.approx(360.0, abs=1e-6) +def test_pie_chart_never_prints_the_same_number_twice() -> None: + """Percentage-shaped values made both defaults render the same digits. + + `[40, 30, 20, 10]` is how most pie data arrives, and it came out as + "Direct 40 (40%)" — a legend row that reads as repeated text, and long + enough to overflow the legend box that then grew a horizontal scrollbar. + """ + chart = xy.pie_chart( + ["Direct", "Partner", "Organic", "Other"], + [40.0, 30.0, 20.0, 10.0], + width=300, + height=300, + ) + spec, _ = chart.figure().build_payload() + names = [t["name"] for t in spec["traces"]] + assert names == ["Direct (40%)", "Partner (30%)", "Organic (20%)", "Other (10%)"] + + # The choice is made once for the whole pie, not per slice: a legend where + # one row carries a bare value and the next does not is worse than either + # consistent shape. 10.5 does not render as its 10% share, so every row keeps + # its value even though the other three would have collided. + mixed = xy.pie_chart(["a", "b", "c", "d"], [40.0, 30.0, 20.0, 10.5], width=300, height=300) + mixed_spec, _ = mixed.figure().build_payload() + assert [t["name"] for t in mixed_spec["traces"]] == [ + "a 40 (40%)", + "b 30 (30%)", + "c 20 (20%)", + "d 10.5 (10%)", + ] + + # A zero slice draws no wedge and gets no row, so it cannot veto the choice. + zeroed = xy.pie_chart(["a", "b", "c", "d"], [40.0, 30.0, 30.0, 0.0], width=300, height=300) + zero_spec, _ = zeroed.figure().build_payload() + assert [t["name"] for t in zero_spec["traces"]] == ["a (40%)", "b (30%)", "c (30%)"] + + # Either switch alone is untouched: with no share to collide with, the value + # is always shown. + values_only = xy.pie_chart(["a", "b"], [40.0, 60.0], show_percent=False, width=300, height=300) + values_spec, _ = values_only.figure().build_payload() + assert [t["name"] for t in values_spec["traces"]] == ["a 40", "b 60"] + + def test_pie_chart_user_tooltip_wins() -> None: chart = xy.pie_chart(["a", "b"], [1.0, 1.0], xy.tooltip(title="custom"), width=300, height=300) spec, _ = chart.figure().build_payload() diff --git a/tests/test_static_client_security.py b/tests/test_static_client_security.py index 4676435e..28aa8973 100644 --- a/tests/test_static_client_security.py +++ b/tests/test_static_client_security.py @@ -48,6 +48,11 @@ def _read(path: Path) -> str: ':where(.xy [data-xy-slot="tooltip_label"])::after{', ':where(.xy [data-xy-slot="legend"]){', ':where(.xy [data-xy-slot="legend_title"]){', + # The row is a flex line and the label ellipsizes, so a long entry cannot + # push a horizontal scrollbar onto the legend box. Both stay in the `:where()` + # layer so an author's utility class still wins. + ':where(.xy [data-xy-slot="legend_item"]){', + ':where(.xy [data-xy-slot="legend_label"]){', ':where(.xy [data-xy-slot="legend_swatch"]){', ':where(.xy [data-xy-slot="modebar"]){', ':where(.xy) button[data-xy-slot="modebar_button"]{', From 175118a7488a5085a23cb680d17b09768fe2150c Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Thu, 30 Jul 2026 17:57:26 +0000 Subject: [PATCH 3/6] Fix the five CI failures the merged polar audit round introduced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `alek/polar-axes` went red at 7449360 (the squash of #380) after being green at a3e28bf, and all five failures are mine. **Two were tests pinning code I moved.** `test_tick_sides_bump_wire_protocol_and_client_in_lockstep` asserted the exact `00_header` import line, which grew a third name; it now checks that the client imports PROTOCOL rather than the spelling of the list. `test_triangle_mesh_resource_cleanup_deletes_every_coordinate_buffer` looked for the buffer names inside `_destroyTraceResources`, where they no longer live; it reads the shared `TRACE_GPU_BUFFERS` list and asserts the teardown uses it. (That second fix was already written, just not in the squash.) **Two were the polar legend gutter ellipsizing static labels.** At a flat 96 px the exporters' legend had 44 px of label room, so `series one` came out `series...` and `Organic 20 (33%)` came out `Orga...`. The gutter is now 22% of the canvas clamped to 120-200 px, which gives 68 px at the narrowest non-compact width and 146 px at the default export size — both failing labels fit. **One was the compact colorbar spending plot width.** Reserving a side gutter for the endpoint tick labels cost 36 px, and `test_narrow_fluid_resize_stays_painted_and_preserves_plot_space` guards exactly that (plot width >= 280, colorbar width 18). The endpoints now stack above and below the gradient instead of sitting beside it: centred on an 18 px bar they overflow ~4 px a side into the gap already reserved, so the reservation goes back to what it was and the fix is free. The rotated title joins the interior ladder in dropping — at phone width it has nowhere to go, and `box.title` already names the scale and its range. That test's "hide every tick and the title" assertion was the defect the audit reported, so it now states the new contract: the two extremes stay, everything else goes. Also reverts the DPR-change deferral. `render_smoke_nonumpy.py`'s `dprw` probe calls `_onDprChange()` and reads `dpr`/`canvas.width`/`chrome.width` on the very next line — a DPR change with no container resize has no later event to piggyback on — and routing it through `_queueResize` broke that contract. It also saved nothing: the ResizeObserver's queued pass already early-returns when width, height and dpr are all unchanged, and when the CSS size did change too, the second pass is doing real work at a new size. The dpr-baked width/radius rescale stays. --- CHANGELOG.md | 14 +-- js/src/50_chartview.ts | 106 ++++++++++++----------- tests/pyplot/test_tick_side_rendering.py | 5 +- tests/test_legend_resize_regression.py | 21 ++++- tests/test_polar_audit_fixes.py | 54 +++++++----- 5 files changed, 119 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b55a73e..b3ace714 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,9 +49,12 @@ in the README). the identical box, and wide enough to hold an ordinary row rather than ellipsize it. Compact widths take a 64 px band beneath the disc instead. An authored `anchor` or four-tuple `padding` still wins. -- Compact vertical colorbars keep their two extreme tick labels and their title. - Collapsing them hid every number and the scale name, leaving an unlabelled - gradient; only the interior tick ladder and the text-free minor ticks drop now. +- Compact vertical colorbars keep their two extreme tick labels, restacked above + and below the gradient. Collapsing them hid every number, leaving an unlabelled + gradient; only the interior ladder, the rotated title and the text-free minor + ticks drop now, and the box's own `title`/ARIA text still names the scale. + Stacking is what makes it free: a side gutter wide enough for `0.25` would cost + 36 px of the plot width the compact collapse exists to protect. - A time-valued radial axis autoranges from its data instead of from epoch zero, which had squeezed every modern instant into a hairline ring at the rim. An explicit `r_axis(margin=)` restores the outer pad it used to discard. @@ -66,8 +69,9 @@ in the README). when the transition ends. - A device-pixel-ratio change (browser zoom, or a window moving between displays) now rescales the per-instance stroke widths and corner radii that are baked in - device pixels, and coalesces into a single resize frame instead of laying out - and painting twice. + device pixels, so authored strokes and wedge corners keep their intended size + across a zoom. The DPR handler stays synchronous: a DPR change with no container + resize has no later event to piggyback on. - `xy.pie_chart` appears in the generated chart-factory API reference alongside the other polar compositions. - A legend row too wide for its box ellipsizes instead of growing a horizontal diff --git a/js/src/50_chartview.ts b/js/src/50_chartview.ts index 845b800c..065b6875 100644 --- a/js/src/50_chartview.ts +++ b/js/src/50_chartview.ts @@ -150,14 +150,17 @@ function xyTextAdvance(text, fontSize) { const COLORBAR_THICKNESS = 18; const COLORBAR_GAP = 24; const COMPACT_COLORBAR_GAP = 8; -// Room beside a compact vertical colorbar for its two endpoint tick labels, and -// for its rotated title. The compact form used to hide every tick and the title -// outright, which left an unlabelled gradient — a colour ramp with no numbers on -// it says nothing at all, so it is not a smaller version of the chrome, it is -// the absence of it. Two numbers and the scale name are what make the ramp -// readable; interior ticks are what a narrow chart can actually afford to drop. -const COMPACT_COLORBAR_TICK_ROOM = 30; -const COMPACT_COLORBAR_TITLE_ROOM = 14; +// A compact vertical colorbar keeps its two EXTREME tick labels, stacked above +// and below the gradient rather than beside it. Hiding every tick left an +// unlabelled gradient — a colour ramp with no numbers on it says nothing at all, +// so the compact form was not a smaller version of the chrome but the absence of +// it. Stacking is what makes the fix free: a side gutter wide enough for `0.25` +// cost 36 px of plot width, which is the very thing the compact collapse exists +// to protect, while two centred labels overflow the 18 px bar by ~4 px a side and +// fit inside the gap that is already reserved. Interior ticks and the rotated +// title are what a phone-width chart genuinely cannot afford; the title stays +// readable through the box's own `title`/ARIA text. +const COMPACT_COLORBAR_LABEL_GAP = 3; let XY_A11Y_ID = 0; // Legend hover emphasis (interaction spec §9): opacity kept by non-hovered series on // the marks canvas, and by non-hovered rows in the legend box itself. @@ -649,8 +652,7 @@ export class ChartView { ? axesColorbar ? 44 + (colorbar.label ? 18 : 0) : (this._compactVerticalColorbar - ? COMPACT_COLORBAR_GAP + COLORBAR_THICKNESS + COMPACT_COLORBAR_TICK_ROOM - + (colorbar.label ? COMPACT_COLORBAR_TITLE_ROOM : 0) + ? COMPACT_COLORBAR_GAP + COLORBAR_THICKNESS + 8 : 62 + automaticColorbarGap + (colorbar.label ? 18 : 0)) : 0; const colorbarBottomRoom = horizontalColorbar @@ -1717,14 +1719,15 @@ export class ChartView { const mq = window.matchMedia(`(resolution: ${window.devicePixelRatio}dppx)`); this._onDprChange = () => { if (this._destroyed) return; - // Queued, not immediate. Browser zoom changes devicePixelRatio *and* the - // container's CSS size, so this fired a synchronous full resize+paint and - // the ResizeObserver then queued a second one for the same gesture: two - // layouts and two frames for one zoom. `_queueResize` coalesces both into - // the single rAF that already serves resizes; it re-reads - // devicePixelRatio, and `measure` re-reads the container for fluid - // charts so the queued pass sees the post-zoom box. - this._queueResize(this.size.w, this.size.h, this.fluid || this.fluidH); + // Synchronous on purpose, and pinned that way: `render_smoke_nonumpy.py`'s + // `dprw` probe calls this and reads `dpr`/`canvas.width`/`chrome.width` on + // the very next line, because a DPR change with no container resize has no + // later event to piggyback on. Deferring it into `_queueResize` broke that + // contract, and the redundant second frame it was meant to save does not + // exist: the ResizeObserver's queued pass early-returns when width, height + // and dpr are all unchanged, and when the CSS size *did* change too, the + // second pass is doing real work at a new size. + this._resize(this.size.w, this.size.h); // re-reads devicePixelRatio this._armDprWatch(); }; mq.addEventListener?.("change", this._onDprChange, { once: true }); @@ -3435,6 +3438,9 @@ export class ChartView { tick.style.cssText = horizontal ? `position:absolute;left:${100 * fraction}%;top:${barThickness + 2}px;transform:translateX(-50%);white-space:nowrap;` : `position:absolute;left:${barThickness + 5}px;top:${100 * (1 - fraction)}%;transform:translateY(-50%);white-space:nowrap;`; + // The compact form restacks the two endpoints above/below the gradient, so + // keep the beside-the-bar placement to restore when the container widens. + tick._xyBesideCss = tick.style.cssText; this._applySlot(tick, "colorbar_tick"); box.appendChild(tick); } @@ -3462,15 +3468,9 @@ export class ChartView { if (cb.label) { const label = document.createElement("span"); label.textContent = String(cb.label); - // The vertical title sits outside the tick column. On a compact width that - // column holds two short endpoint labels instead of a full ladder, so the - // title moves in to match (`_positionColorbar` re-sets this on resize). - const titleGap = this._compactVerticalColorbar - ? COLORBAR_THICKNESS + COMPACT_COLORBAR_TICK_ROOM - : barThickness + 40; label.style.cssText = horizontal ? `position:absolute;left:50%;top:${barThickness + 18}px;transform:translateX(-50%);white-space:nowrap;` - : `position:absolute;left:${titleGap}px;top:50%;writing-mode:vertical-rl;transform:translateY(-50%) rotate(180deg);white-space:nowrap;`; + : `position:absolute;left:${barThickness + 40}px;top:50%;writing-mode:vertical-rl;transform:translateY(-50%) rotate(180deg);white-space:nowrap;`; this._applySlot(label, "colorbar_title"); box.appendChild(label); } @@ -3510,41 +3510,47 @@ export class ChartView { : this.plot.y + (this.plot.h - barHeight) * (1 - Number(anchor[1] ?? 0.5))) + "px"; this._colorbar.style.width = (horizontal ? axesPlacement ? this.plot.w : barWidth - : axesPlacement - ? this.plot.w + 44 - : compactVertical - ? COLORBAR_THICKNESS + COMPACT_COLORBAR_TICK_ROOM - + (cb.label ? COMPACT_COLORBAR_TITLE_ROOM : 0) - : 66) + "px"; + : axesPlacement ? this.plot.w + 44 : compactVertical ? COLORBAR_THICKNESS : 66) + "px"; this._colorbar.style.height = (horizontal ? axesPlacement ? this.plot.h + 24 : 50 : Math.max(24, barHeight)) + "px"; this._colorbar.dataset.xyCompact = compactVertical ? "true" : "false"; - // Compact keeps the two EXTREME ticks and the title; only the interior - // ladder is dropped. Hiding all of them left a bare gradient with no numbers - // and no scale name, which is unreadable rather than merely condensed. + // Compact keeps the two EXTREME tick labels — hiding all of them left a bare + // gradient with no numbers on it — and restacks them above and below the + // gradient. Beside the bar they would need a gutter wide enough for `0.25`, + // which costs 36 px of the plot width the compact collapse exists to protect; + // centred on an 18 px bar they overflow ~4 px a side into the gap already + // reserved. The interior ladder still drops, and so does the rotated title: + // at phone width it has nowhere to go, and the box's own `title`/ARIA text + // already names the scale and its range. const ticks = [...this._colorbar.querySelectorAll('[data-xy-slot="colorbar_tick"]')]; const fractions = ticks.map((node) => Number(node.dataset.xyColorbarFraction)); const lowest = Math.min(...fractions); const highest = Math.max(...fractions); - ticks.forEach((node, index) => { + for (const [index, node] of ticks.entries()) { const fraction = fractions[index]; - node.hidden = compactVertical - && Number.isFinite(fraction) - && fraction !== lowest - && fraction !== highest; - }); - for (const node of this._colorbar.querySelectorAll('[data-xy-slot="colorbar_title"]')) { - node.hidden = false; - if (!horizontal) { - node.style.left = (compactVertical - ? COLORBAR_THICKNESS + COMPACT_COLORBAR_TICK_ROOM - : (axesPlacement ? this.plot.w : COLORBAR_THICKNESS) + 40) + "px"; + const endpoint = !Number.isFinite(fraction) || fraction === lowest || fraction === highest; + node.hidden = compactVertical && !endpoint; + if (horizontal || !node._xyBesideCss) continue; + if (!compactVertical || !endpoint) { + node.style.cssText = node._xyBesideCss; + continue; } - } - // Minor ticks stay off in the compact form: they carry no text, so they add - // ink without adding a reading. - for (const node of this._colorbar.querySelectorAll("[data-xy-colorbar-minor]")) { + // Above the top of the gradient for the maximum, below the bottom for the + // minimum, both centred on the bar. + const above = fraction === highest; + const offset = COMPACT_COLORBAR_LABEL_GAP; + node.style.cssText = + "position:absolute;left:50%;white-space:nowrap;" + + (above + ? `top:-${offset}px;transform:translate(-50%,-100%);` + : `top:calc(100% + ${offset}px);transform:translateX(-50%);`); + } + // The rotated title and the text-free minor ticks are ink a phone-width chart + // cannot spend; `box.title` keeps the scale name reachable. + for (const node of this._colorbar.querySelectorAll( + '[data-xy-slot="colorbar_title"], [data-xy-colorbar-minor]' + )) { node.hidden = compactVertical; } } diff --git a/tests/pyplot/test_tick_side_rendering.py b/tests/pyplot/test_tick_side_rendering.py index c390c3d4..8df54519 100644 --- a/tests/pyplot/test_tick_side_rendering.py +++ b/tests/pyplot/test_tick_side_rendering.py @@ -122,7 +122,10 @@ def test_tick_sides_bump_wire_protocol_and_client_in_lockstep() -> None: assert spec["x_axis"]["tick_sides"] == ["bottom", "top"] assert spec["protocol"] == PROTOCOL_VERSION == 12 assert f"PROTOCOL = {PROTOCOL_VERSION};" in header - assert 'import { PROTOCOL, xyByteSpan } from "./00_header";' in client + # The point is that the client reads PROTOCOL from the header, not the exact + # spelling of the import list — which grows whenever the header gains another + # shared constant (it now also exports TRACE_GPU_BUFFERS). + assert "PROTOCOL" in client.split(' from "./00_header";', 1)[0] assert "spec.protocol !== PROTOCOL" in client diff --git a/tests/test_legend_resize_regression.py b/tests/test_legend_resize_regression.py index 6c404156..5963e14c 100644 --- a/tests/test_legend_resize_regression.py +++ b/tests/test_legend_resize_regression.py @@ -318,12 +318,19 @@ const compactNodes = [...view._colorbar.querySelectorAll( '[data-xy-slot="colorbar_tick"], [data-xy-slot="colorbar_title"]' )]; + const visibleTickText = (nodes) => nodes + .filter((node) => !node.hidden && node.dataset.xySlot === "colorbar_tick") + .map((node) => node.textContent); const compactState = { plotWidth: view.plot.w, colorbarWidth: view._colorbar.getBoundingClientRect().width, compact: view._colorbar.dataset.xyCompact, hiddenChrome: compactNodes.filter((node) => node.hidden).length, chromeCount: compactNodes.length, + visibleTicks: visibleTickText(compactNodes), + titleHidden: compactNodes + .filter((node) => node.dataset.xySlot === "colorbar_title") + .every((node) => node.hidden), }; view._resize(760, 500); @@ -555,9 +562,17 @@ def test_narrow_fluid_resize_stays_painted_and_preserves_plot_space() -> None: assert payload["compactState"]["plotWidth"] >= 280, payload assert payload["compactState"]["colorbarWidth"] == pytest.approx(18, abs=1), payload assert payload["compactState"]["compact"] == "true", payload - assert payload["compactState"]["hiddenChrome"] == payload["compactState"]["chromeCount"], ( - payload - ) + # The compact form keeps the two EXTREME tick labels, stacked above and below + # the gradient, and drops the interior ladder plus the rotated title. Hiding + # every one of them — the previous contract this line asserted — left a colour + # ramp with no numbers on it, which is unreadable rather than condensed. The + # width and plot-space assertions above are what keep the fix free: restacking + # the endpoints costs no side gutter, so the collapse still hands the plot the + # room it was collapsing for. + assert payload["compactState"]["visibleTicks"] == ["0", "1"], payload + assert payload["compactState"]["titleHidden"] is True, payload + hidden = payload["compactState"]["hiddenChrome"] + assert hidden == payload["compactState"]["chromeCount"] - 2, payload assert payload["restoredState"] == {"compact": "false", "hiddenChrome": 0}, payload diff --git a/tests/test_polar_audit_fixes.py b/tests/test_polar_audit_fixes.py index 769b7dc5..8ed116c9 100644 --- a/tests/test_polar_audit_fixes.py +++ b/tests/test_polar_audit_fixes.py @@ -480,25 +480,29 @@ def test_client_legend_places_in_the_reserved_box() -> None: # -- compact colorbar ------------------------------------------------------- -def test_compact_colorbars_keep_their_endpoints_and_title() -> None: - """Hiding every tick and the title left a gradient with no numbers on it.""" - # Interior ticks drop; the two extremes and the title do not. - assert "node.hidden = compactVertical\n && Number.isFinite(fraction)" in CHARTVIEW - assert "&& fraction !== lowest" in CHARTVIEW - assert "&& fraction !== highest;" in CHARTVIEW +def test_compact_colorbars_keep_their_endpoint_labels() -> None: + """Hiding every tick left a gradient with no numbers on it. + + The two extremes survive, restacked above and below the gradient. Beside the + bar they would need a gutter wide enough for `0.25`, which costs 36 px of the + plot width the compact collapse exists to protect; centred on the 18 px bar + they fit in the gap already reserved, so the fix is free. + """ assert ( - "for (const node of this._colorbar.querySelectorAll('[data-xy-slot=\"colorbar_title\"]')) {" - in CHARTVIEW + "const endpoint = !Number.isFinite(fraction) " + "|| fraction === lowest || fraction === highest;" in CHARTVIEW ) - assert "node.hidden = false;" in CHARTVIEW - # Text-free minor ticks stay hidden: ink without a reading. - assert 'querySelectorAll("[data-xy-colorbar-minor]")' in CHARTVIEW - - -def test_compact_colorbar_room_covers_the_labels_it_keeps() -> None: - assert "const COMPACT_COLORBAR_TICK_ROOM = 30;" in CHARTVIEW - assert "const COMPACT_COLORBAR_TITLE_ROOM = 14;" in CHARTVIEW - assert "COMPACT_COLORBAR_GAP + COLORBAR_THICKNESS + COMPACT_COLORBAR_TICK_ROOM" in CHARTVIEW + assert "node.hidden = compactVertical && !endpoint;" in CHARTVIEW + # Restacked, and the beside-the-bar placement is restored on the way out. + assert "tick._xyBesideCss = tick.style.cssText;" in CHARTVIEW + assert "node.style.cssText = node._xyBesideCss;" in CHARTVIEW + assert "const COMPACT_COLORBAR_LABEL_GAP = 3;" in CHARTVIEW + # The reservation is unchanged, which is what keeps the plot space the + # collapse was collapsing for. + assert "COMPACT_COLORBAR_GAP + COLORBAR_THICKNESS + 8" in CHARTVIEW + # The rotated title and the text-free minor ticks are what a phone cannot + # spend; `box.title` keeps the scale name reachable. + assert "'[data-xy-slot=\"colorbar_title\"], [data-xy-colorbar-minor]'" in CHARTVIEW # -- dpr-baked buffers and animation cadence -------------------------------- @@ -513,11 +517,17 @@ def test_a_dpr_change_rescales_the_buffers_baked_in_device_pixels() -> None: assert "this._rescaleDprBakedBuffers();\n this._layout();" in CHARTVIEW -def test_a_dpr_change_coalesces_into_one_resize_frame() -> None: - """Browser zoom changes dpr AND the container box, so a synchronous resize - plus the ResizeObserver's queued one laid out and painted twice.""" - assert "this._queueResize(this.size.w, this.size.h, this.fluid || this.fluidH);" in CHARTVIEW - assert "this._resize(this.size.w, this.size.h); // re-reads devicePixelRatio" not in CHARTVIEW +def test_a_dpr_change_stays_synchronous() -> None: + """`render_smoke_nonumpy.py`'s `dprw` probe calls `_onDprChange()` and reads + `dpr`/`canvas.width`/`chrome.width` on the next line: a DPR change with no + container resize has no later event to piggyback on. Deferring it into + `_queueResize` broke that contract and saved nothing — the ResizeObserver's + queued pass already early-returns when width, height and dpr are unchanged. + """ + assert "this._resize(this.size.w, this.size.h); // re-reads devicePixelRatio" in CHARTVIEW + assert ( + "this._queueResize(this.size.w, this.size.h, this.fluid || this.fluidH);" not in CHARTVIEW + ) def test_data_animations_throttle_the_label_dom_rebuild() -> None: From 52b13584a8a24b296cefc93ec430fa1e699e0f4e Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Thu, 30 Jul 2026 18:03:32 +0000 Subject: [PATCH 4/6] Type the colorbar tick node that carries the stashed beside-bar CSS The compact colorbar restack records each tick's beside-the-bar cssText on the node itself, but the tick was typed HTMLSpanElement by createElement, so tsc rejected the ad-hoc property and js/build.mjs failed before emitting a bundle. Every CI job that builds the render client failed on that one typecheck error. Annotate the local `any`, the same way the legend box's `lg` is annotated for its stashed row list. --- js/src/50_chartview.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/js/src/50_chartview.ts b/js/src/50_chartview.ts index 065b6875..86d95999 100644 --- a/js/src/50_chartview.ts +++ b/js/src/50_chartview.ts @@ -3418,7 +3418,9 @@ export class ChartView { const raw = tickValues[tickIndex]; const value = Number(raw); if (!Number.isFinite(value) || value < Math.min(lo, hi) || value > Math.max(lo, hi)) continue; - const tick = document.createElement("span"); + // `any` because the node carries the stashed beside-the-bar cssText below + // (same reason as the legend box's `lg`). + const tick: any = document.createElement("span"); tick.textContent = hasExplicitTicks && Array.isArray(cb.tick_labels) && From 71c2ba65571ca7698c9d1876d4bda1dd6d8c4dda Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Thu, 30 Jul 2026 18:16:13 +0000 Subject: [PATCH 5/6] Keep the pie-legend fix from breaking the swatch cascade and legend scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pytest regressions from the previous round, both from the same over-reach: the legend row was made a flex line with an ellipsizing label. A flex container blockifies its children's computed display, so an author's `inline-flex` swatch utility computed as `flex` and the Tailwind slot-cascade test failed. And `white-space:nowrap` removed the label wrapping that made a long legend in a narrow chart taller than its height cap, so the box no longer overflowed at all and the resize regression test's `legendHasOverflow` went false. Neither the flex row nor the ellipsis was needed for the reported bug. The sideways scrollbar came from grid columns that refused to shrink; with `minmax(0, max-content)` a long label wraps inside its column, which fixes the scrollbar without dropping text, without touching the swatch's formatting context, and while keeping the wrapping the narrow-chart contract depends on. The row keeps its full name in title/ARIA. The chrome stylesheet is byte-identical to its state before this branch again. Also fix a buffer-shrinking hazard the dpr rescale introduced: it re-uploads whole buffers from _cpuStyle/_cpuRadius, but the streaming-append fast path extends styleBuf on the GPU and advances n without growing those mirrors, so after an append the mirror was short — re-uploading it would shrink the store out from under the appended rows, and scaling it would leave that tail at the old dpr regardless. Such a record is now skipped with its dpr stamp left stale, which is exactly what makes the append guard fall back to the rebuild that renormalizes every row (scripts/append_stream_smoke.py's dprChangeRebuilds). --- CHANGELOG.md | 12 ++++--- js/src/20_theme.ts | 4 +-- js/src/50_chartview.ts | 23 ++++++++++--- spec/design/polar-axes.md | 18 +++++----- tests/test_polar_audit_fixes.py | 49 ++++++++++++++++++++-------- tests/test_static_client_security.py | 5 --- 6 files changed, 73 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3ace714..c8f110b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,15 +71,19 @@ in the README). now rescales the per-instance stroke widths and corner radii that are baked in device pixels, so authored strokes and wedge corners keep their intended size across a zoom. The DPR handler stays synchronous: a DPR change with no container - resize has no later event to piggyback on. + resize has no later event to piggyback on. A trace whose CPU style/radius mirror + no longer spans every row on the GPU — which is what a streaming tail append + leaves behind — is skipped rather than repaired in place, so the existing + append-time rebuild still does the renormalizing for it. - `xy.pie_chart` appears in the generated chart-factory API reference alongside the other polar compositions. -- A legend row too wide for its box ellipsizes instead of growing a horizontal +- A legend row too wide for its box wraps instead of growing a horizontal scrollbar. The box is capped at `--xy-legend-max-width`, but its grid columns were `max-content` and refused to shrink, so an over-wide row overflowed and `overflow:auto` answered sideways — hiding the label it was meant to show. - Vertical scrolling is unchanged; the full text stays available in - `title`/ARIA, matching how categorical tick labels already ellipsize. + Columns are now `minmax(0, max-content)` and the inline axis never scrolls. + Vertical scrolling is unchanged, and rows carry their full name in + `title`/ARIA for the ones the height cap clips. - `xy.pie_chart` no longer prints the same number twice. Values that already sum to 100 — how most pie data arrives — made `show_values` and `show_percent` collide, so `[40, 30, 20, 10]` rendered `Direct 40 (40%)`: a legend row that diff --git a/js/src/20_theme.ts b/js/src/20_theme.ts index 9bfa9036..5d7b523b 100644 --- a/js/src/20_theme.ts +++ b/js/src/20_theme.ts @@ -127,9 +127,7 @@ export const XY_CHROME_CSS = ` :where(.xy [data-xy-slot="tooltip_label"])::after{content:": "} :where(.xy [data-xy-slot="legend"]){left:var(--xy-legend-left,auto);right:var(--xy-legend-right,auto);top:var(--xy-legend-top,auto);bottom:var(--xy-legend-bottom,auto);transform:var(--xy-legend-transform,none);max-width:var(--xy-legend-max-width);max-height:var(--xy-legend-max-height);gap:2px;font-size:11px;background:var(--chart-legend-bg,rgba(128,128,128,.08));border-radius:4px;padding:4px 8px;color:var(--chart-text,inherit)} :where(.xy [data-xy-slot="legend_title"]){font-weight:400;text-align:center} -:where(.xy [data-xy-slot="legend_item"]){display:flex;align-items:center;min-width:0} -:where(.xy [data-xy-slot="legend_label"]){min-width:0;overflow:hidden;text-overflow:ellipsis;white-space:nowrap} -:where(.xy [data-xy-slot="legend_swatch"]){display:inline-block;flex:none;width:var(--xy-legend-swatch-width,12px);height:var(--xy-legend-swatch-height,10px);vertical-align:-1px;background:var(--xy-legend-swatch-paint,transparent);border-radius:2px;margin-right:var(--xy-legend-swatch-margin-right,5px);fill:var(--xy-legend-swatch-fill);stroke:var(--xy-legend-swatch-stroke);stroke-width:var(--xy-legend-swatch-stroke-width);stroke-dasharray:var(--xy-legend-swatch-dasharray)} +:where(.xy [data-xy-slot="legend_swatch"]){display:inline-block;width:var(--xy-legend-swatch-width,12px);height:var(--xy-legend-swatch-height,10px);vertical-align:-1px;background:var(--xy-legend-swatch-paint,transparent);border-radius:2px;margin-right:var(--xy-legend-swatch-margin-right,5px);fill:var(--xy-legend-swatch-fill);stroke:var(--xy-legend-swatch-stroke);stroke-width:var(--xy-legend-swatch-stroke-width);stroke-dasharray:var(--xy-legend-swatch-dasharray)} :where(.xy [data-xy-slot="legend_swatch"] > svg){display:block;width:100%;height:100%} :where(.xy [data-xy-slot="colorbar"]){color:var(--chart-text,inherit);font-size:10px} :where(.xy [data-xy-slot="colorbar_bar"]){background:var(--xy-colorbar-gradient);border:1px solid currentColor;box-sizing:border-box} diff --git a/js/src/50_chartview.ts b/js/src/50_chartview.ts index 86d95999..0db864a2 100644 --- a/js/src/50_chartview.ts +++ b/js/src/50_chartview.ts @@ -2194,6 +2194,19 @@ export class ChartView { if (!record) return; const previous = Number(record._styleDpr); if (!(previous > 0) || previous === dpr) return; + // Repair in place ONLY while the CPU mirrors still cover every row the + // GPU holds. The streaming-append fast path extends styleBuf with a tail + // `bufferSubData` and advances `n` without growing `_cpuStyle` + // (54_kernel.ts), so after an append the mirror is short: re-uploading it + // would shrink the store out from under the appended rows, and scaling it + // would leave that tail at the old dpr either way. Leave `_styleDpr` + // stale instead — the append guard then refuses the fast path and its + // rebuild renormalizes every row at the current dpr, which is the + // fallback that case has always relied on. + const rows = Number(record.n); + if (!(rows > 0)) return; + if (record._cpuStyle && record._cpuStyle.length !== rows * 4) return; + if (record._cpuRadius && record._cpuRadius.length !== rows * 2) return; const factor = dpr / previous; // Widths ride component 2 of the canonical style row; the other three // components (opacity, artist alpha, symbol) are dpr-independent. @@ -2606,10 +2619,12 @@ export class ChartView { // `minmax(0, max-content)`, not bare `max-content`: the box is capped at // `--xy-legend-max-width`, and a column that refuses to shrink below its // content made a long row overflow horizontally — a legend with a horizontal - // SCROLLBAR, which hides the label it is meant to be showing. Vertical - // overflow still scrolls (that is the browser legend's advantage over the - // static exporters, which can only ellipsize); horizontal overflow - // ellipsizes per row instead, with the full text in `title`/ARIA. + // SCROLLBAR, which hides the label it is meant to be showing. Shrinkable + // columns let a long label WRAP inside its column instead, so the inline axis + // never needs to scroll and the text stays whole; the block axis still + // scrolls, which is the browser legend's advantage over the static + // exporters, which can only ellipsize. Row `title`/ARIA carries the full + // name either way, for the rows the block-axis cap does clip. lg.style.cssText = "position:absolute;" + `display:grid;grid-template-columns:repeat(${horizontal ? ncols : 1},minmax(0,max-content));` + "column-gap:2em;row-gap:.5em;overflow-x:hidden;overflow-y:auto;"; diff --git a/spec/design/polar-axes.md b/spec/design/polar-axes.md index 8803cf31..33b7d0a5 100644 --- a/spec/design/polar-axes.md +++ b/spec/design/polar-axes.md @@ -130,14 +130,16 @@ Five properties this pins down, each of which has matching coverage: under it. `_polar_legend_reserve` (`_svg.py`, mirrored by `_polarLegendReserve`) therefore takes a gutter off the canvas edge **before** the disc is fitted, and records it as `plot["legend_box_*"]` / `view._legendBox`; the legend places and - bounds itself in that box, and `loc` chooses where within it. `_polar_legend_room` - (22% of the canvas width, clamped to 120–200 px) on the side `loc` names, or `_POLAR_LEGEND_BAND` (64 px) beneath the - disc at compact widths, where a side gutter would leave a disc too small to - read. Derived from the canvas width rather than measured from the label set, for the - same reason the subdivision count is a shared formula: every renderer knows the - canvas to the pixel, while a measured reservation would drift with each - renderer's font metrics. A label wider than the gutter ellipsizes with its full - text in `title`/ARIA, as the static exporters already do against the plot width. Nothing is reserved when the author supplied an + bounds itself in that box, and `loc` chooses where within it. + `_polar_legend_room` (22% of the canvas width, clamped to 120–200 px) on the + side `loc` names, or `_POLAR_LEGEND_BAND` (64 px) beneath the disc at compact + widths, where a side gutter would leave a disc too small to read. Derived from + the canvas width rather than measured from the label set, for the same reason + the subdivision count is a shared formula: every renderer knows the canvas to + the pixel, while a measured reservation would drift with each renderer's font + metrics. A label wider than the gutter wraps in the browser and ellipsizes in + the static exporters, which have no scroll axis to fall back on; either way the + full text stays in `title`/ARIA. Nothing is reserved when the author supplied an `anchor` (an explicit plot-relative placement they own, still resolved against the plot) or a four-tuple `padding` (which already states the box the plot should occupy, and remains the way to hand-reserve a caption band), and nothing diff --git a/tests/test_polar_audit_fixes.py b/tests/test_polar_audit_fixes.py index 8ed116c9..3fc34284 100644 --- a/tests/test_polar_audit_fixes.py +++ b/tests/test_polar_audit_fixes.py @@ -372,28 +372,32 @@ def test_client_caps_the_title_box_at_the_measured_wrap_width() -> None: # -- legend overflow -------------------------------------------------------- -def test_a_long_legend_row_ellipsizes_instead_of_scrolling_sideways() -> None: +def test_a_long_legend_row_wraps_instead_of_scrolling_sideways() -> None: """A pie legend grew a horizontal scrollbar, hiding the label it was showing. The box is capped at `--xy-legend-max-width`, but its grid columns were `max-content` — they refused to shrink — so an over-wide row overflowed and - `overflow:auto` answered with a sideways scrollbar. Vertical scrolling stays - (it is what the browser legend has over the static exporters, which can only - ellipsize); horizontal overflow now ellipsizes per row. + `overflow:auto` answered with a sideways scrollbar. Shrinkable columns let the + label wrap inside its column instead, so nothing needs to scroll sideways and + no text is dropped. Block-axis scrolling stays: it is what the browser legend + has over the static exporters, which can only ellipsize. + + The row deliberately stays a BLOCK, not a flex line. A flex container + blockifies its children's computed `display`, which would turn an author's + `inline-flex` swatch utility into `flex` + (`test_tailwind_root_customization.py`), and a nowrap label removes the very + wrapping that keeps a narrow chart's legend scrollable rather than clipped + (`test_legend_resize_regression.py`). The swatch keeps aligning through the + `vertical-align` it already carries. """ - theme = (ROOT / "js/src/20_theme.ts").read_text(encoding="utf-8") # Columns that can shrink, and no horizontal scroll axis. assert "minmax(0,max-content)" in CHARTVIEW assert "overflow-x:hidden;overflow-y:auto;" in CHARTVIEW - # Only the LABEL clips. The swatch is `flex:none` and keeps overflow visible, - # so an authored oversized marker still draws outside its 18x14 box. - assert 'data-xy-slot="legend_item"]){display:flex;align-items:center;min-width:0}' in theme - assert ( - 'data-xy-slot="legend_label"]){min-width:0;overflow:hidden;' - "text-overflow:ellipsis;white-space:nowrap}" in theme - ) - assert 'data-xy-slot="legend_swatch"]){display:inline-block;flex:none;' in theme - # An ellipsis must never make text unreachable: same full-text-in-title/ARIA + theme = (ROOT / "js/src/20_theme.ts").read_text(encoding="utf-8") + assert 'data-xy-slot="legend_item"]){' not in theme + assert 'data-xy-slot="legend_label"]){' not in theme + assert 'data-xy-slot="legend_swatch"]){display:inline-block;width:' in theme + # Clipping must never make text unreachable: same full-text-in-title/ARIA # rule the categorical tick labels use. assert "row.title = String(it.name);" in CHARTVIEW assert 'row.setAttribute("aria-label", String(it.name));' in CHARTVIEW @@ -517,6 +521,23 @@ def test_a_dpr_change_rescales_the_buffers_baked_in_device_pixels() -> None: assert "this._rescaleDprBakedBuffers();\n this._layout();" in CHARTVIEW +def test_the_dpr_rescale_defers_to_the_append_rebuild_on_a_short_mirror() -> None: + """The rescale re-uploads whole buffers from `_cpuStyle`/`_cpuRadius`, but the + streaming-append fast path extends `styleBuf` with a tail `bufferSubData` and + advances `n` without growing those mirrors (54_kernel.ts). Re-uploading a short + mirror would shrink the store out from under the appended rows, and scaling it + would leave that tail at the old dpr regardless. Leaving `_styleDpr` stale + hands the repair back to the append guard's rebuild — the fallback + `scripts/append_stream_smoke.py` asserts via `dprChangeRebuilds`. + """ + assert "const rows = Number(record.n);" in CHARTVIEW + assert "if (record._cpuStyle && record._cpuStyle.length !== rows * 4) return;" in CHARTVIEW + assert "if (record._cpuRadius && record._cpuRadius.length !== rows * 2) return;" in CHARTVIEW + # The guard the fallback runs through must stay in place. + kernel = (ROOT / "js/src/54_kernel.ts").read_text(encoding="utf-8") + assert "if (g.styleBuf && g._styleDpr !== this.dpr) return false;" in kernel + + def test_a_dpr_change_stays_synchronous() -> None: """`render_smoke_nonumpy.py`'s `dprw` probe calls `_onDprChange()` and reads `dpr`/`canvas.width`/`chrome.width` on the next line: a DPR change with no diff --git a/tests/test_static_client_security.py b/tests/test_static_client_security.py index 28aa8973..4676435e 100644 --- a/tests/test_static_client_security.py +++ b/tests/test_static_client_security.py @@ -48,11 +48,6 @@ def _read(path: Path) -> str: ':where(.xy [data-xy-slot="tooltip_label"])::after{', ':where(.xy [data-xy-slot="legend"]){', ':where(.xy [data-xy-slot="legend_title"]){', - # The row is a flex line and the label ellipsizes, so a long entry cannot - # push a horizontal scrollbar onto the legend box. Both stay in the `:where()` - # layer so an author's utility class still wins. - ':where(.xy [data-xy-slot="legend_item"]){', - ':where(.xy [data-xy-slot="legend_label"]){', ':where(.xy [data-xy-slot="legend_swatch"]){', ':where(.xy [data-xy-slot="modebar"]){', ':where(.xy) button[data-xy-slot="modebar_button"]{', From e58ad8befda8d2f0b34974c36fba78b351419c5b Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Thu, 30 Jul 2026 18:23:45 +0000 Subject: [PATCH 6/6] Give the widest render smoke headroom over its 120 s cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/render_smoke_nonumpy.py timed out at 120 s on both Test jobs for 71c2ba6, with no failing assertion — chromium simply had not finished. The same step measured 98.4 s on the previous commit (18:05:00 to 18:06:38), so the probe had 22 s of margin against a software rasterizer on a shared runner, and a slightly slower runner spends it. It is by far the widest page in the tree: every mark family, LOD drill-in, picking, box select, the modebar, context loss and recovery, and the DPR watch, all through SwiftShader. append_stream_smoke.py already allows 180 s for much less. 300 s keeps a genuine hang failing while jitter no longer does. Also correct the note on the synchronous-DPR test: deferring _onDprChange would have made the dprw probe read a stale dpr and fail its assertion. It was not the cause of the earlier timeout, which was this same marginal cap. --- scripts/render_smoke_nonumpy.py | 11 ++++++++++- tests/test_polar_audit_fixes.py | 6 ++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/scripts/render_smoke_nonumpy.py b/scripts/render_smoke_nonumpy.py index 24aedf26..1e5291f0 100644 --- a/scripts/render_smoke_nonumpy.py +++ b/scripts/render_smoke_nonumpy.py @@ -1329,7 +1329,16 @@ def main() -> None: ], capture_output=True, text=True, - timeout=120, + # This probe is the widest one in the tree — a single page that + # exercises every mark family, LOD drill-in, picking, selection, + # the modebar, context-loss recovery and the DPR watch — and it + # SOFTWARE-rasterizes all of it through SwiftShader. On a GitHub + # runner it measured 98 s against the 120 s it used to allow, so a + # slightly slower runner timed the whole job out with nothing + # broken. Sized for headroom instead: a real hang still fails, + # 22 s of jitter no longer does. `append_stream_smoke.py` already + # allows 180 s for a far smaller page. + timeout=300, ) m = re.search(r"([^<]*)", out.stdout) title = m.group(1) if m else "(none)" diff --git a/tests/test_polar_audit_fixes.py b/tests/test_polar_audit_fixes.py index 3fc34284..f578edf3 100644 --- a/tests/test_polar_audit_fixes.py +++ b/tests/test_polar_audit_fixes.py @@ -542,8 +542,10 @@ def test_a_dpr_change_stays_synchronous() -> None: """`render_smoke_nonumpy.py`'s `dprw` probe calls `_onDprChange()` and reads `dpr`/`canvas.width`/`chrome.width` on the next line: a DPR change with no container resize has no later event to piggyback on. Deferring it into - `_queueResize` broke that contract and saved nothing — the ResizeObserver's - queued pass already early-returns when width, height and dpr are unchanged. + `_queueResize` would read a stale `dpr` there, and it saved nothing anyway — + the ResizeObserver's queued pass already early-returns when width, height + and dpr are all unchanged, so the redundant second frame it was meant to + avoid does not exist. """ assert "this._resize(this.size.w, this.size.h); // re-reads devicePixelRatio" in CHARTVIEW assert (