From 209a050dc76d78c4da2315cbb7e297805d3b5ce4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 21:51:33 +0000 Subject: [PATCH] svg: IDPrefix keeps several documents on one page apart Every SVG document counts its clip, marker and gradient ids from one and names its title and desc with fixed ids. Inline two charts into one HTML page and they share a namespace: the second chart's url(#c1) resolves to the first chart's clip path, cutting its cells to the wrong rectangle, and its aria-labelledby reads out the first chart's title. svg.IDPrefix puts a caller-chosen prefix in front of every id the document defines and every reference to one. With no prefix the output is byte for byte what it was. A prefix that cannot start an XML name is reported on Open, as a bad font is. geom.Text's doc now says outright that labels over a heatmap need the heatmap's ColorBy repeated on the text layer for the light/dark ink to follow the cells; the layer never reads the layer beneath it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LNzcSGVVqpaZ85k19GV3TK --- backend/svg/backend.go | 15 ++++++------ backend/svg/svg_test.go | 53 +++++++++++++++++++++++++++++++++++++++++ backend/svg/target.go | 43 +++++++++++++++++++++++++++++++++ geom/text.go | 4 ++++ 4 files changed, 108 insertions(+), 7 deletions(-) diff --git a/backend/svg/backend.go b/backend/svg/backend.go index 1e85a0e..e4a8a7a 100644 --- a/backend/svg/backend.go +++ b/backend/svg/backend.go @@ -54,7 +54,8 @@ func newBackend(out io.Writer, w, h int, dpr float64, o options) *backend { } // The ids the accessible title and description are written under. A document -// has one chart in it, so they need no counter. +// has one chart in it, so they need no counter; [IDPrefix] is what keeps two +// documents on one page apart. const ( titleID = "figure-title" descID = "figure-desc" @@ -74,11 +75,11 @@ func (b *backend) accessibilityAttrs(head *bytes.Buffer) { head.WriteString(` role="img" aria-labelledby="`) switch { case b.desc.Title != "" && b.desc.Detail != "": - head.WriteString(titleID + " " + descID) + head.WriteString(b.opts.idPrefix + titleID + " " + b.opts.idPrefix + descID) case b.desc.Title != "": - head.WriteString(titleID) + head.WriteString(b.opts.idPrefix + titleID) default: - head.WriteString(descID) + head.WriteString(b.opts.idPrefix + descID) } head.WriteString(`"`) } @@ -88,13 +89,13 @@ func (b *backend) accessibilityAttrs(head *bytes.Buffer) { // reader should find them. func (b *backend) accessibilityElements(head *bytes.Buffer) { if b.desc.Title != "" { - head.WriteString(``) + head.WriteString(`<title id="` + b.opts.idPrefix + titleID + `">`) xmlEscape(head, b.desc.Title) head.WriteString(``) b.nl(head) } if b.desc.Detail != "" { - head.WriteString(``) + head.WriteString(``) xmlEscape(head, b.desc.Detail) head.WriteString(``) b.nl(head) @@ -103,7 +104,7 @@ func (b *backend) accessibilityElements(head *bytes.Buffer) { func (b *backend) id(prefix string) string { b.nextID++ - return prefix + strconv.Itoa(b.nextID) + return b.opts.idPrefix + prefix + strconv.Itoa(b.nextID) } // nl writes a newline in pretty mode and nothing otherwise, so the compact and diff --git a/backend/svg/svg_test.go b/backend/svg/svg_test.go index 0f729b7..3da4679 100644 --- a/backend/svg/svg_test.go +++ b/backend/svg/svg_test.go @@ -176,6 +176,59 @@ func TestClipAndTransformGroup(t *testing.T) { } } +// Two documents inlined into one page share an id namespace, and both count +// from one. Every id a document defines, and every reference to one, has to +// carry the prefix, or the second chart's cells are clipped to the first's +// panel and its aria-labelledby reads the first one's title. +func TestIDPrefixReachesEveryIdAndReference(t *testing.T) { + b, finish := open(t, svg.IDPrefix("chart2-")) + b.(ir.Semantics).Describe(ir.Description{Title: "T", Detail: "D"}) + var clip ir.Path + clip.Rect(ir.R(0, 0, 50, 50)) + b.Push(&clip, ir.Affine{A: 1, D: 1}) + b.Markers(ir.MarkerCircle, []ir.Point{{X: 1, Y: 1}}, ir.MarkerStyle{Size: 6, Fill: ir.RGB(0, 128, 0)}) + var p ir.Path + p.Rect(ir.R(0, 0, 10, 10)) + b.FillPath(&p, ir.Fill{ + Color: ir.RGB(0, 0, 0), End: ir.Point{X: 10}, + Stops: []ir.GradientStop{{Offset: 0, Color: ir.RGB(0, 0, 0)}, {Offset: 1, Color: ir.RGB(255, 255, 255)}}, + }, ir.NonZero) + b.Pop() + got := finish() + + for _, want := range []string{ + ``, `clip-path="url(#chart2-c1)"`, + ``, ``, + } { + if !strings.Contains(got, want) { + t.Errorf("missing %s in:\n%s", want, got) + } + } + for _, unprefixed := range []string{`id="c1"`, `id="m2"`, `id="g3"`, `id="figure-`, `"#c1`, `"#m2`, `(#c1`, `(#g3`, `="figure-`, `"figure-`} { + if strings.Contains(got, unprefixed) { + t.Errorf("found unprefixed %s in:\n%s", unprefixed, got) + } + } +} + +func TestABadIDPrefixIsReportedOnOpen(t *testing.T) { + for _, p := range []string{"1chart", "-x", `a"b`, "a b", "รค"} { + var buf bytes.Buffer + if _, err := svg.Writer(&buf, svg.IDPrefix(p)).Open(ir.Surface{WidthPx: 10, HeightPx: 10, DPR: 1}); err == nil { + t.Errorf("IDPrefix(%q) opened without an error", p) + } + } + for _, p := range []string{"", "a", "_x", "chart-2.", "Fig_3-"} { + var buf bytes.Buffer + if _, err := svg.Writer(&buf, svg.IDPrefix(p)).Open(ir.Surface{WidthPx: 10, HeightPx: 10, DPR: 1}); err != nil { + t.Errorf("IDPrefix(%q): %v", p, err) + } + } +} + func TestUnbalancedPushIsAnError(t *testing.T) { var buf bytes.Buffer target := svg.Writer(&buf) diff --git a/backend/svg/target.go b/backend/svg/target.go index 249a4b8..da49053 100644 --- a/backend/svg/target.go +++ b/backend/svg/target.go @@ -18,6 +18,8 @@ type options struct { fontFail error family string pretty bool + idPrefix string + idFail error } // WithFont supplies a TrueType or OpenType file whose metrics are used for @@ -47,6 +49,44 @@ func WithFontFamily(family string) Option { // smaller, and golden files are diffed by tooling rather than read. func Pretty() Option { return func(o *options) { o.pretty = true } } +// IDPrefix puts p in front of every id the document defines: its clip paths, +// marker shapes and gradients, and the and <desc> its aria-labelledby +// names. +// +// A standalone file needs none, which is why the default is no prefix and +// leaves the output byte for byte as it was. It is for several documents +// inlined into one HTML page, which share one id namespace: every chart counts +// its ids from one, so the second chart's url(#c1) resolves to the first +// chart's clip and its cells are cut to the wrong rectangle, and its +// aria-labelledby reads out the first chart's title. Give each document its own +// prefix โ€” "sales-", "chart2-" โ€” and they cannot meet. +// +// p must be usable at the front of an XML name: an ASCII letter or underscore, +// then letters, digits, '-', '_' or '.'. Anything else is reported on Open +// rather than written into an attribute it would break. +func IDPrefix(p string) Option { + return func(o *options) { + if !validPrefix(p) { + o.idFail = fmt.Errorf("id prefix %q is not the start of an XML name", p) + return + } + o.idPrefix = p + } +} + +func validPrefix(p string) bool { + for i := 0; i < len(p); i++ { + c := p[i] + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c == '_': + case i > 0 && (c >= '0' && c <= '9' || c == '-' || c == '.'): + default: + return false + } + } + return true +} + // Writer returns a Target that writes an SVG document to w. func Writer(w io.Writer, opts ...Option) ir.Target { return &target{w: nopCloser{w}, opts: build(opts)} @@ -78,6 +118,9 @@ func (t *target) Open(s ir.Surface) (ir.Backend, error) { if t.opts.fontFail != nil { return nil, fmt.Errorf("figure/backend/svg: %w", t.opts.fontFail) } + if t.opts.idFail != nil { + return nil, fmt.Errorf("figure/backend/svg: %w", t.opts.idFail) + } if t.w == nil { f, err := os.Create(t.path) if err != nil { diff --git a/geom/text.go b/geom/text.go index e9d7caa..a6144f4 100644 --- a/geom/text.go +++ b/geom/text.go @@ -61,6 +61,10 @@ import ( // A layer given [ColorBy] takes each label's ink from the fill that scale // gives the row, dark on light and light on dark, so that a qualitative // palette does not leave half its categories unreadable. [Color] overrides it. +// The layer reads only its own options, never the layer under it: labels over +// a heatmap need the heatmap's ColorBy repeated on the text layer, with the +// same scale value so the two agree and the chart keeps one colourbar โ€” +// without it every label is drawn in the theme's label ink, dark on dark cells. // // AvoidOverlap opts into the renderer's panel-local label layout. Point labels // may move; box labels remain anchored to their own box and are dropped if they