Fix chrome-slot styling contract: snake_case keys + selection paint - #40
Merged
Conversation
Two fixes to the "your styles always win" styling contract (docs/styling.md),
both surfaced by an audit of the CSS/customization surface.
1. snake_case style keys were silently dropped in the browser. The documented
API form — `styles={"title": {"font_size": 18}}` — ships the raw snake_case
key in the spec (Python kebab-normalizes only for its grammar check), and the
client's _stylePropertyName only converted camelCase, so it called
setProperty("font_size", ...) which the browser ignores. Multi-word
properties (font_size, letter_spacing, line_height, ...) validated fine and
then rendered unstyled. Normalize `_`->`-` in _stylePropertyName so the
snake/camel/kebab forms are interchangeable; this also lets the unitless
check see the real (kebab) name so `line_height: 1.5` stays unitless.
2. The `selection` slot broke the contract. _updateBand re-pinned border and
background inline on every pointermove, so class_names={"selection": ...}
(and styles[selection]) lost to the inline style. Drive the band via a
data-fc-band attribute and move the paint into a zero-specificity :where()
default, mirroring the canvas-cursor pattern. Also documents the previously
undocumented --chart-zoom-selection / --chart-zoom-selection-fill tokens.
Bundles regenerated via `node js/build.mjs`. Adds regression coverage in
test_static_client_security.py.
Merging this PR will not alter performance
Comparing Footnotes
|
Follow-up to the styling-slot fixes. _applyStyle already converts a snake_case
`max_height` to the CSS `max-height` on the element, but _slotStyleValue — the
guard _resize uses to decide whether to re-apply the automatic legend cap — did
a raw hasOwnProperty lookup and only recognized `max-height`/`maxHeight`. A
snake_case `styles={"legend": {"max_height": 50}}` therefore slipped past the
guard, so on a responsive resize the auto-cap overwrote the explicit 50px with
the grown plot height (browser-verified 50px -> 526px).
Canonicalize keys inside _slotStyleValue (via _stylePropertyName) so snake/camel/
kebab all resolve, and collapse the now-redundant double guard to one check.
Adds tests/test_legend_resize_regression.py: a headless-Chromium probe that
renders the standalone export, forces a resize, and asserts the computed
max-height stays 50px (skips when no chromium; retries transient warm-up misses).
Verified it fails on the pre-fix client (afterResize 526px) and passes after.
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
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.
Summary
An audit of the CSS / customization surface (there's no Tailwind/CSS in the repo — styling is the
:where()chrome stylesheet +--chart-*tokens + per-slotclass_names/styles) turned up two ways the "your styles always win" contract indocs/styling.mdwas silently violated. This fixes both.1. snake_case style keys were silently dropped in the browser (HIGH)
The documented headline example ships snake_case keys:
style_mappingkebab-normalizes only for its grammar check (_css_property_name), so validation passes andfont_sizereaches the client unchanged._stylePropertyNameonly converted camelCase (/[A-Z]/), never underscores, so it calledel.style.setProperty("font_size", "18px"), which the browser ignores.Net: every multi-word property in the documented snake_case form (
font_size,letter_spacing,line_height,border_radius, …) validated with a green light and then rendered unstyled — the exact "silently wrong chart" outcome the project sells against.Fix: normalize
_→-in_stylePropertyNameso snake/camel/kebab are interchangeable. This also lets the unitless-property check see the real kebab name, so{"line_height": 1.5}stays unitless instead of getting a spuriouspx. Fixed client-side (not in Python) so it also covers per-annotationstyle={...}and leaves the Python spec contract untouched.2. The
selectionslot pinned paint inline (MEDIUM)_updateBandre-pinnedborder/backgroundinline on every pointermove, soclass_names={"selection": "border-red-500"}(and evenstyles["selection"]) lost to the inline style — the one chrome slot where classes couldn't win.Fix: drive the band via a
data-fc-band="zoom"|"select"attribute and move the paint into a zero-specificity:where()default (mirroring the existing canvas-cursor pattern). Only the mode discriminator + structural position/size stay inline.3. Undocumented tokens (LOW)
--chart-zoom-selection/--chart-zoom-selection-fillwere real, overridable tokens missing from the token table — now documented.Changes
js/src/50_chartview.js— underscore normalization in_stylePropertyNamejs/src/53_interaction.js+js/src/20_theme.js— selection band viadata-fc-band+:where()defaultdocs/styling.md— document the zoom-band tokenspython/xy/static/{index,standalone}.js— regenerated vianode js/build.mjstests/test_static_client_security.py— regression coverage for both fixesVerification
node js/build.mjs --check→ bundles freshcargo build --release+pytest tests/test_static_client_security.py tests/test_components.py tests/test_css_validation.py tests/test_figure.py→ 251 passedFollow-up (not in this PR)
The client tests are source-grep assertions — which is how #1 slipped through (the numeric-styles test greps for the px logic but never runs it). A real DOM-level regression test via the existing Playwright dev-dep (render with
styles={"title": {"font_size": 30}}, assertgetComputedStyle(...).fontSize === "30px") would close the gap for good. Happy to add it if wanted. Separately,modebaropacity andlegendmax-height are also pinned inline (defensible as state/layout) — could get a doc note or the same treatment.