Skip to content

Commit f5acb05

Browse files
committed
Implement CSS 2.1 §8.3.1's real adjoining-margin-set algorithm
CssBox.MarginTopCollapse only ever resolved a pairwise Math.Max between exactly two margins - never the full "adjoining margin set" (collapsing to max(positives) + min(negatives), spanning an arbitrary run of self-collapsing empty boxes and nested first/last-in-flow children). This happened to match spec for same-sign pairs, which is why 1 of 5 existing tests already passed; the other 4 were confirmed, documented, [Ignore]d bugs from an earlier audit pass. All 5 now pass for real. CollapsedMarginTopChain/CollapsedMarginBottomChain walk forward through an unblocked run of first/last-in-flow children (an inline or inline-block parent can't participate - margin collapsing is a block- formatting-context concept), so a nested child sits flush with its parent instead of being pushed down twice. CollectAdjoiningMarginsBeforeSibling walks backward through however many self-collapsing (empty) siblings precede a box, reaching all the way to the parent's own top margin if the whole run turns out self-collapsing. CollapseMarginSet does the actual max(positives) + min(negatives) fold. A floated box's own margin never participates in collapsing at all (CSS 2.1 §8.3.1: floats are out of flow) - the predecessor's trailing margin is still real space, but the float's own margin sums on top rather than merging. Two real, non-obvious bugs surfaced and got fixed while verifying this against the full regression suite (not just the 5 targeted tests): 1. A "child sits flush, trust the parent already absorbed it" design only works if CollapsedMarginTop tracks CUMULATIVE REALIZED displacement (parent.CollapsedMarginTop + this box's own excess), not each box's own semantic total in isolation - the first version let a chain reaching the document root (which cannot itself move) incorrectly "claim credit" for a value nothing had ever visually applied, silently discarding real margins throughout ordinary documents. Caught by the pixel-diff regression corpus, not the targeted tests, which don't exercise a chain deep enough to hit it. 2. CssBoxProperties.ActualMarginTop resolves a percentage margin against the box's OWN Size.Width (a pre-existing quirk, left alone to avoid a wider blast radius) - safe once a box's own width has actually been computed, but the forward-chain lookahead deliberately reads a NOT-yet-laid-out descendant's margin, so Size.Width is still the zero default there. New ResolveMarginTopForChain/ ResolveMarginBottomForChain resolve a percentage margin against the containing block's already-known width instead, when read through the lookahead specifically. Also fixes CssBoxHr's own separate copy of the placement formula (missed by the earlier self-collapsing-anchor fix, since it duplicates rather than calls CssBox's placement code) to match. Re-approves 6 pixel-diff baselines whose content legitimately changed now that margin collapsing actually works: most visibly, a margin percentage now correctly resolves against the containing block's WIDTH (not height) per CSS2.1, and an auto-height parent with no border/padding now correctly lets a sole child's top/bottom margins collapse all the way through it rather than showing as visible space inside the parent's own background - both are well-known, if surprising, real CSS behaviors, confirmed by direct comparison against each diff image before approving. MaxWidth/Iframes' diffs are network- dependent broken-image-placeholder rendering (external URLs unreachable in this environment for both the old and new baseline capture) and don't reflect a functional change either way.
1 parent 67bab41 commit f5acb05

10 files changed

Lines changed: 268 additions & 69 deletions

File tree

‎Source/HtmlRenderer/Core/Dom/CssBox.cs‎

Lines changed: 254 additions & 9 deletions
Large diffs are not rendered by default.

‎Source/HtmlRenderer/Core/Dom/CssBoxHr.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,13 @@ protected override void PerformLayoutImp(RGraphics g)
4848

4949
var prevSibling = DomUtils.GetPreviousSibling(this);
5050
double left = ContainingBlock.Location.X + ContainingBlock.ActualPaddingLeft + ActualMarginLeft + ContainingBlock.ActualBorderLeftWidth;
51+
// The baseline for this rule's own collapsed top margin, matching CssBox.PerformLayoutImp's
52+
// own placement: a self-collapsing prevSibling (CSS 2.1 8.3.1) contributes no space of its own
53+
// and must not be anchored against directly (see FindMarginAnchorSibling).
54+
var marginAnchor = FindMarginAnchorSibling(prevSibling);
5155
// StaticBottom (not ActualBottom): a relatively-positioned previous sibling's visual offset must
5256
// not drag this rule down with it (CSS 2.1 9.4.3), matching the same fix in CssBox.PerformLayoutImp.
53-
double top = (prevSibling == null && ParentBox != null ? ParentBox.ClientTop : ParentBox == null ? Location.Y : 0) + MarginTopCollapse(prevSibling) + (prevSibling != null ? prevSibling.StaticBottom + prevSibling.ActualBorderBottomWidth : 0);
57+
double top = (marginAnchor == null && ParentBox != null ? ParentBox.ClientTop : ParentBox == null ? Location.Y : 0) + MarginTopCollapse(prevSibling) + (marginAnchor != null ? marginAnchor.StaticBottom + marginAnchor.ActualBorderBottomWidth : 0);
5458
Location = new RPoint(left, top);
5559
ActualBottom = top;
5660

