Let chart chrome keep its styling all the way into a file - #325
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds deterministic ChangesRendering contracts and export behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Figure
participant Payload
participant slot_styles
participant SVGWriter
participant RasterWriter
Figure->>Payload: build figure specification
Payload->>slot_styles: normalize dom.styles
slot_styles->>SVGWriter: provide slot declarations
slot_styles->>RasterWriter: provide slot declarations
SVGWriter-->>Payload: emit styled SVG
RasterWriter-->>Payload: emit styled raster
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
XY publishes "five ways to style" a chart, and one of them stopped at the
browser. `styles={slot: {...}}` validated, shipped on the wire, and was then
dropped in full by the SVG, PNG and PDF writers — the capability inventory put
it at 2 of 29 slots styleable natively. A chart styled for its live view
exported unstyled, silently, which is the failure mode §28 exists to prevent.
The nine slots that name chrome a static file actually contains — title,
axis_title, tick_label, the three legend slots, the three colorbar slots — now
carry font-size, font-weight, font-style, font-family, letter-spacing, opacity
and the text paint into all three renderers. That is 10 of 29. The rest are
live-only chrome (tooltip, modebar, crosshair, selection, badge) with nothing
in a file to paint, and `class_names` cannot apply in a file at all: a class
selects a rule out of a stylesheet an export does not have.
The plumbing was already there — `spec["dom"]["styles"]` has always shipped —
so this is a read, not a protocol change. Unstyled output stays byte-identical;
a test pins that. The raster's baked atlas is one face, so it honors a slot's
size and paint and leaves the typeface properties to the vector writers, which
is the one divergence and is pinned too. Where two surfaces name the same
chrome the narrower selector wins: an axis's own label_color over the
chart-wide slot.
Three more gaps the same audit turned up, all in the same seam:
- The legend had three spellings that disagreed. `styles={"legend": ...}`,
`xy.legend(style=...)` and `--chart-legend-bg` now merge into one declaration
block before either writer sees it. The theme token is documented and public
and neither static writer read it at all. An explicit `background` also
painted at 8% opacity while the browser painted it opaque; the frame-alpha
token stays the separate knob for the default grey frame.
- `loc="best"` — Matplotlib's default, so the first spelling users try — fell
through the writers' substring match and parked the legend dead center, on
top of the data. It is implemented now, scoring each candidate box by the
fraction of sampled marks inside it. It resolves once at payload-build time,
so the client and both writers receive a settled location and cannot
disagree. The vocabulary is also closed: `"top-left"`, `"northeast"` and
`"LOWER RIGHT"` used to land the legend somewhere arbitrary rather than fail.
- `theme()` accepted any keyword and emitted a CSS declaration no renderer
reads, so `theme(grid_colour=...)` changed nothing and said nothing. Unknown
tokens raise and name the accepted set — and the real `--chart-*` tokens
(tooltip_bg, legend_bg, annotation_text, accent, focus, …) became reachable
by name instead of being dead CSS.
Evidence: spec/assets/chrome-styling-before-after.png.
Closing the `loc` vocabulary caught XY's own documentation: the components/facets-and-layers page passes `loc="top left"`, which is not Matplotlib's spelling, so the substring match found "left" but neither "upper" nor "lower" and the page has been rendering a *center*-left legend. Refusing it would be pedantry — `top` and `bottom` name the same edges as `upper` and `lower`, and they are what CSS, Plotly and that doc page all say. So the vocabulary normalizes what is unambiguous and refuses only what is genuinely a guess: case and whitespace are free, `-`/`_` separate words, either word order works, `right`/`left` alone mean the centered edges, and `top`/`bottom` map to `upper`/`lower`. `northeast`, `outside right` and `middle left` still raise, because there is no honest answer for them. Also here, both surfaced by the same audit: - A legend style written in ordinary kebab-case CSS — the spelling every doc example uses — lost its `box-shadow` and `border-radius` outright, because the writers key on the browser's camelCase property names. Folding the slot into the merged declaration block fixes it; pinned by a test that asserts the two spellings produce identical bytes. - `docs/styling/chrome-slots.md` still published the pre-#311 slot count and the old all-dropped row.
Alek99
force-pushed
the
alek/cust-audit-3
branch
from
July 27, 2026 01:39
bfaece4 to
e550849
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/xy/_legendfit.py`:
- Around line 116-126: Update the normalization logic around the visible-mark
scoring helper to use each axis’s display transform rather than raw linear
domain arithmetic, including log and symlog scales. Preserve reversed-axis
handling, but discard transformed samples outside [0, 1] instead of clipping
them to the boundaries before occupancy scoring. Add regressions covering fixed
domains and log-scaled axes, including out-of-domain marks.
In `@python/xy/_raster.py`:
- Around line 1049-1051: Update the extra-legend loop in the raster writer to
mirror `_svg.py`: resolve each `extra` through `legend_options_with_slot(spec,
extra)`, then pass the resulting legend options plus `legend_label_slot` and
`legend_title_slot` into `_emit_legend` so extra legends receive the same
background, legend, label, and title styling as the main legend.
In `@python/xy/_svg.py`:
- Around line 3368-3381: Update _colorbar so title_attrs and tick_attrs provide
an explicit 10px font-size fallback when title_slot or tick_slot is unset,
matching _emit_colorbar’s slot_font_size(..., 10.0) behavior. Preserve any
explicitly configured slot font sizes and ensure both colorbar text paths use
the aligned default.
In `@python/xy/components.py`:
- Around line 4228-4231: Remove the key.startswith("__") conversion branch in
the theme property handling, or validate its resolved custom property against
the supported token vocabulary before returning it. Ensure unsupported names
such as __grid_colour are rejected, while direct custom properties remain
available through the documented style= escape hatch.
In `@spec/api/capability-matrix.md`:
- Line 17: Update the capability-matrix summary generated by
scripts/gen_capability_matrix.py to state that 10 slots reach native writers,
with 9 using per-slot styles and root using the chart-level style token bag.
Regenerate spec/api/capability-matrix.md so its summary matches the Notes and
export documentation, and remove the contradictory “through a channel other than
per-slot styles” wording.
In `@spec/api/styling.md`:
- Around line 714-716: Align raster text documentation with the renderer: in
spec/api/styling.md lines 714-716 distinguish SVG/PDF support from raster’s
size-and-paint-only subset; in docs/styling/capabilities.md lines 90-98 state
that raster supports only font size and text paint; in
docs/styling/chrome-slots.md lines 259-266 remove raster guarantees for opacity
and letter-spacing; and in CHANGELOG.md lines 15-23 remove claims that those
properties survive all three renderers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83e71028-addd-40de-8558-d68ce171be59
⛔ Files ignored due to path filters (1)
spec/assets/chrome-styling-before-after.pngis excluded by!**/*.png
📒 Files selected for processing (17)
CHANGELOG.mddocs/api-reference/limitations-and-alpha-status.mddocs/styling/capabilities.mddocs/styling/chrome-slots.mdpython/xy/_legendfit.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_validate.pypython/xy/components.pypython/xy/styling/capabilities.pyspec/api/capability-matrix.mdspec/api/export.mdspec/api/styling.mdtests/test_export_style_survival.pytests/test_legend_best_placement.pytests/test_theme_tokens.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/api/styling.md`:
- Around line 781-785: Expand the `best` placement specification around
`xy._legendfit` to normatively define mark sampling, including the stride,
maximum sample cap, density tiers, and near-tie threshold. Record every
decimation and tier decision explicitly, and preserve the stated behavior of
selecting the least-occupied candidate with earlier-candidate preference on
near-ties.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a6609af-9804-48bf-8a55-4abcfc26629c
⛔ Files ignored due to path filters (1)
spec/assets/chrome-styling-before-after.pngis excluded by!**/*.png
📒 Files selected for processing (16)
docs/api-reference/limitations-and-alpha-status.mddocs/styling/capabilities.mddocs/styling/chrome-slots.mdpython/xy/_legendfit.pypython/xy/_payload.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/_validate.pypython/xy/components.pypython/xy/styling/capabilities.pyspec/api/capability-matrix.mdspec/api/export.mdspec/api/styling.mdtests/test_export_style_survival.pytests/test_legend_best_placement.pytests/test_theme_tokens.py
🚧 Files skipped from review as they are similar to previous changes (14)
- python/xy/_payload.py
- docs/styling/chrome-slots.md
- python/xy/components.py
- docs/api-reference/limitations-and-alpha-status.md
- python/xy/styling/capabilities.py
- spec/api/capability-matrix.md
- docs/styling/capabilities.md
- python/xy/_validate.py
- spec/api/export.md
- tests/test_theme_tokens.py
- python/xy/_legendfit.py
- tests/test_export_style_survival.py
- python/xy/_raster.py
- python/xy/_svg.py
Review of the previous two commits found four real defects, three of them in code they introduced. `loc="best"` scored occupancy in *value* space. On a log axis that is the wrong space: 1..10000 is four evenly spaced decades on screen, but raw subtraction puts them at 0, .001, .01, .1, 1 — so `best` saw a series crushed against one edge and guarded a corner the marks do not occupy. Placement now applies the axis display transform (`log`/`symlog`, matching `_svg._Scale`) before scoring. Out-of-domain samples were clamped onto the plot edges with `np.clip`. Every renderer clips those marks away, so clamping invented occupancy in a corner the viewer sees as empty — a chart with a fixed `domain=` and a tail outside it would refuse the corner that tail visibly vacated. They are dropped instead, and a series with nothing visible left is not scored at all. The raster writer applied the legend slot and the `--chart-legend-bg` token to the *main* legend only; `_svg` applied them to every legend. So a chart with a categorical color channel — which adds a second legend — styled correctly in SVG and PDF and silently skipped that legend in PNG. Both writers now fold the same declaration block into every legend. `theme()` grew a `__name` escape hatch that mapped to `--name`, which quietly reopened the dead-CSS hole the closed vocabulary exists to shut: `theme(__grid_colour=...)` emitted `--grid-colour` and changed nothing. Removed; `style=` is the documented, validated way to set a custom property. Also, without changing pixels: - `_SLOT_RASTER_PROPS` was dead and overstated. The glyph primitive takes a size and one RGBA paint, so `opacity` and `letter-spacing` are vector-only along with the typeface properties. Now `SLOT_RASTER_PROPS`, used by the tests, and the spec says which subset each writer reads instead of implying all three honor all eight. - An unstyled colorbar renders 11px in SVG (inherited from the root) and 10px in the raster. That predates per-slot support and closing it would change the pixels of every existing unstyled colorbar, so it is registered as a known renderer divergence rather than silently left for someone to find. - The generated matrix's one-line summary said the 10 native-reaching slots got there "through a channel other than per-slot styles" — true of `root` alone, and the opposite of what these commits ship for the other nine. - The `best` sampling policy — 4096-point stride, 512-sample cap, display space, drop-not-clamp, candidate order, 0.02 tie band — is now normative in the spec (§28), not left for a reader to infer from the code. - `xy.legend(style=...)` is canonicalized to the writers' property spelling like the slot already was. It reaches them through `chrome_styles` today, so this changes nothing now, but a legend built without that mirror would have lost its kebab-case declarations.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/api/styling.md (1)
757-760: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the browser-only slot list exhaustive or explicitly non-exhaustive.
The capability matrix also marks
chrome,canvas,labels,legend_item,legend_swatch,colorbar_bar, andannotation_labelas nativenone, but this paragraph names only tooltip, modebar, crosshair, selection, and badge slots. Replace “the remaining slots are” with “including” or list all unsupported slots so the specification matches the capability matrix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/api/styling.md` around lines 757 - 760, Update the browser-only slot description in the capability-matrix paragraph to use non-exhaustive wording such as “including,” or expand the list to include every native-none slot identified by the matrix, including chrome, canvas, labels, legend_item, legend_swatch, colorbar_bar, and annotation_label.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/gen_capability_matrix.py`:
- Around line 67-69: Update the native-writer description in the generated
capability-matrix text around the counts['slots_styleable_natively'] entry to
match the no-cascade contract: state that all native-writer styling is merged
into and read from the chart-level style= token bag, rather than claiming
per-slot styles are consumed directly. Keep the documented root-slot behavior
consistent with the same channel.
In `@spec/api/styling.md`:
- Around line 793-799: Update the “Series stride” description in the normative
sampling table to qualify O(1) placement as applying only to the 4096-point
sampling fast path; explicitly state that the no-finite-pair fallback scans the
full array and therefore has O(n) worst-case cost.
In `@tests/test_legend_best_placement.py`:
- Around line 164-175: Update test_best_scores_a_log_axis_in_display_space to
use an asymmetric, non-monotonic set of log-scaled x/y values and domains so
display-space scoring differs from raw-space scoring. Also update the symlog
placement fixture around the referenced lines to use interior values rather than
only -100, 0, and 100, and pass explicit non-default x_constant and y_constant
values. Preserve assertions that verify the intended legend placement.
---
Outside diff comments:
In `@spec/api/styling.md`:
- Around line 757-760: Update the browser-only slot description in the
capability-matrix paragraph to use non-exhaustive wording such as “including,”
or expand the list to include every native-none slot identified by the matrix,
including chrome, canvas, labels, legend_item, legend_swatch, colorbar_bar, and
annotation_label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d683219-3487-49d0-b5a8-56846124a6cc
📒 Files selected for processing (12)
docs/styling/capabilities.mdpython/xy/_legendfit.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/components.pypython/xy/styling/capabilities.pyscripts/gen_capability_matrix.pyspec/api/capability-matrix.mdspec/api/styling.mdtests/test_export_style_survival.pytests/test_legend_best_placement.pytests/test_theme_tokens.py
💤 Files with no reviewable changes (1)
- python/xy/components.py
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/styling/capabilities.md
- tests/test_theme_tokens.py
- python/xy/styling/capabilities.py
- python/xy/_legendfit.py
- tests/test_export_style_survival.py
- python/xy/_raster.py
- python/xy/_svg.py
A second review pass found the earlier "known divergence" note was the wrong call, and two of the tests added with it were vacuous. The colorbar text size is not a divergence to document, it is an SVG bug. The `colorbar` slot's stylesheet rule is `font-size:10px` and the raster writer passes 10 explicitly; the SVG writer emitted no size at all and inherited the root `<svg>`'s 11px, so it was the odd renderer out on every unstyled colorbar. It names the size now, from a `COLORBAR_FONT_SIZE` both writers read, and the divergence entry is removed because the divergence is gone. Every other slot already agreed — colorbar was the single exception, which is what made it look like a policy rather than a mistake. The log and symlog tests did not test anything. Both fixtures were symmetric: `test_best_scores_a_log_axis_in_display_space` put identical decades on both axes, so value-space scoring saw the same diagonal and returned `"upper left"` too, and the symlog case used `[-100, 0, 100]` on a symmetric domain, which is invariant to the constant because symlog is odd. Both passed with the transform removed. They now use a scatter whose display-space and value-space occupancy genuinely disagree (`"lower left"` vs `"upper right"`) and interior asymmetric symlog values with two explicit constants, and a guard test asserts the fixture itself still discriminates. Verified by mutation: reverting the transform, the constant, or drop-not-clamp each fails the tests that cover it. Two smaller corrections: - The spec claimed placement "costs O(1) on a large series". It is a fast path, not a bound — a sample with no finite pair falls back to scanning the full array, so the worst case is O(n) once at build time. Says so now. - The generated matrix still told readers the native writers "read the chart-level `style=` token bag and nothing per-slot", which is what this branch stopped being true. It now describes the property subset the writers actually read, and why `class_names=` still reaches neither.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_legend_best_placement.py`:
- Around line 204-208: Correct the explanatory comment in
test_symlog_normalization_uses_the_axis_constant so it identifies [-100, 0, 100]
as the old problematic symmetric edge fixture and describes [0.0, 1.0, 10.0,
100.0] as the interior, asymmetric values used by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96a66f80-065e-4272-b7dd-85f52381710a
📒 Files selected for processing (8)
docs/styling/capabilities.mdpython/xy/_raster.pypython/xy/_svg.pypython/xy/styling/capabilities.pyscripts/gen_capability_matrix.pyspec/api/capability-matrix.mdspec/api/styling.mdtests/test_legend_best_placement.py
💤 Files with no reviewable changes (2)
- docs/styling/capabilities.md
- python/xy/styling/capabilities.py
🚧 Files skipped from review as they are similar to previous changes (4)
- spec/api/capability-matrix.md
- spec/api/styling.md
- python/xy/_svg.py
- python/xy/_raster.py
Comment on lines
+204
to
+208
| def test_symlog_normalization_uses_the_axis_constant() -> None: | ||
| # Interior, asymmetric values: `[-100, 0, 100]` on a symmetric domain is | ||
| # invariant to the constant (symlog is odd), so it cannot detect whether the | ||
| # constant is read at all. | ||
| values = np.array([0.0, 1.0, 10.0, 100.0]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment mislabels the old (bad) fixture as the new one.
The comment says [-100, 0, 100] is the "interior, asymmetric" values, but that's the previously-flagged problematic fixture (symmetric about zero, at domain edges, invariant to the constant since symlog is odd) — not the new [0.0, 1.0, 10.0, 100.0] values actually used below. As written, the comment inverts the explanation and will confuse future readers trying to understand why this fixture design was chosen.
✏️ Proposed fix
- # Interior, asymmetric values: `[-100, 0, 100]` on a symmetric domain is
- # invariant to the constant (symlog is odd), so it cannot detect whether the
- # constant is read at all.
+ # `[-100, 0, 100]` on a symmetric domain is invariant to the constant
+ # (symlog is odd), so it cannot detect whether the constant is read at
+ # all. Use interior, non-symmetric values spanning multiple decades
+ # instead.
values = np.array([0.0, 1.0, 10.0, 100.0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_symlog_normalization_uses_the_axis_constant() -> None: | |
| # Interior, asymmetric values: `[-100, 0, 100]` on a symmetric domain is | |
| # invariant to the constant (symlog is odd), so it cannot detect whether the | |
| # constant is read at all. | |
| values = np.array([0.0, 1.0, 10.0, 100.0]) | |
| def test_symlog_normalization_uses_the_axis_constant() -> None: | |
| # `[-100, 0, 100]` on a symmetric domain is invariant to the constant | |
| # (symlog is odd), so it cannot detect whether the constant is read at | |
| # all. Use interior, non-symmetric values spanning multiple decades | |
| # instead. | |
| values = np.array([0.0, 1.0, 10.0, 100.0]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_legend_best_placement.py` around lines 204 - 208, Correct the
explanatory comment in test_symlog_normalization_uses_the_axis_constant so it
identifies [-100, 0, 100] as the old problematic symmetric edge fixture and
describes [0.0, 1.0, 10.0, 100.0] as the interior, asymmetric values used by the
test.
Main's Matplotlib-compatibility stack (#280, #281) landed independent work on the same seam, so four files conflicted for real rather than textually. `_svg.py` and `_raster.py` had both grown per-slot chrome styling. Main read `spec["dom"]["styles"]["title"]` inline for the title and axis title, moved chrome text to Matplotlib's weight-400 default, reworked legend geometry around a layout-measured `font_size`, and added legend hatching, per-legend clipping for anchored legends, and `_native_font_emphasis`. This branch had routed a wider set of slots through shared helpers. Neither side wins line-by-line, so each hunk keeps main's geometry and defaults and routes the lookup through the helpers; `_raster.py` was reset to main's copy and the slot work re-applied on top, because hand-merging would have silently damaged the new hatch and clip paths. Three consequences worth naming: - The atlas turns out to carry a bold and an italic face, so `SLOT_RASTER_PROPS` gains `font-weight` and `font-style`. The vector-only set is now `font-family`, `letter-spacing` and `opacity` — no family axis, no per-glyph advance, no alpha on the blit. - Main reads the raw `dom.styles` block, which keeps whatever spelling the caller used, so `styles={"title": {"font_size": 22}}` missed its `font-size` lookup. Routing through `slot_styles` canonicalizes both spellings. - `slot_text_attrs` escaped with `escape`, which leaves `"` intact — a quoted font-family stack closed the attribute and broke the document. It uses `_escape_attr` now, which is the helper main added for exactly this. `legend(loc=)` needed a design decision rather than a merge. Main resolves `top`/`bottom` inside `_legend_layout` and pins the core to pass the caller's spelling through, which fixes the same `loc="top left"` bug this branch fixed by normalizing at validation. Main's design wins: validation now only lowercases and collapses whitespace, checks the vocabulary with aliases folded in for the check alone, and returns the caller's string. The part main does not cover — refusing `northeast`, `middle left`, `zzz` and `""` instead of landing the legend somewhere arbitrary — is kept. `spec/api/styling.md` took both new sections. The before/after sheet is re-rendered against current main: the `loc="top left"` panel is gone, because main now places it correctly on its own, replaced by an unrecognized `loc` that this branch refuses rather than centering. 3078 tests pass. `ty` reports 36 diagnostics, all of them in files byte-identical to origin/main — the mpl-compat stack raised the baseline from 13, and this branch adds none.
Only conflict was a comment in tests/test_export_style_survival.py that #328 updated from 23 to 29 slots. Kept this branch's wording, which reads the count from CHART_DOM_SLOTS instead of writing it down, so the next slot addition cannot leave it stale again.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/_svg.py (1)
2303-2331: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAxis-title font attrs: slot/axis precedence is inverted or dropped, unlike the raster writer.
append_axis_titledoes not correctly implement its own stated contract ("the axis's own label_* keys ... win over the chart-wide slot; the slot supplies whatever they leave unset"):
- When
label_font_family/label_font_styleis set on the axis,slot_text_attrs(slot, ...)is bypassed entirely, so theaxis_titleslot'sletter-spacing/opacity(andfont-styleif only family was set) are silently dropped instead of falling through.- When neither is set,
weight = axis_style.get("label_font_weight", 400)is passed intoslot_text_attrs(slot, font_weight=weight), but insideslot_text_attrsthedefaultsvalue only applies when the slot itself lacks the property (style.get(prop, defaults.get(...))). So when both an axislabel_font_weightand a slotfont-weightare set, the slot's value wins, inverting the documented "narrower selector wins" precedence.The raster writer (
python/xy/_raster.py'semit_axis_title) gets this right by putting the narrower axis key first and the slot as its fallback:axis_style.get("label_font_weight", axis_title_slot.get("font-weight", 400))andaxis_style.get("label_font_style") or axis_title_slot.get("font-style"). This is exactly the kind of SVG/raster divergencetest_native_raster_matches_the_svg_writer_on_slot_stylingexists to catch, but that test doesn't exercise an axis-level override combined with anaxis_titleslot.🐛 Proposed fix: merge per property instead of swapping the whole attribute set
- # The axis's own label_* keys are the narrower selector, so they win - # over the chart-wide slot; the slot supplies whatever they leave unset. - family = axis_style.get("label_font_family") - font_style = axis_style.get("label_font_style") - weight = axis_style.get("label_font_weight", 400) - paint = _css(axis_style.get("label_color"), "") or slot_text_color(slot, default_text) - font_attrs = (f' font-family="{_escape_attr(family)}"' if family is not None else "") + ( - f' font-style="{_escape_attr(font_style)}"' if font_style is not None else "" - ) - if not font_attrs: - font_attrs = slot_text_attrs(slot, font_weight=weight) - else: - font_attrs = f' font-weight="{_escape_attr(weight)}"' + font_attrs + # The axis's own label_* keys are the narrower selector, so they win + # over the chart-wide slot; the slot supplies whatever they leave + # unset. Merge per property (mirroring `_raster.emit_axis_title`) + # rather than swapping the whole attribute set, or an axis-authored + # font-family/font-style silently drops the slot's letter-spacing + # and opacity, and an axis-authored font-weight alone is silently + # overridden by the slot's own font-weight. + paint = _css(axis_style.get("label_color"), "") or slot_text_color(slot, default_text) + merged_slot = dict(slot) + for style_key, slot_prop in ( + ("label_font_family", "font-family"), + ("label_font_style", "font-style"), + ("label_font_weight", "font-weight"), + ): + if axis_style.get(style_key) is not None: + merged_slot[slot_prop] = axis_style[style_key] + font_attrs = slot_text_attrs(merged_slot, font_weight=400)Want me to also add a regression test alongside
test_the_narrower_selector_wins_over_the_chart_wide_slotcoveringlabel_font_weight/label_font_familyvs. anaxis_titleslot withletter-spacing/opacity?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/_svg.py` around lines 2303 - 2331, Update append_axis_title to merge axis-level label_font_family, label_font_style, and label_font_weight with axis_title slot attributes per property, preserving slot attributes such as letter-spacing and opacity. Use axis values when present and slot values as fallbacks, including slot font-style when only the axis family is set, while retaining the existing default weight of 400 and matching emit_axis_title’s precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/xy/_svg.py`:
- Around line 2303-2331: Update append_axis_title to merge axis-level
label_font_family, label_font_style, and label_font_weight with axis_title slot
attributes per property, preserving slot attributes such as letter-spacing and
opacity. Use axis values when present and slot values as fallbacks, including
slot font-style when only the axis family is set, while retaining the existing
default weight of 400 and matching emit_axis_title’s precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3781bb64-1c44-47db-b59a-77d104a657b3
⛔ Files ignored due to path filters (1)
spec/assets/chrome-styling-before-after.pngis excluded by!**/*.png
📒 Files selected for processing (9)
python/xy/_raster.pypython/xy/_svg.pypython/xy/_validate.pypython/xy/components.pypython/xy/styling/capabilities.pyspec/api/styling.mdtests/test_export_style_survival.pytests/test_legend_best_placement.pytests/test_png_export.py
🚧 Files skipped from review as they are similar to previous changes (5)
- python/xy/_validate.py
- python/xy/styling/capabilities.py
- spec/api/styling.md
- python/xy/components.py
- python/xy/_raster.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
XY publishes "five ways to style" a chart, and one of them stopped at the
browser.
styles={slot: {...}}validated, shipped on the wire, and was thendropped in full by the SVG, PNG and PDF writers — the capability inventory put it
at 2 of 29 slots styleable natively. A chart styled for its live view
exported unstyled, silently, which is the failure mode §28 exists to prevent.
Per-slot styling reaches a file — 2 of 29 slots → 10
The nine slots that name chrome a static file actually contains —
title,axis_title,tick_label, the three legend slots, the three colorbar slots —now carry
font-size,font-weight,font-style,font-family,letter-spacing,opacityand the text paint (fill, orcolor) into allthree renderers.
The plumbing was already there —
spec["dom"]["styles"]has always shipped — sothis is a read, not a protocol change. Unstyled output stays byte-identical,
pinned by a test.
The rest stay browser-only, and for a reason rather than by omission: the
remaining slots are live chrome (
tooltip*,modebar*,crosshair_*,selection,badge*) with nothing in a file to paint, andclass_namescannotapply in a file at all — a class selects a rule out of a stylesheet an export
does not have. The raster's baked atlas is a single face, so it honors a slot's
size and paint and leaves the typeface properties to the vector writers; that is
the one divergence, and it is pinned too. Where two surfaces name the same
chrome the narrower selector wins: an axis's own
label_colorbeatsstyles={"axis_title": ...}.Three more gaps in the same seam
The legend had three spellings that disagreed.
styles={"legend": ...},xy.legend(style=...)and--chart-legend-bgnow merge into one declarationblock before either writer sees it. The theme token is documented and public and
neither static writer read it at all. An explicit
backgroundalso painted at8% opacity while the browser painted it opaque, so
background="#fef3c7"exported as a barely visible tint;
--xy-legend-frame-alpharemains the separateknob for the default grey frame.
loc="best"was ignored. It is Matplotlib's defaultloc— the firstspelling users try — and it fell through the writers' substring match and parked
the legend dead center, on top of the data. It now scores each candidate box by
the fraction of sampled marks inside it and keeps the least occupied, preferring
the earlier candidate on a near-tie. It resolves once, at payload-build time
(
xy._legendfit), so the client and both static writers receive a settledlocation and cannot disagree.
The vocabulary is also closed — and closing it caught XY's own documentation.
locwas an unvalidated substring match, so"northeast"landed dead centerand
"top left"landed center-left, which is what thecomponents/facets-and-layersdocs page has been rendering. Spellings that areunambiguous are now normalized rather than refused: case and whitespace are
free,
-/_separate words, either word order works,"right"/"left"alonemean the centered edges, and
top/bottommap toupper/lower.A legend style written in ordinary kebab-case CSS — the spelling every doc
example uses — also lost its
box-shadowandborder-radiusoutright, becausethe writers key on the browser's camelCase names. The merged declaration block
fixes that too.
theme()accepted any keyword and emitted a CSS declaration no rendererreads, so
theme(grid_colour=...)— or any typo — changed nothing and saidnothing. Unknown tokens raise and name the accepted set, and the real
--chart-*tokens (tooltip_bg,legend_bg,annotation_text,accent,focus, …) became reachable by name instead of being dead CSS.Verification
3078 passed, 4 skipped;ty checkat the 13-diagnostic mainbaseline;
ruff check/formatand all pre-commit hooks green.pristine
origin/mainworktree and against this branch.spec/api/export.md§9,spec/api/styling.md,docs/api-reference/limitations-and-alpha-status.md, and the generatedcapability matrix.
Summary by CodeRabbit
loc="best"legend placement with display-space scoring (including log/symlog handling).class_namesbehavior.loc="best", theme token validation, and cross-export style survival.