Repository navigation
Conversation
eXpl0it3r
force-pushed
the
feature/css21-cascade-boxmodel-cleanup
branch
2 times, most recently
from
November 9, 2026 07:49
32226ac to
5a78ad2
Compare
Collaborator
|
Rebased onto the updated #266 and added one fix to the margin collapsing:
Tests are in |
eXpl0it3r
force-pushed
the
feature/css21-cascade-boxmodel-cleanup
branch
2 times, most recently
from
November 9, 2026 08:14
0b67670 to
7183e98
Compare
eXpl0it3r
approved these changes
Nov 9, 2026
eXpl0it3r
force-pushed
the
feature/css21-cascade-boxmodel-cleanup
branch
from
November 9, 2026 08:21
7183e98 to
80c7705
Compare
…rder Css/GlobalKeywordPropertyTests.cs only verified the five global keywords parse and get flagged correctly (IsInherited/IsInitial/ etc.) - nothing exercised actual cascade RESOLUTION: does an inherited property really pick up the parent's value, does initial really reset ignoring inheritance, does !important really outrank a later inline style. All 15 pass against the existing cascade engine unmodified. Ports the CSS 2.1-relevant subset of PeachPDF's GlobalKeywordCascadeTests.cs (44 tests) - inherit/initial/unset and basic cascade-order regressions (inline vs author, !important precedence, source-order tiebreak). revert/revert-layer (CSS Cascading and Inheritance Level 5), cascade layers, and PeachPDF's own 3-phase-cascade-refactor regression history are not ported - out of CSS 2.1 scope or specific to PeachPDF's own architecture.
… bug text-indent was fully implemented (parsed + wired into layout) but untested beyond CSS-OM parsing - added first-line-only indent coverage (the plain CSS2.1 behavior; CSS Text Level 4's hanging/ each-line/RTL variants aren't ported, since this fork's text-indent is the simpler CSS2.1 one). Attempting the equivalent word-spacing port surfaced a real, confirmed bug: word-spacing is fully wired (CSS-OM, CssBoxProperties, CssUtils dispatch, CssRect.FullWidth) but has zero effect on ordinary adjacent words in normal text flow, whether set on the text-owning block directly or an ancestor - the anonymous text box that actually owns the words never inherits it, always reading back "normal". PeachPDF's own equivalent test passes (confirmed by running it), so this is a real regression against a working reference implementation, not a PeachPDF gap to report upstream. Two tests document this precisely, [Ignore]d rather than fixed here (root-causing the anonymous-text-box inheritance path was out of this pass's budget); flagged as a follow-up task.
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.
Rounds out the box-model (ch.8/10) and table (ch.17) spot-checks this branch's plan originally called for: min-height/max-height precedence (CSS2.1 §10.7, including the percentage-height-against-auto-height- container edge case), table-cell height stretching to its row's height even when the cell's own explicit height is shorter (§17.5.3), and border-spacing:0 leaving no gap between adjacent cells. All pass against the existing engine unmodified. Chapter 11 (overflow/clip/visibility) and 13 (paged media) were spot-checked against PeachPDF's fuller suites and found already adequately covered for CSS 2.1 - PeachPDF's remaining OverflowClipIntegrationTests cases are border-radius+stacking-context specific (CSS3), and its paged-media margin-box/named-page tests are CSS3 Paged Media/GCPM, both out of this sweep's CSS 2.1 scope.
Floats, absolutely positioned boxes, table cells and boxes with overflow other than visible keep their children's margins inside. A parent with a fixed height doesn't pass its last child's bottom margin on either.
eXpl0it3r
force-pushed
the
feature/css21-cascade-boxmodel-cleanup
branch
from
November 9, 2026 08:26
80c7705 to
a7a435b
Compare
Collaborator
|
Thank you! |
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.
Part 4 (final) of a CSS 2.1 test-parity sweep against PeachPDF's Integration test suite. Stacked on #266/#265/#264/#263 - the diff below includes all four until they merge. Only the commits from "Port cascade-resolution tests..." onward are this PR's own.
Summary
Scope: chapters 6 (cascade/inheritance resolution), 8 (box model - margin collapsing), 10 (visual formatting details), 16 (
text-indent), and 17 (tables) cross-checks.Css/GlobalKeywordPropertyTests.csonly verifiedinherit/initial/unsetparse and get flagged correctly - nothing exercised actual cascade resolution. Ported the CSS 2.1-relevant subset of PeachPDF'sGlobalKeywordCascadeTests.cs(44 tests) - 15 pass against the existing cascade engine unmodified.[Ignore]d tests; flagged as a follow-up task rather than fixed here.CssBox.MarginTopCollapseonly ever resolved a pairwiseMath.Maxbetween exactly two margins - never CSS 2.1 §8.3.1's actual "adjoining margin set" (which spans an arbitrary run of self-collapsing empty boxes and nested first/last-in-flow children, and collapses tomax(positives) + min(negatives), notMath.Maxalone). This was already caught and documented as 4 confirmed,[Ignore]d bugs by an earlier audit pass; all 4 now pass for real, alongside a full adjoining-margin-set implementation (CollapsedMarginTopChain/CollectAdjoiningMarginsBeforeSibling/CollapseMarginSet). Verifying this against the full pixel-diff regression corpus (not just the 5 targeted tests) surfaced and fixed two further real bugs - see the commit message for the full account, including a percentage-margin-resolved-too-early bug and a "chain reaching the document root silently discards the margin" bug. Re-approved 6 pixel-diff baselines whose content legitimately changed now that margin collapsing actually works (e.g.margin: 10%now correctly resolves against the containing block's width, and an auto-height parent with no border/padding correctly lets a margin collapse all the way through it) - each diff reviewed by hand before approving.border-spacing:0. Chapters 11 (overflow/clip) and 13 (paged media) were spot-checked against PeachPDF's fuller suites and found already adequately covered for CSS 2.1 - PeachPDF's remaining cases in both are CSS3-specific (border-radius+stacking-context; Paged Media/GCPM margin boxes).Test plan
HtmlRenderer.Testfull suite: 2449 passed, 0 failedHtmlRenderer.IntegrationTestfull suite: 412 passed, 0 failedHtmlRenderer.PdfSharp.Testfull suite: 39 passed, 0 failed