amadan
Hold the three block margins to the stylesheet, not just the leading
Review finding, and the gap is one I reported and did not close.
TestPageGeometryMatchesTheStylesheet reads eight numbers out of
show.html and none of them is a margin, so paragraphSpaceBelow,
listSpaceBelow and listItemSpace were free to drift from the rules they
restate — while pdf.go's own header calls itself a mirror of that
stylesheet.
This branch is what makes it bite. All three are now load-bearing
zeroes, so the plausible next edit — putting the paragraph spacing back
with `margin-block: 0 1em`, reverting the density change — would move
the page and leave the PDF where it was. Only the line-height half of
that drift was caught; the margins would have gone through silently, and
the export would quietly stop being the same document as the screen.
Three cases close it, in the table's own must-find-its-pattern style so
a restructure fails loudly instead of matching nothing and passing:
- the paragraph's margin-block against paragraphSpaceBelow,
- the ul/ol margin-block against listSpaceBelow,
- and the absence of a `[docs-text] li` margin rule against
listItemSpace, which has no rule to read because "an li has no
margin" is spelled by having none (a browser-default li has none, so
an explicit 0 would be a no-op).
Two things that would otherwise have made these vacuous. blockEndEm
reads the END value of the margin-block shorthand rather than the first
number in it: a pattern capturing the first would read "0" out of
"margin-block: 0 1em" and pass while the stylesheet said the opposite of
pdf.go. And the li case pins `[docs-text] li > :last-child` as a
landmark before asserting an absence, because an absence alone holds
just as well against an empty file; it also checks in whichever
direction the constant points, so restoring the margin on both sides
stays possible and restoring it on one does not.
export.BlockSpace returns (value, ok) rather than following SizeScale's
bare-zero convention. SizeScale can report an unknown mark as 0 because
no real multiplier is 0; every one of these margins IS 0, so a
misspelled name returning a bare 0 would compare equal to the right
answer and report nothing while checking nothing.
Each case was watched failing against the drift it guards, one at a
time, by editing show.html and reverting:
paragraph bottom margin: the stylesheet says 1em, internal/export says 0em
list bottom margin: the stylesheet says 1em, internal/export says 0em
list item margin: internal/export says an li has none, the stylesheet
gives it one: "[docs-text] li {\n margin-block-end: 0.35em;\n }"
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+124
−0
internal/docs/export_test.go+101 −0internal/export/pdf.go+23 −0
CI passed — run details
Intent
Attributed to this commit: 40.2 g CO₂e (5336029 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-typography, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/internal/docs/export_test.go b/internal/docs/export_test.go| index 69252cc..5a5dfb5 100644 |
| --- a/internal/docs/export_test.go |
| +++ b/internal/docs/export_test.go |
| @@ -229,6 +229,107 @@ func TestPageGeometryMatchesTheStylesheet(t *testing.T) { |
| t.Errorf("%s: the stylesheet says %v, internal/export says %v", c.what, got, c.want) |
| } |
| } |
| + |
| + // The block-level margins, which the table above cannot see. |
| + // |
| + // internal/export/pdf.go states these three as constants and its |
| + // own header calls them a mirror of this stylesheet, but until now |
| + // nothing held any of them to it — the table reads eight numbers |
| + // and none of them is a margin. That gap bites precisely because |
| + // the three are now load-bearing ZEROES: a later edit putting the |
| + // paragraph spacing back (`margin-block: 0 1em`, a plausible |
| + // "revert the density change") would move the page and leave the |
| + // PDF where it was, and only the line-height half of that drift |
| + // would have been caught. |
| + // |
| + // margin-block is a one-or-two-value shorthand and the number that |
| + // matters is the END margin, so these read the whole value list and |
| + // take its last field rather than capturing the first number they |
| + // see — a pattern that captured the first would read "0" out of |
| + // "margin-block: 0 1em" and pass while the stylesheet said the |
| + // opposite of what pdf.go says. |
| + for _, c := range []struct { |
| + what string |
| + pattern string |
| + constant string |
| + }{ |
| + {"paragraph bottom margin", `\[docs-text\] p \{[^}]*margin-block:\s*([^;}]+)`, "paragraph"}, |
| + {"list bottom margin", `\[docs-text\] ul,\s*\[docs-text\] ol \{[^}]*margin-block:\s*([^;}]+)`, "list"}, |
| + } { |
| + want, ok := export.BlockSpace(c.constant) |
| + if !ok { |
| + t.Errorf("%s: internal/export has no %q margin — it was renamed and this case is no longer comparing anything", c.what, c.constant) |
| + continue |
| + } |
| + m := regexp.MustCompile(c.pattern).FindStringSubmatch(sheet) |
| + if m == nil { |
| + t.Errorf("%s: found nothing matching %s in views/show.html — the stylesheet was restructured and this test can no longer see what it is checking", c.what, c.pattern) |
| + continue |
| + } |
| + got, err := blockEndEm(m[1]) |
| + if err != nil { |
| + t.Errorf("%s: %v", c.what, err) |
| + continue |
| + } |
| + if got != want { |
| + t.Errorf("%s: the stylesheet says %vem, internal/export says %vem", c.what, got, want) |
| + } |
| + } |
| + |
| + // listItemSpace has no rule of its own to read: the stylesheet says |
| + // "an li has no margin" by having no `[docs-text] li` margin rule at |
| + // all, because a browser-default li has none and an explicit 0 would |
| + // be a no-op rule. |
| + // |
| + // An absence is a weak thing to assert alone — it would hold just as |
| + // well against an empty file — so this pins a landmark first. If the |
| + // `[docs-text] li > :last-child` rule is gone, the list rules were |
| + // restructured and the absence below has stopped meaning anything, |
| + // which is a failure rather than a pass. And it checks in whichever |
| + // direction the constant points, so putting the margin back on both |
| + // sides stays possible and putting it back on one does not. |
| + itemSpace, ok := export.BlockSpace("list_item") |
| + if !ok { |
| + t.Error(`internal/export has no "list_item" margin — it was renamed and this case is no longer comparing anything`) |
| + } else if !strings.Contains(sheet, "[docs-text] li > :last-child {") { |
| + t.Error("list item margin: views/show.html no longer has a `[docs-text] li > :last-child` rule — the list rules were restructured and this check can no longer see what it is checking") |
| + } else { |
| + liMargin := regexp.MustCompile(`\[docs-text\] li \{[^}]*margin[^}]*\}`).FindString(sheet) |
| + switch { |
| + case itemSpace == 0 && liMargin != "": |
| + t.Errorf("list item margin: internal/export says an li has none, the stylesheet gives it one: %q", liMargin) |
| + case itemSpace != 0 && liMargin == "": |
| + t.Errorf("list item margin: internal/export says %vem, the stylesheet gives an li no margin rule at all", itemSpace) |
| + } |
| + } |
| +} |
| + |
| +// blockEndEm reads the block-END margin out of a `margin-block` |
| +// shorthand, in em: one value sets both edges, two set start then end. |
| +// |
| +// A unit other than em is an error rather than a guess. internal/export |
| +// states these margins in em of the body size, so a value in px or rem |
| +// is not something this comparison can honestly turn into a number — |
| +// and saying so loudly is the point, because the alternative is a test |
| +// that quietly compares the wrong quantity. |
| +func blockEndEm(value string) (float64, error) { |
| + fields := strings.Fields(strings.TrimSpace(value)) |
| + if len(fields) == 0 || len(fields) > 2 { |
| + return 0, fmt.Errorf("margin-block: %q is not one or two values", value) |
| + } |
| + end := fields[len(fields)-1] |
| + if end == "0" { |
| + return 0, nil |
| + } |
| + num, found := strings.CutSuffix(end, "em") |
| + if !found { |
| + return 0, fmt.Errorf("margin-block ends in %q — internal/export states these margins in em, so this cannot be compared", end) |
| + } |
| + var f float64 |
| + if _, err := fmt.Sscanf(num, "%g", &f); err != nil { |
| + return 0, fmt.Errorf("margin-block ends in %q, which is not a number", end) |
| + } |
| + return f, nil |
| } |
| // The toolbar's option values are a vocabulary shared by three files: |
diff --git a/internal/export/pdf.go b/internal/export/pdf.go| index b80b0e5..e5b56a2 100644 |
| --- a/internal/export/pdf.go |
| +++ b/internal/export/pdf.go |
| @@ -89,6 +89,29 @@ var sizeOrder = []string{"size_sm", "size_lg", "size_xl"} |
| // rather than quietly comparing nothing. |
| func SizeScale(mark string) float64 { return sizeScale[mark] } |
| +// BlockSpace exposes one block-level bottom margin, in em, for the same |
| +// test and the same reason: these three numbers restate show.html's |
| +// stylesheet and nothing but a test can hold them to it. |
| +// |
| +// Two return values, unlike SizeScale above, and that is not |
| +// symmetry-breaking for its own sake. SizeScale can report an unknown |
| +// mark as 0 because 0 is not a multiplier any real mark has, so the |
| +// wrong answer is conspicuous. Every one of these three margins IS |
| +// zero today, so a misspelled name returning a bare 0 would compare |
| +// equal to the right answer and the test would report nothing while |
| +// checking nothing. |
| +func BlockSpace(name string) (float64, bool) { |
| + switch name { |
| + case "paragraph": |
| + return paragraphSpaceBelow, true |
| + case "list": |
| + return listSpaceBelow, true |
| + case "list_item": |
| + return listItemSpace, true |
| + } |
| + return 0, false |
| +} |
| + |
| // Report is what an export could not do faithfully — §11.2's |
| // degrade-loudly rule, in the one form this exporter can currently |
| // fail: a character none of the core faces has a glyph for. A document |