20 Bytes
Loading
21 Bytes
Loading
-35 Bytes
Loading
-28 Bytes
Loading
-96 Bytes
Loading
427 Bytes
Loading
-59 Bytes
Loading

‎Source/Test/HtmlRenderer.IntegrationTest/BoxModel/CollapsedMarginBeforeTests.cs‎

Lines changed: 9 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,15 @@ namespace HtmlRenderer.IntegrationTest.BoxModel;
77
/// set, resolved at one break point by the frame the boxes are children of.
88
/// </summary>
99
/// <remarks>
10-
/// <para>
11-
/// The set spans two frames, and these pin both halves: what precedes a box is the frame's to see (a
12-
/// predecessor's bottom margin, and everything a run of self-collapsing predecessors folds in), while the box's
13-
/// own top margin and the first-in-flow-child chain adjoining it are a walk into its own subtree. §8.3.1
14-
/// collapses the whole set at once, so the two are folded together rather than resolved separately.
15-
/// </para>
16-
/// <para>
17-
/// This fork's margin-collapse machinery (<c>CssBox.MarginTopCollapse</c>/<c>MarginBottomCollapse</c>,
18-
/// Dom/CssBox.cs ~1103-1183) only ever resolves a <i>pairwise</i> <c>Math.Max</c> between exactly two margins at
19-
/// a time - never the full CSS2.1 "adjoining margin set" (which can span an arbitrary run of self-collapsing
20-
/// boxes and nested first-in-flow children, and which spec requires collapsing to
21-
/// <c>max(positives) + min(negatives)</c>, not <c>Math.Max</c> alone). <c>Math.Max</c> on two same-signed margins
22-
/// happens to equal the spec result by coincidence, but it diverges the moment the set has to look past a single
23-
/// pair - whether because a negative margin is mixed in, or because more than two margins are actually
24-
/// adjoining. See each test's own <see cref="IgnoreAttribute"/> for the specific way it trips over this.
25-
/// </para>
10+
/// <c>CssBox.MarginTopCollapse</c> (Dom/CssBox.cs) implements the real adjoining-margin-set algorithm:
11+
/// <c>CollapsedMarginTopChain</c>/<c>CollapsedMarginBottomChain</c> walk forward through an unblocked run of
12+
/// first/last-in-flow children (so a nested child sits flush with its parent instead of being pushed down a
13+
/// second time), <c>CollectAdjoiningMarginsBeforeSibling</c> walks backward through however many self-collapsing
14+
/// (empty) siblings precede a box - reaching all the way to the parent's own top margin if the whole run turns
15+
/// out self-collapsing - and <c>CollapseMarginSet</c> collapses the resulting set to
16+
/// <c>max(positives) + min(negatives)</c>, not a plain <c>Math.Max</c> between just two values (which only
17+
/// happens to match when both margins share a sign). A floated box's own margin never participates in
18+
/// collapsing at all (CSS 2.1 §8.3.1: floats are out of flow).
2619
/// </remarks>
2720
[DoNotParallelize]
2821
[TestClass]
@@ -45,12 +38,6 @@ public void AdjoiningSiblingMargins_CollapseToTheLargerPositive()
4538
Assert.AreEqual(30, b.Location.Y - a.ActualBottom, Delta);
4639
}
4740

48-
[Ignore("Confirmed real bug, not a porting mistake: CssBox.MarginTopCollapse's sibling branch (Dom/CssBox.cs " +
49-
"~line 1108) computes `Math.Max(prevSibling.ActualMarginBottom, ActualMarginTop)` for the gap between " +
50-
"two in-flow siblings - for a positive/negative mixed pair (30, -10) that picks 30 (the larger " +
51-
"value, treating the negative margin as if it weren't there), instead of CSS2.1's mixed-sign rule of " +
52-
"summing the largest positive and the most-negative value (30 + (-10) = 20). Confirmed: " +
53-
"b.Location.Y - a.ActualBottom is 30 on this fork, not the spec-correct 20.")]
5441
[TestMethod]
5542
public void MixedSignSiblingMargins_Sum()
5643
{
@@ -64,14 +51,6 @@ public void MixedSignSiblingMargins_Sum()
6451
Assert.AreEqual(20, b.Location.Y - a.ActualBottom, Delta);
6552
}
6653

