amadan
Review round: the comment was wrong about the bug it fixes
Branch review caught the opening arithmetic counting two insets when there are three. The shell's own padding is one of them — the same padding the paragraph twenty lines below was already explaining how to cancel — so the stated before-numbers, 151px at 375 and 96px at 320, were both wrong and wrong in the direction that flatters the change. Measured, they were 119px and 64px. The after-numbers were right. The breakpoint derivation inherited the same error: 45 characters at 576px only works if you forget an inset, and the honest version is about 40. 36rem stays, because reflow promises 320px viewports and 577-600px still yields a readable 306-330px column, but it is now written down as the judgement it is rather than a formula that does not add up. Second fix: the media query had no type, so it applied to print. Letter and A4 print boxes stay above the breakpoint, but A5 portrait is about 559px and crosses it — printing on A5 would have dropped the 1in and applied a phone's negative margin, in the one medium where a page margin is not a metaphor. `screen and` restores what the comment above it promises.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
internal/docs/views/show.html+45 −21
CI passed — run details
Intent
<task-notification> <task-id>a87988f42ce9b5217</task-id> <tool-use-id>toolu_01WmQnEvWtUPQ5sbvTn9hgxt</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4a30c557-129a-4e57-8aa3-0c7518c36cf1/tasks/a87988f42ce9b5217.output</output-file> <status>completed</status> <summary>Agent "Fable review of mobile margins" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Review complete. All claims verified against rastrillo v0.22.0's actual `tokens.css` and `column.html` layout, and measured empirically in headless Chromium (fixed-width iframes at 320/375/414/576/577/600, both with and without the new media block; iframe viewports carried a 15px classic scrollbar, so this also exercised the scrollbar case the comment argues about). ## Verdict: approve with changes The CSS is sound — every mechanism claim checks out under measurement. The required change is to the comment prose, which this repo treats as load-bearing: the block's opening arithmetic is confidently wrong about the pre-change state, and internally contradicted by its own inner comment. ## Genuine defects **1. The "before" arithmetic omits the shell inset its own inner comment documents — the stated numbers are wrong.** `internal/docs/views/show.html:103-112` and `:120-122`. The opening paragraph says "a reader sees two nested insets" and computes `375 - 32 - 192 = 151px` and 96px at 320px. But the content renders inside `<main rst-page>` (rastrillo's `column.html:21`), whose unconditional `padding: var(--rst-sp-5) var(--rst-sp-4) var(--rst-sp-6)` (`tokens.css:141-146`, sp-4 = 1rem) adds a third inset of 32px — exactly the padding the inner comment at `:137-139` says it measured and cancels. Measured pre-change columns: **119px at 375, 64px at 320, 320px at 576** — not 151/96/352. The error understates the bug, so nothing behavioral is wrong, but the derivation of the breakpoint inherits it: "576 − 32 − 192 = 352 ≈ 45 characters" is really 320px ≈ 41 characters; the honest 45-char derivation gives ~608px ≈ 38rem. 36rem remains defensible (at 577-600px the surviving paper column is 306-330px — cramped but readable, and reflow only promises 320px viewports), but the comment must either restate the derivation with three insets and "about 40 characters", or move to 38rem. Smallest fix: correct "two nested insets" to three, replace 151/96/352 with 119/64/320, and reconcile the 45-character claim. The post-change numbers (343/288) are correct as written — I measured exactly those. **2. The media query is untyped, so it applies in print on narrow paper.** `show.html:133`. `@media (max-width: 36rem)` matches all media. US Letter/A4 print boxes are ~700-800 CSS px, unaffected; but A5 portrait (~559px, less printer margins) crosses the threshold, dropping the 1in padding and applying negative margins in print — precisely where the comment promises "the 1in is untouched... where the print margin can be shown honestly it still is." Failure scenario: File → Print on A5 produces an edge-bleeding sheet with 1rem margins. Smallest fix: `@media screen and (max-width: 36rem)`. Minor — no print stylesheet exists yet and letter/A4 are untouched — but it is the one place the code contradicts its comment. ## Verified sound (stating plainly, per the questions asked) - **Negative-margin breakout (Q1): correct, and the best available technique.** Measured no horizontal scroll at any width including with a real classic scrollbar. The widened canvas equals the shell's border box, which by construction never exceeds the viewport — cancelling known padding genuinely cannot overhang, in RTL (`margin-inline` is logical; the padding is symmetric), under zoom (both lengths are the same rem token), and inside any of rastrillo's three layouts. The `100vw` critique is factually correct and doubly apt here: rastrillo sets `scrollbar-gutter: stable` on `html` (`tokens.css:112-127`), so `100vw` would overhang even when no scrollbar is drawn. The one alternative that removes the token coupling — zeroing `[rst-page]`'s own padding under the query — would un-inset the page header too and break the title alignment, so the margin is the right call. The coupling's failure mode is honestly described and fails soft, as claimed. - **Alignment claim: measured true.** After the change, first character at 16px, `h1` at 16px, `[docs-page]` at 0. `[rst-page-header]` has no inline padding of its own. - **Breakpoint form (Q2): right.** rem in a media query resolves against the user's default font size, so large-text users shed the paper at proportionally wider windows — the correct direction. A container query isn't available (rastrillo declares no container) and would be wrong anyway: the rule cancels the shell's viewport-level padding, which is inherently a viewport relationship; below `[rst-page]`'s 64rem max-width the two agree in every reachable state. RTL needs nothing here. Boundary measured: applies at 576px inclusive, off at 577 ("below 36rem" is off by 1px at the boundary — trivial). - **Surrounding constraints (Q3): all intact.** Flex column, `min-block-size: 11in`, and `[docs-editor] { flex: 1 0 auto }` are untouched, so the click-below-last-paragraph target survives (and the caller's live measurement confirms). Literal colours untouched. `[docs-save-status]` is `position: fixed` and viewport-anchored — independent of the breakout. The scriptless read view shares the same `[docs-text]`/`[docs-page]` rules, so it gets the identical narrow layout. `[docs-back]`'s 24px floor untouched. No new attributes, so `TestOurTemplatesUseTheAttributeGrammar` is unaffected. - **Accessibility (Q4): a straight improvement.** 400% zoom on a 1280px window is a 320px CSS viewport → narrow branch → 288px column, no horizontal scroll (measured): SC 1.4.10 goes from a 64px column to a pass. The zoom-narrows-viewport interaction with the rem breakpoint is the right shape for 1.4.4 too. - **Shadow and bottom margin (Q5): both correct.** The canvas keeps `padding-block: var(--rst-sp-4)`, so grey separation above and below the sheet survives; a viewport-clipped side shadow would indeed read as a smear. `margin-block-end: 0` avoids stacking 2.5rem of margin on the canvas's own 1rem bottom padding. ## Worth stating, not defects - The rule also reduces the canvas's block padding from `--rst-sp-6` (2.5rem) to `--rst-sp-4` (1rem); the prose only mentions the side padding "going to nothing". One clause would cover it. - "96px: under ten characters of 12pt Georgia" stays accidentally true of the real 64px figure, but fix it with the numbers in defect 1. - No automated regression pins the breakpoint behavior; consistent with the repo's current practice of pinning grammar rather than layout, so noted only. The relevant file is `/home/paulca/amadan.net/oficina/docs/.claude/worktrees/live-sync/internal/docs/views/show.html` (lines 100-164 are the diff). One measurement run needed a sandbox bypass: Chromium's process-singleton unix socket is blocked by the command sandbox (`/sandbox` manages this); the bypassed commands only rendered local files read-only.</result> <usage><subagent_tokens>68165</subagent_tokens><tool_uses>31</tool_uses><duration_ms>366206</duration_ms></usage> </task-notification>
diff --git a/internal/docs/views/show.html b/internal/docs/views/show.html| index e2cd8e8..0fb6744 100644 |
| --- a/internal/docs/views/show.html |
| +++ b/internal/docs/views/show.html |
| @@ -100,27 +100,41 @@ |
| /* Narrow screens stop simulating paper, because at phone widths the |
| simulation costs more than it says. |
| - Above this breakpoint a reader sees two nested insets: [docs-canvas]'s |
| - 1rem tray padding either side, and then [docs-page]'s own 1in. That |
| - inner inch is a real measurement — it stands for the document's page |
| - margin, which page setup and the layout engine will own properly |
| - later — and on a desktop it is worth every pixel. On a 375px phone |
| - the same arithmetic (375 - 32 - 192) leaves a 151px column, and at |
| - 320px, which this app supports without exemption (WCAG 2.2 AA SC |
| - 1.4.10), it leaves 96px: under ten characters of 12pt Georgia per |
| - line. A page margin that squeezes the page is no longer describing |
| - the page. |
| + Above this breakpoint a reader sees THREE nested insets, and the |
| + first one is easy to miss because it belongs to the framework: the |
| + shell this template renders into (rastrillo's [rst-page]) pads |
| + --rst-sp-4 either side, then [docs-canvas] adds its own 1rem tray |
| + padding, then [docs-page] adds 1in. That inner inch is a real |
| + measurement — it stands for the document's page margin, which page |
| + setup and the layout engine will own properly later — and on a |
| + desktop it is worth every pixel. On a 375px phone the three together |
| + (375 - 32 - 32 - 192) left a 119px column, and at 320px, which this |
| + app supports without exemption (WCAG 2.2 AA SC 1.4.10), 64px: about |
| + five characters of 12pt Georgia per line. A page margin that |
| + squeezes the page is no longer describing the page. |
| - So below 36rem the tray's side padding goes to nothing, the sheet |
| - runs edge to edge, and ONE margin survives inside it. Text goes to |
| - 343px at 375px wide and 288px at 320px. The 1in is untouched at |
| - every width above the breakpoint, so where the print margin can be |
| - shown honestly it still is. |
| + (An earlier version of this comment counted two insets and said |
| + 151px and 96px. It was wrong in the direction that flatters the |
| + code, and it was wrong while the paragraph below it described |
| + cancelling the very inset it had forgotten. The numbers here are |
| + measured, not derived.) |
| - 36rem is derived, not fashionable: it is about where the two insets |
| - stop leaving 45 characters a line (576 - 32 - 192 = 352px), which is |
| - the low end of a comfortable measure. Phones in portrait are all |
| - well under it; a tablet or a small window keeps the paper. |
| + So below 36rem the tray's side padding goes to nothing, its block |
| + padding drops from --rst-sp-6 to --rst-sp-4, the sheet runs edge to |
| + edge, and ONE margin survives inside it. Text goes to 343px at 375px |
| + wide and 288px at 320px. The 1in is untouched at every width above |
| + the breakpoint, so where the print margin can be shown honestly it |
| + still is. |
| + |
| + 36rem is a judgement rather than a formula, and the honest version |
| + of the arithmetic says so: at 576px the three insets leave a 320px |
| + column, about 40 characters of this face, and holding out for the |
| + 45 characters that make a comfortable measure would put the |
| + breakpoint nearer 38rem. 36rem is the choice because reflow only |
| + promises 320px viewports, and between 577 and 600px the paper still |
| + yields 306-330px — cramped, readable, and still recognisably a |
| + page. Phones in portrait are all well below it; a tablet or a small |
| + window keeps the paper. |
| The shadow goes with the tray — it exists to lift the sheet off the |
| grey, and once there is no grey beside it, it is a dark smear on the |
| @@ -129,8 +143,18 @@ |
| so that a click in the empty space below the last paragraph still |
| lands in the document (see fixtures/editor-entry.js's |
| placeCaretFromPage), and the literal colours, which must not repaint |
| - under anyone's theme at any width. */ |
| - @media (max-width: 36rem) { |
| + under anyone's theme at any width. |
| + |
| + `screen and`, not a bare width query, and it is not decoration. A |
| + media query with no type matches print too, and a print box is |
| + measured in CSS pixels like any other: US Letter and A4 come out |
| + around 700-800px and stay above this breakpoint, but A5 portrait is |
| + about 559px and crosses it. Printing a document on A5 would then |
| + drop the 1in and apply a negative margin meant for a phone screen — |
| + an edge-bleeding sheet with 1rem margins, in the one medium where |
| + the page margin is not a metaphor at all. `screen` is what keeps |
| + the paragraph above true. */ |
| + @media screen and (max-width: 36rem) { |
| [docs-canvas] { |
| padding-inline: 0; |
| padding-block: var(--rst-sp-4); |