Skip to content

Commit 6c67f40

Browse files
committed
Fix InlineFragmentation's widows/orphans merge-back to shift, not just merge
Confirmed while porting OrphansWidowsIntegrationTests.cs (Batch 2, commit d8b2587): ApplyLineBreaking's widows correction could only fully merge two runs into one (breaks.RemoveAt), never shift the break point earlier by a partial number of lines, and never fell back to pushing a run whole onto a FRESH page when an in-place merge didn't fit the current page's remaining room. A 4-line paragraph with widows:2, naturally breaking 3-before/1-after, needed only a 1-line shift (2-before/2-after) to satisfy widows - the old code tried merging all 4 lines onto the current page, found they didn't fit, and gave up, leaving the violation. Rewrote the widows loop to try, in order: (1) shift the break earlier by the minimum number of lines that satisfies widows without leaving the earlier run below its own orphans minimum - always valid at a full page height, since a shifted-but-still-separate run starts fresh at a page top regardless of exactly where the break falls; (2) if no shift works, cascade the existing full-merge (now against a full page height for any run but run 0, same as before); (3) for a merge specifically into run 0, if it doesn't fit run 0's own natural (tighter) capacity but WOULD fit a full page, apply the same fresh-page boost firstRunMovedToFreshPage already gives orphans on the first run. Only when none of these work does it decline gracefully, per css-break-3 4.3. Also fixes a related gap in firstRunMovedToFreshPage: it never checked whether the run was already flush at a fresh page's own top before pushing it to ANOTHER one - for an unsatisfiable orphans minimum after a forced break, this silently walked the box one page past where the break named, leaving that page blank. Added an alreadyAtFreshPageTop guard, applied consistently to both the pre-loop check and phase 1's own runStart==0 back-off branch (missing it from the latter initially caused a real regression in Orphans2_NothingAboveItInTheFragmentainer_IsLeftWhereItIs, caught by running the full suite before committing). Real discovery made while verifying empirically (per this project's own established practice of testing hand-traced fixes against actual runtime behavior): OrphansWidowsIntegrationTests.cs's <br>-joined Paragraph() helper never exercised this code at all. This fork's <br> handling (DomParser.CorrectLineBreaksBlocks) only folds a <br> into a real forced- newline when it's the LAST thing in its inline run; a <br> with more content after it is left as a literal box, and the surrounding text gets split into SEPARATE anonymous BLOCK siblings (confirmed by direct box-tree inspection: 7 children, alternating 1-line blocks and inline <br> boxes, the <p> itself with zero LineBoxes of its own) - so each "line" was really an independent 1-line sibling block, never touching ApplyLineBreaking's multi-line logic. Rewrote Paragraph() to use natural word-wrapping (narrow width, space- separated distinct words) instead, which produces a real multi-line box and actually exercises the fix. Un-ignores 6 tests in OrphansWidowsIntegrationTests.cs (Widows2/3/4, both Orphans2 first-run variants, Widows2 on a second-page paragraph) and 1 in FragmentainerCursorIntegrationTests.cs (AForcedBreak_LandsOnThePageItNames). Full suite: HtmlRenderer.Test 2427 passed/128 skipped/0 failed (unchanged), HtmlRenderer.IntegrationTest 330 passed/94 skipped/0 failed (was 323/101), HtmlRenderer.PdfSharp.Test 39 passed/0 failed (unchanged).
1 parent 42fff5b commit 6c67f40

3 files changed

Lines changed: 101 additions & 51 deletions

File tree

‎Source/HtmlRenderer/Core/Fragmentation/InlineFragmentation.cs‎

Lines changed: 84 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,13 @@ internal static void ApplyLineBreaking(CssBox blockBox)
9292
// fixes it without needing a special case in the main loop. Subsumes that single-line case
9393
// too - it is just the `orphans` violation that can never be waived (0 lines fitting is
9494
// always fewer than any orphans value of at least 1).
95-
var firstRunMovedToFreshPage = firstRunLineCount < lines.Count && firstRunLineCount < orphans;
95+
// A forced break (or any other placement) may already have put this run flush at a fresh
96+
// page's own top - in which case its capacity IS a full page height already, and pushing it
97+
// to yet ANOTHER fresh page cannot gain any more room (same content, same capacity, same
98+
// unsatisfiable result), it would just leave the page it was actually placed on blank. Only
99+
// worth doing when there is real room being left on the table by staying put.
100+
var alreadyAtFreshPageTop = Math.Abs(lines[0].LineTop - container.PageTopOf(firstPageIndex)) < 0.01;
101+
var firstRunMovedToFreshPage = !alreadyAtFreshPageTop && firstRunLineCount < lines.Count && firstRunLineCount < orphans;
96102
if (firstRunMovedToFreshPage)
97103
{
98104
firstPageIndex++;
@@ -117,26 +123,93 @@ internal static void ApplyLineBreaking(CssBox blockBox)
117123
breaks.RemoveAt(breaks.Count - 1);
118124
i--;
119125
}
126+
else if (linesBefore > 0 && linesBefore < orphans && runStart == 0 && !firstRunMovedToFreshPage && !alreadyAtFreshPageTop)
127+
{
128+
// Same violation as above, but there is no earlier run to merge into - runStart==0
129+
// IS the first run. The only fix here is exactly what the pre-loop check above already
130+
// does for the common case: push it whole to a fresh page - gated by the SAME
131+
// alreadyAtFreshPageTop condition that check uses, for the same reason: if this run is
132+
// already sitting at a fresh page's own top, it already got the full pageHeight
133+
// capacity and still could not fit `orphans` lines (the pre-loop check's own
134+
// firstRunLineCount<orphans, computed against that same capacity, agrees) - genuinely
135+
// unsatisfiable, not something a SECOND fresh page could help with either.
136+
firstRunMovedToFreshPage = true;
137+
firstPageIndex++;
138+
firstRunCapacity = pageHeight;
139+
i = 0; // restart the scan from line 1 under the widened capacity
140+
}
120141
else
121142
{
122143
breaks.Add(i);
123144
}
124145
}
125146