67-
[Ignore("Confirmed real bug, not a porting mistake: because MarginTopCollapse only ever looks at the single " +
68-
"immediate previous sibling (Dom/CssBox.cs ~1103-1127), it cannot treat a self-collapsing empty box " +
69-
"as transparent for a single 3-way adjoining set the way CSS2.1 requires. Instead it resolves two " +
70-
"SEPARATE pairwise gaps in series - a-to-middle: Math.Max(a.marginBottom=10, middle.marginTop=40)=40, " +
71-
"then middle-to-b: Math.Max(middle.marginBottom=40, b.marginTop=10)=40 - and since the empty middle " +
72-
"box has zero height (its own margin-bottom is never folded into its own ActualBottom), those two " +
73-
"40px gaps stack in series instead of collapsing to one shared 40px value. Confirmed: " +
74-
"b.Location.Y - a.ActualBottom is 80 on this fork (40+40), not the spec-correct 40.")]
7554
[TestMethod]
7655
public void ASelfCollapsingSiblingBetweenTwoBoxes_JoinsTheSet()
7756
{
@@ -87,16 +66,6 @@ public void ASelfCollapsingSiblingBetweenTwoBoxes_JoinsTheSet()
8766
Assert.AreEqual(40, b.Location.Y - a.ActualBottom, Delta);
8867
}
8968

90-
[Ignore("Confirmed real bug, not a porting mistake: MarginTopCollapse's parent-child branch (Dom/CssBox.cs " +
91-
"~1111-1114) is `Math.Max(0, ActualMarginTop - Math.Max(parent.ActualMarginTop, parent.CollapsedMarginTop))` " +
92-
"- an approximation that does not implement 'the whole chain collapses to one value, taken once' the " +
93-
"way CSS2.1 requires. For outer(margin-top:10)/inner(margin-top:30) here: outer's own placement " +
94-
"against 'a' only applies outer's OWN 10px margin (Math.Max(a.marginBottom=0, outer.marginTop=10)=10, " +
95-
"not the spec-correct 30 which should reflect the whole outer+inner set), and inner then gets pushed " +
96-
"an ADDITIONAL Math.Max(0, 30-Math.Max(10,10))=20px below outer's own top - so inner ends up 20px " +
97-
"below outer (not flush with it), and outer is only 10px below 'a' (not 30px). Confirmed on this " +
98-
"fork: outer.Location.Y - a.ActualBottom is 10 (not 30), and inner.Location.Y - outer.Location.Y is " +
99-
"20 (not 0).")]
10069
[TestMethod]
10170
public void AFirstInFlowChildsTopMargin_JoinsItsParentsOwnSet()
10271
{
@@ -115,16 +84,6 @@ public void AFirstInFlowChildsTopMargin_JoinsItsParentsOwnSet()
11584
Assert.AreEqual(outer.Location.Y, inner.Location.Y, Delta);
11685
}
11786

118-
[Ignore("Confirmed real bug, not a porting mistake: the parent-child join's guard condition (Dom/CssBox.cs " +
119-
"~1111) only checks `ActualPaddingTop < 0.1 && ActualPaddingBottom < 0.1` on the box and its parent - " +
120-
"it never checks BORDER width at all, even though CSS2.1 requires ANY border or padding between " +
121-
"parent and child to block the join, not just padding. So outer's border-top:1px here fails to block " +
122-
"the outer/inner join: inner still gets pulled into the same Math.Max(0, inner.marginTop=30 - " +
123-
"Math.Max(outer.marginTop=10, outer.CollapsedMarginTop=10))=20px-below-outer's-ClientTop computation " +
124-
"as the no-border case, landing it 1px (border) + 20px = 21px below outer, not the spec-correct " +
125-
"1px + 30px = 31px where the border fully blocks the join and both margins stand independently. (The " +
126-
"outer-vs-'a' gap of 10px does coincidentally match here, since that part never depended on the " +
127-
"border in the first place.)")]
12887
[TestMethod]
12988
public void ABorderOnTheParent_BlocksTheChainAndLeavesBothMarginsStanding()
13089
{
@@ -141,15 +100,6 @@ public void ABorderOnTheParent_BlocksTheChainAndLeavesBothMarginsStanding()
141100
Assert.AreEqual(31, inner.Location.Y - outer.Location.Y, Delta);
142101
}
143102

144-
[Ignore("Confirmed real bug, not a porting mistake, compounding two gaps: (1) this fork's block-placement " +
145-
"seam does not special-case Position=float the way it does display:none/absolute/fixed siblings (the " +
146-
"same gap BoxModel.BlockPlacementSeamTests.AFloatedSibling_IsNotThePredecessorTheFrameResolvesAgainst " +
147-
"documents) - so the floated 'f' box is positioned via the exact same in-flow " +
148-
"top = prevSibling.ActualBottom + MarginTopCollapse(prevSibling) formula as any ordinary block, " +
149-
"instead of being excluded from margin collapsing entirely per CSS2.1 (a float's margin should never " +
150-
"adjoin anything). (2) That collapse is then itself the mixed-sign Math.Max bug this class's remarks " +
151-
"describe: Math.Max(a.marginBottom=20, f.marginTop=-5)=20, not the spec-correct sum " +
152-
"20+(-5)=15. Confirmed: f.Location.Y - a.ActualBottom is 20 on this fork, not 15.")]
153103
[TestMethod]
154104
public void AFloatsOwnMarginDoesNotMerge_ButThePredecessorsStillOccupiesSpace()
155105
{

0 commit comments

Comments
 (0)