fix(layout): apply a plain inline element's own margin-left/margin-right - #124
Merged
Merged
Conversation
…ght (#124) InlineItem (the atom of inline layout) carries no margin field of any kind, and nothing in inline collection ever consulted a plain (non- replaced, non-atomic) inline element's own Style.Margin.Left/.Right -- its margin was simply never applied. Confirmed live on news.ycombinator.com: `<b class="hnname" style="margin-right:5px">Hacker News</b><a href="newest">new</a>` -- genuinely no whitespace between the two elements in the source HTML, so the margin-right is the ONLY source of the gap a real browser renders. Without it, the header read "Hacker Newsnew" instead of "Hacker News new". Fixed with a pendingMargin accumulator on the layouter: entering a plain inline element adds its margin-left, leaving one adds its margin-right, and whichever InlineItem is created next takes the accumulated value into its SpaceBefore field via a new takeMargin() method -- reusing the same field and line-layout machinery collapsible whitespace already uses rather than adding a parallel code path. Adjacent inline elements' margins correctly add (not collapse, unlike block margins). Reset alongside the whitespace-collapsing state at a genuine break (a promoted block or forced <br>): a stale margin has nothing left on the same line to apply to once one interrupts. A real, narrower limitation surfaced by the session's own regression test, documented rather than shipped over: a margin lands correctly whenever the margined element is not the very first item on its line, but layoutInline/wrapOneLine/WrapItems deliberately ignore SpaceBefore for a line's first item (so collapsible leading whitespace never creates a phantom indent) -- a margin riding in that same field is ignored for the identical reason. A dedicated non-collapsible field would be needed to survive that case too, not attempted here absent any confirmed live page needing it. Verified live: the header now renders with the correct gap. Bench is flat (0.559->0.556 SSIM) -- a single 5px header gap is a vanishing fraction of a page whose comparison region is dominated by 30 story-list rows of ordinary font-rasteriser variance, the same pattern this session's other small header/nav-only fixes have shown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
InlineItem(the atom of inline layout) carries no margin field of any kind, and nothing in inline collection ever consulted a plain (non-replaced, non-atomic) inline element's ownStyle.Margin.Left/.Right— its margin was simply never applied.Confirmed live on news.ycombinator.com:
<b class="hnname" style="margin-right:5px">Hacker News</b><a href="newest">new</a>— genuinely no whitespace between the two elements in the source HTML, so themargin-rightis the ONLY source of the gap a real browser renders. Without it, the header read "Hacker Newsnew" instead of "Hacker News new".Changes
layout/layout.go: apendingMarginaccumulator on thelayouter: entering a plain inline element adds itsmargin-left, leaving one adds itsmargin-right, and whicheverInlineItemis created next takes the accumulated value into itsSpaceBeforefield via a newtakeMargin()method — reusing the same field and line-layout machinery collapsible whitespace already uses rather than adding a parallel code path. Adjacent inline elements' margins correctly add (not collapse, unlike block margins). Reset alongside the whitespace-collapsing state at a genuine break (a promoted block or forced<br>): a stale margin has nothing left on the same line to apply to once one interrupts.layout/inlinemargin_test.go(new): four tests — margin-right, margin-left, adjacent-margins-add, and the block-break drops-pending-margin boundary case — all confirmed to fail via genuine revert-and-rerun before the fix.A documented, narrower limitation
Surfaced by the test suite itself (not shipped over): a margin lands correctly whenever the margined element is not the very first item on its line, but
layoutInline/wrapOneLine/WrapItemsdeliberately ignoreSpaceBeforefor a line's first item (so collapsible leading whitespace never creates a phantom indent) — a margin riding in that same field is ignored for the identical reason. A dedicated non-collapsible field would be needed to survive that case too, not attempted here absent any confirmed live page needing it (this engine's real, confirmed use of inline margin is always between two things already sharing a line, as on this page).Verified live
The header now renders with the correct gap ("Hacker News new" instead of "Hacker Newsnew"). Bench is flat (SSIM 0.559→0.556) — a single 5px header gap is a vanishing fraction of a page whose comparison region is dominated by 30 story-list rows of ordinary font-rasteriser variance, the same pattern this session's other small header/nav-only fixes have shown.
Test plan
go build ./...cleango test ./...— zero regressionsCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com