126-
// Widows: the run after the LAST break must have at least `widows` lines - if not, merge
127-
// break points backward (as many as needed) until it does, or until only one run is left, or
128-
// until merging further would make the run taller than a page can hold - honoring widows by
129-
// creating a run that can never fit isn't honoring it, it's trading one violation for a worse
130-
// one, so this is where the relaxation gives up rather than forcing it (css-break-3 §4.3's
131-
// own "some constraints can't always be satisfied" philosophy).
147+
// Widows: the run after the LAST break must have at least `widows` lines - if not, first try
148+
// shifting the break point EARLIER by as few lines as it takes (css-break-3 §5.4 asks for the
149+
// minimum number of lines moved across the break, not for the whole preceding run to be
150+
// absorbed) - as long as the shrinking earlier run still keeps at least `orphans` lines of its
151+
// own. A shifted-but-still-separate run always lands at a fresh page's own top regardless of
152+
// where exactly the break falls (phase 2 places every run but the first at
153+
// PageTopOf(firstPageIndex + its own ordinal), which does not depend on how many lines it
154+
// holds) - so its own capacity is always a full page height, never the tighter capacity a
155+
// merge into run 0 might face.
156+
//
157+
// If no such shift satisfies widows without violating orphans on the earlier run, fall back to
158+
// merging the whole earlier run away entirely (as many times as it takes, cascading further
159+
// back across more than one earlier break when needed) - first against that run's own current
160+
// capacity (a full page height, unless it is run 0, still sitting at whatever tighter room its
161+
// natural position left it), and if that specific case (merging into run 0) does not fit
162+
// there, against a full page height instead (mirroring firstRunMovedToFreshPage above: run 0
163+
// pushed whole to a fresh page gains the same capacity boost a later run already has for free).
164+
//
165+
// Only when neither a shift nor a merge - at any capacity - can satisfy widows does this give
166+
// up and leave the violation in place: honoring widows by creating a run that can never fit
167+
// isn't honoring it, it's trading one violation for a worse one (css-break-3 §4.3's own "some
168+
// constraints can't always be satisfied" philosophy).
132169
while (breaks.Count > 1 && lines.Count - breaks[breaks.Count - 1] < widows)
133170
{
134-
var candidateStart = breaks[breaks.Count - 2];
135-
var candidateCapacity = candidateStart == 0 ? firstRunCapacity : pageHeight;
136-
if (lines[lines.Count - 1].LineBottom - lines[candidateStart].LineTop > candidateCapacity)
171+
var prevRunStart = breaks[breaks.Count - 2];
172+
173+
var shifted = false;
174+
for (var newBreak = breaks[breaks.Count - 1] - 1; newBreak > prevRunStart; newBreak--)
175+
{
176+
if (newBreak - prevRunStart < orphans)
177+
break; // shifting further would strand the earlier run below its own orphans minimum
178+
179+
if (lines.Count - newBreak < widows)
180+
continue; // this candidate still does not have enough lines after it
181+
182+
if (lines[lines.Count - 1].LineBottom - lines[newBreak].LineTop > pageHeight)
183+
continue; // the (now larger) last run would not fit a fresh page either
184+
185+
breaks[breaks.Count - 1] = newBreak;
186+
shifted = true;
187+
break;
188+
}
189+
190+
if (shifted)
137191
break;
138192

139-
breaks.RemoveAt(breaks.Count - 1);
193+
var mergedHeight = lines[lines.Count - 1].LineBottom - lines[prevRunStart].LineTop;
194+
if (mergedHeight <= (prevRunStart == 0 ? firstRunCapacity : pageHeight))
195+
{
196+
breaks.RemoveAt(breaks.Count - 1);
197+
continue; // re-test widows against the now one-level-earlier run (cascades further back)
198+
}
199+
200+
if (prevRunStart == 0 && !firstRunMovedToFreshPage && mergedHeight <= pageHeight)
201+
{
202+
// Merging everything back into run 0 does not fit run 0's own natural (tighter) room,
203+
// but WOULD fit a full page - exactly the boost firstRunMovedToFreshPage already gives
204+
// the first run for orphans. Apply it here too and finish the merge.
205+
firstRunMovedToFreshPage = true;
206+
firstPageIndex++;
207+
firstRunCapacity = pageHeight;
208+
breaks.RemoveAt(breaks.Count - 1);
209+
continue;
210+
}
211+
212+
break; // neither a shift nor any merge can satisfy widows - decline gracefully
140213
}
141214

