Repository navigation
fix(layout): <br> advances one line box instead of two - #37
leshchenko1979 wants to merge 1 commit into
Conversation
Macawls
left a comment
Macawls
left a comment
There was a problem hiding this comment.
Thanks, single <br> is fixed and the e2e test reproduces the bug on main. But this introduces a regression for consecutive breaks: since a non-trailing break contributes zero height, <br><br> no longer leaves a blank line.
Repro, font-size:20px, display:block, baseline of the last text run:
| input | main |
this branch | browser |
|---|---|---|---|
one<br>two<br>three |
115.3 | 67.3 | ~67 |
one<br><br>three |
91.3 | 43.3 | ~67 |
main was wrong in the other direction (an extra line), but <br><br> for a blank line is very common in email-style HTML, so this one is more visible. A break that is directly followed by another break (or is the last child, as you already handle) needs to contribute a line box of its own. Could you fix that and add the <br><br> case to ogre_br_e2e_test.go?
…lank line
A <br> was converted to a text node holding a newline, which the wrapper
turned into a space under white-space:normal and the layout then treated as
an ordinary text leaf. That leaf took a full line box of its own on top of
the line box of the text that followed, so every break advanced the baseline
by two lines (48px at font-size 20, line-height 1.15) instead of one.
Mark the node as a line break and lay it out as a zero-size leaf, except
where it opens a line box of its own. A break opens one exactly when the
next rendered sibling is another break: the run's last break is followed by
the text that supplies the line it starts, and a trailing break opens nothing
at all, because the browser drops the empty line box at the end of a block.
A run of N breaks therefore advances N-1 empty lines plus the line of
whatever follows.
Measured against Chromium 151, a following block lands at 23/23/46/69px
after none/one/two/three trailing breaks, and at 69/92px after two and three
breaks between two lines. Ogre's line count matches in every case; the
remaining 1px is the pre-existing line-height rounding, not this defect.
The red tests, before the fix:
--- FAIL: TestRenderConsecutiveBrLeavesBlankLine
two consecutive <br> advance 24 px, want 48 (two line boxes of 24):
single=[24 48 72] double=[24 48 72]
--- FAIL: TestRenderTrailingBrOpensNoExtraLine
1 trailing <br> push the sibling 24 px down, want 0 (0 line boxes of 24)
Fixes Macawls#35
Refs Macawls#37
eddfe95 to
f2fee21
Compare
|
You were right, and the case you named was the whole of it: a single Fixed at the rule rather than the symptom. A break opens a line box exactly when the next rendered sibling is another break — the run's last break is followed by the text that supplies the line it starts. A run of N breaks therefore advances N-1 empty lines plus the line of whatever follows. That rule also exposed the inverse bug in my previous commit: a trailing break kept a full line box, which the browser does not do (it drops the empty line box at the end of a block). Both are fixed by the same condition. Measured against Chromium 151, with a following block so the box height is observable:
The remaining 1px is the pre-existing line-height rounding (1.15 × 20 = 23 in CSS, 24 in the engine), not this defect. Tests, both red before the fix: Branch rebuilt on current |
…lank line
A <br> was converted to a text node holding a newline, which the wrapper
turned into a space under white-space:normal and the layout then treated as
an ordinary text leaf. That leaf took a full line box of its own on top of
the line box of the text that followed, so every break advanced the baseline
by two lines (48px at font-size 20, line-height 1.15) instead of one.
Mark the node as a line break and lay it out as a zero-size leaf, except
where it opens a line box of its own. A break opens one exactly when the
next rendered sibling is another break: the run's last break is followed by
the text that supplies the line it starts, and a trailing break opens nothing
at all, because the browser drops the empty line box at the end of a block.
A run of N breaks therefore advances N-1 empty lines plus the line of
whatever follows.
Measured against Chromium 151, a following block lands at 23/23/46/69px
after none/one/two/three trailing breaks, and at 69/92px after two and three
breaks between two lines. Ogre's line count matches in every case; the
remaining 1px is the pre-existing line-height rounding, not this defect.
The red tests, before the fix:
--- FAIL: TestRenderConsecutiveBrLeavesBlankLine
two consecutive <br> advance 24 px, want 48 (two line boxes of 24):
single=[24 48 72] double=[24 48 72]
--- FAIL: TestRenderTrailingBrOpensNoExtraLine
1 trailing <br> push the sibling 24 px down, want 0 (0 line boxes of 24)
Fixes Macawls#35
Refs Macawls#37
Fixes #35
Problem
<br>did not break a line at all in the sense that matters: the break was converted to a text node holding"\n", the wrapper collapsed that newline to a space underwhite-space: normal, and the layout then laid the break out as an ordinary text leaf. That leaf took a full line box of its own on top of the line box already occupied by the text that followed it, so every break advanced the baseline by two lines instead of one.Measured on case
19-br-vs-block(font-size 20, line-height 1.15), ink band tops in pixels:Ink bounding box for the same case went from
(40,44,107,159)to(40,44,107,111)against Chromium(39,44,114,107)— the block no longer runs 52px past the last line.Fix
parse/node.go: newLineBreak boolonNode, marking a text node produced by<br>. The distinction matters because source whitespace is trimmed, so a newline insideTextcan only mean a break.parse/parse.go:<br>setsLineBreak: true.layout/builder.go:buildBreakNodelays the break out as a zero-size leaf. It contributes no height, because the line box it starts comes from the next text leaf — except when it is the last child, where a trailing break legitimately opens one more empty line, exactly as a browser does.Test
ogre_br_e2e_test.go— renders three lines separated by<br>and the same three lines separated bydisplay:blockspans, then compares the ink-band pitch of the two. The block version is the control: it already advanced one line box per line, so the<br>version must match it.Fails on unpatched code:
Passes after the fix.
go vet ./...clean,go test ./...all packages ok.On the harness number
19-br-vs-blockstays at 0.26 % diff before and after, and that is not a failure of this fix: the metric counts differing pixels, and the residual is dominated by vertical ink offset and Ogre's slightly taller glyphs, not by line pitch. The band table above is the measurement that actually moves.04-line-heightlikewise stays at 0.51 % for the same reason.Not implemented
<br>at the very end of a block opens an empty line; this matches Chromium but adds height to the container, which is correct and intentionally not clamped.<br>insidewhite-space: prestill goes through the ordinary newline path; that path was already correct and is untouched.