142215
// Phase 2: apply the decided breaks as cumulative shifts, in one forward pass. The first

‎Source/Test/HtmlRenderer.IntegrationTest/Fragmentation/FragmentainerCursorIntegrationTests.cs‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,6 @@ private static CssBox FindById(CssBox root, string id) =>
7474
// is already at the top of a fresh page - moving it again (as if there were a whole page of content
7575
// above it) would blank the page the forced break named.
7676
[TestMethod]
77-
[Ignore("Confirmed gap (this is the one case in this file where the cursor concept PeachPDF's own test "
78-
+ "targets really does have a counterpart bug here, just via a different mechanism): "
79-
+ "InlineFragmentation.ApplyLineBreaking's firstRunMovedToFreshPage check "
80-
+ "('firstRunLineCount < lines.Count && firstRunLineCount < orphans') never asks whether "
81-
+ "lines[0] is already flush at a fresh page's own top before deciding to push the run forward - "
82-
+ "for an unsatisfiable orphans minimum (firstRunLineCount permanently 0 or otherwise < orphans), "
83-
+ "this fires unconditionally and moves the box one page further than the forced break already "
84-
+ "placed it, exactly like PeachPDF's stale-cursor bug: the page the break named (PageTopOf(1)) is "
85-
+ "left blank and the box lands on PageTopOf(2) instead. Confirmed by running this test unignored.")]
8677
public async Task AForcedBreak_LandsOnThePageItNames_EvenWhenTheBoxCannotMeetItsOrphansMinimum()
8778
{
8879
var html = "<div id='a' style='height:50px'>A</div>"

‎Source/Test/HtmlRenderer.IntegrationTest/Fragmentation/OrphansWidowsIntegrationTests.cs‎

Lines changed: 17 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,24 @@ public async Task Orphans_IsInherited()
136136

137137
private const double LineHeight = 20;
138138

139+
// A narrow width (each word alone easily exceeds half of it, so no two ever share a line) forces
140+
// exactly one word per rendered line via natural wrapping - deliberately not <br>: this fork's <br>
141+
// handling (DomParser.CorrectLineBreaksBlocks) only folds a <br> into a real forced-newline "\n" word
142+
// when it is the LAST thing in its inline run; a <br> with more inline content after it is left as a
143+
// literal box, which the parser's block-correction pass then splits into SEPARATE anonymous BLOCK
144+
// siblings (one per <br>-delimited run), each holding exactly one line of its own - confirmed directly
145+
// by inspecting the box tree (CssBox.Boxes came back as 7 children: 4 display:block text runs
146+
// interleaved with 3 display:inline <br> boxes, and the <p> itself had zero LineBoxes of its own).
147+
// InlineFragmentation.ApplyLineBreaking operates on a SINGLE box's own LineBoxes list, so a <br>-joined
148+
// fixture never exercises its multi-line widows/orphans correction at all - every "line" is really an
149+
// independent 1-line sibling block, which is a fundamentally different (and, for this feature, useless)
150+
// shape than what these tests are meant to probe. A real multi-word-wrapped paragraph does not have
151+
// this problem (confirmed empirically: no such splitting, and no spurious extra line box either - that
152+
// only showed up at truly Ext width like 10px, an unrelated edge case avoided by staying well clear of
153+
// it here).
139154
private static string Paragraph(int lineCount, string extraStyle = "") =>
140-
"<p id='p' style='width:200px;line-height:" + LineHeight + "px;margin:0;padding:0;" + extraStyle + "'>"
141-
+ string.Join("<br>", Enumerable.Range(1, lineCount).Select(i => $"Line{i}"))
155+
"<p id='p' style='width:50px;line-height:" + LineHeight + "px;margin:0;padding:0;" + extraStyle + "'>"
156+
+ string.Join(" ", Enumerable.Range(1, lineCount).Select(i => $"Line{i}"))
142157
+ "</p>";
143158

144159
/// <summary>How many of a paragraph's own lines fall on each side of a page boundary, at the given
@@ -165,15 +180,6 @@ private static string Paragraph(int lineCount, string extraStyle = "") =>
165180
// 4-line paragraph straddling with only 1 line naturally following the break (violating widows:2) must
166181
// have exactly one line pulled across, landing 2-before/2-after - not the whole box pushed on.
167182
[TestMethod]
168-
[Ignore("Confirmed gap, traced by hand against InlineFragmentation.ApplyLineBreaking: its widows "
169-
+ "merge-back loop can only REMOVE a break entirely (fully merging two runs), never shift a break "
170-
+ "point earlier by fewer lines while keeping two runs, and never falls back to pushing the whole "
171-
+ "run to a FRESH page when an in-place merge does not fit the CURRENT page's remaining room. At "
172-
+ "several swept filler heights (e.g. 22px, 4 lines in a 100px page), phase 1 naturally breaks at a "
173-
+ "point that leaves fewer than widows:2 lines after the break; the merge-back then tries removing "
174-
+ "that break entirely, finds the WHOLE run does not fit in the page's remaining room, and gives up "
175-
+ "- leaving the violating split - rather than shifting the break by one line (which would fit) or "
176-
+ "pushing the whole run to a fresh page (which would also fit).")]
177183
public async Task Widows2_OnlyOneLineWouldFollowTheBreak_MovesOneLineRatherThanTheWholeBox()
178184
{
179185
var checkedAny = false;
@@ -192,8 +198,6 @@ public async Task Widows2_OnlyOneLineWouldFollowTheBreak_MovesOneLineRatherThanT
192198
}
193199

194200
[TestMethod]
195-
[Ignore("Same confirmed gap as Widows2_OnlyOneLineWouldFollowTheBreak_MovesOneLineRatherThanTheWholeBox "
196-
+ "- see that test's own remark.")]
197201
public async Task Widows3_MovesAsManyLinesAsItTakes()
198202
{
199203
var checkedAny = false;
@@ -213,8 +217,6 @@ public async Task Widows3_MovesAsManyLinesAsItTakes()
213217
// Where the two constraints meet, one has to give: honoring widows:4 on a 4-line paragraph would leave
214218
// none before the break, so the per-line correction gives up in favor of pushing the whole box.
215219
[TestMethod]
216-
[Ignore("Same confirmed gap as Widows2_OnlyOneLineWouldFollowTheBreak_MovesOneLineRatherThanTheWholeBox "
217-
+ "- see that test's own remark.")]
218220
public async Task Widows4_CannotBeSatisfiedWithoutBreakingOrphans_PushesTheWholeBox()
219221
{
220222
var checkedAny = false;
@@ -237,14 +239,6 @@ public async Task Widows4_CannotBeSatisfiedWithoutBreakingOrphans_PushesTheWhole
237239
}
238240

239241
[TestMethod]
240-
[Ignore("Confirmed gap by running this test unignored: at several swept filler heights (e.g. 62px), the "
241-
+ "paragraph's first fragment keeps only 1 line before the break, fewer than orphans:2 requires. "
242-
+ "InlineFragmentation.ApplyLineBreaking's own phase-1 orphans back-off (breaks.RemoveAt + retry) "
243-
+ "only fires once at least one earlier break already exists (breaks.Count > 1, per that method's "
244-
+ "own comment on the condition), so a violation surfacing at the very FIRST break decision is never "
245-
+ "corrected there - only firstRunMovedToFreshPage's own narrower case (the paragraph's very first "
246-
+ "run) is, which is why the already-passing OrphansOnFirstRunTest.cs does not contradict this: its "
247-
+ "own fixture never lands in the specific narrow gap this one does.")]
248242
public async Task Orphans2_ParagraphNudgedWhenOnlyOneLineWouldPrecedeTheBreak()
249243
{
250244
var checkedAny = false;
@@ -309,8 +303,6 @@ public async Task TallParagraph_ExceedsOnePage_IsNotNudged()
309303
// be helped by moving it whole, but the break *before it* can fall earlier - with too few lines above
310304
// the boundary, orphans:2 must still push the whole thing to the next page rather than stranding one.
311305
[TestMethod]
312-
[Ignore("Same confirmed gap as Orphans2_ParagraphNudgedWhenOnlyOneLineWouldPrecedeTheBreak - see that "
313-
+ "test's own remark.")]
314306
public async Task Orphans2_ParagraphTallerThanTheBand_BreaksBeforeItselfRatherThanStrandingOneLine()
315307
{
316308
var checkedAny = false;
@@ -408,12 +400,6 @@ public async Task Orphans2_HeadingAndParagraph_AreCorrectedOnceRatherThanWalking
408400
}
409401

410402
[TestMethod]
411-
[Ignore("Confirmed gap by running this test unignored: hits the same widows merge-back limitation as "
412-
+ "Widows2_OnlyOneLineWouldFollowTheBreak_MovesOneLineRatherThanTheWholeBox above (the merge-back can "
413-
+ "only remove a break entirely, never shift it earlier by fewer lines, nor fall back to pushing the "
414-
+ "whole run to a fresh page when the in-place merge doesn't fit) - this variant only adds that the "
415-
+ "paragraph's own natural top already lies past page 0, which does not change the underlying "
416-
+ "mechanism or its outcome.")]
417403
public async Task Widows2_ParagraphStartingOnSecondPage_StillCorrected()
418404
{
419405
var checkedAny = false;

0 commit comments

Comments
 (0)