Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
54cabab to
45b4b4b
Compare
…tive-leading fixes - Inline-block uses its last in-flow line box baseline, but inline flex/grid containers use their first baseline per css-align. - Bump parley to DioxusLabs/parley#10 which fixes clamping of negative half-leading in line extents and adds opt-in CSS2 \u00a7 10.8 line-box strut support; enable the strut in Blitz's inline layout. - With an explicit baseline, an inline box's line-height contribution may legitimately be negative, so only clamp reserved space for bottom-aligned boxes, and position baseline-aligned boxes from the top of their margin box.
Enabling the strut regressed ~200 WPT tests whose references use vertical-align: top images to suppress the descender gap below baseline-aligned images, which Blitz cannot yet honor.
…le flex/grid baselines from content
c7cb9fd to
69436ae
Compare
|
Data point from a different architecture that landed the same semantics — posting in case it helps the tradeoff discussion here. We maintain a blitz-lineage layout fork (via obscura; the rename is still in flight) where inline runs are taffy flex rows and text is a taffy leaf. The blocker for the approach in this PR: a taffy leaf's measure closure returns What we shipped instead: a post-layout per-line shift. Layout runs with Why we think the line grouping is sound: flex-wrap line breaking is width-driven and baseline-independent, so the FLEX_START line grouping is the correct CSS line-box grouping before any shift is applied. Two properties that made this cheap to trust:
Semantics match what this PR implements: Caveat we accepted: if declared |
|
I think you're going to run into a lot of limitations with lines-as-flex-rows. Would recommend against it. You can apply baseline alignment as a post layout step on top of Parley today if you want to. Takumi does it. And I know of another closed source project that does it. Would recommend doing that if you want vertical-align today. We could do it in Blitz, but our aim here is to build a solid architecture that people can actually build on long-term. And I think the right place for this long-term is in Parley. |
|
Thanks for the read — and no disagreement on the architecture point. To clarify what we actually shipped: the shift is the post-layout step you're describing, computed from font metrics (ascent/descent + declared line-height), not from flex mechanics. The flex rows are inherited structure we treat as an opaque container — taffy's only job there is wrap-based line grouping, which is width-driven, so FLEX_START hands us the correct line boxes to shift within. We wouldn't recommend lines-as-flex-rows as a greenfield design either. Good to know about Takumi — will look at how they handle mixed font-size runs, that's our main remaining edge (per-segment declared line-heights can overflow the taffy line cross-size by a few px). Long-term in Parley makes sense to us. If |
2 participants
Summary
Wires Taffy's baseline output through to Parley's
InlineBox::baselineso in-flow atomic inlines are baseline-aligned per CSS instead of bottom-edge aligned, and bumps Parley to DioxusLabs/parley#7 (rev95a9ff1, which includes DioxusLabs/parley#10) for line-metrics fixes plusvertical-align: top/bottomsupport.In
compute_inline_layout, when updating each in-flow inline box from its Taffy layout output:Taffy baselines are measured from the top border edge; Parley expects the baseline relative to the top of the inline box (which here is the margin box, since
ibox.width/heightinclude margins), hencemargin.top + b. Thebaselines.lastvalues are populated by the Taffy last-baseline work (#721), whose commits are included in this PR now that it targetsmain.Two adjustments to existing negative-vertical-margin handling now that boxes can carry a baseline:
ibox.height) is only clamped to>= 0for bottom-aligned boxes (no baseline); with an explicit baseline the ascent/descent contributions may legitimately be negative (e.g.margin-top: -20emon an inline-flex).ibox.yis the margin-box top (Parley places it atline_baseline - baseline), so the border box goes atibox.y + margin.top(unclamped); bottom-aligned boxes keep the previousmargin.top.max(0.0)bottom-anchoring.Parley bump to
95a9ff1(DioxusLabs/parley#7, on top of #10) brings:line-height-*tests below).builder.set_compute_strut(true).InlineBox::vertical_align(InlineBoxVerticalAlign::{Baseline, Top, Bottom}). Blitz maps the computedbaseline-shiftlonghand (the stylo longhand behindvertical-align: top/bottom) to it:With
vertical-align: top/bottomsupported, the strut no longer regresses the ~200 tests whose references usevertical-align: topimages (spot-checkedwidth-006.xht,inline-replaced-width-001.xht,float-003.xht,width-percentage-001.xht,background-size-025.html: all pass locally with strut + vertical-align).append_inline_box_to_linesignature changed; out-of-flow floated boxes now passalignment: None.WPT results
CI's full-suite run vs
mainreports 142 newly passing / 50 newly failing (net +92) (full diff in the generated section below). All 50 were re-verified locally (each passes onmainat 10606cc and failed on this branch before the latest commits) and triaged. 8 of the 50 are fixed by the latest commits; the remainder cluster as follows.Fixed in this PR (8)
contain-layout-baseline-001/005,contain-layout-flexbox-001,contain-layout-grid-001: layout containment must suppress the box's baseline (css-contain §3 "no baseline"). Blitz now checks the computedcontainvalue forlayout(also covered bystrict/content) and passesbaseline: Nonefor such atomic inlines.baseline-of-scrollable-1a: a scrollable inline flex/grid container still takes its baseline from its content (as if not scrolled), clamped to the border box (css-align §9.1 / Not clamping baseline position when scrollable overflow gives weird results w3c/csswg-drafts#7660) — only inline-block scroll containers synthesize from the bottom margin edge. Blitz now clampsoutput.baselines.firstto[0, height]for flex/grid scroll containers instead of dropping it.floats-141.xht(fixed in an earlier commit): 1px glyph shift from a fractional inline-box baseline;ibox.baselineis now rounded to physical pixels.flexbox-baseline-multi-line-horiz-004.html,grid-baseline-001.html: fixed by bumping Taffy to pick up Flexbox: fix container baseline for column-reverse containers taffy#1127 (wrap-reverse container baseline from the visually startmost line) and Grid: exclude items with auto block-axis margins from baseline alignment taffy#1125 (items with auto block-axis margins don't participate in grid baseline alignment).Upstream Taffy baseline gaps (3 remaining)
Three Taffy fixes landed upstream (DioxusLabs/taffy#1125, #1127, #1126) and this PR's Taffy pin was bumped to a merge of taffy
maininto the last-baseline branch (8c49a27f), which fixedflexbox-baseline-multi-line-horiz-004andgrid-baseline-001(see above). Still failing:baseline-block-with-overflow-001.html,baseline-of-scrollable-1b.html: taffy#1126 added block scroll-container baseline clamping/synthesis, but these still fail because Blitz's inline layout doesn't feed Taffy block-child baselines into inline-block vertical alignment for these structures (the tests wrap the scrollable boxes so the relevant baseline propagation is only partially exercised).flexbox-baseline-multi-line-vert-002.html: the vertical-writing-mode variant additionally depends on the CSS2 strut interaction (see next section).Spurious base passes exposed by parley#10's nbsp fix (5)
inline-block-zorder-002/004/005.xht,inline-table-zorder-001.xht,inline-table-width-002b.xht: onmain, any element whose text content is only renders completely blank (even its background), so these tests compared blank-vs-blank and "passed". Parley update to sync with master branch of Dioxus #10 fixes nbsp handling, so this PR actually renders them, exposing pre-existing issues: the zorder tests fail on paint order (a later sibling block's background, pulled up via negative margin, is painted over earlier inline-block/inline-table content, contra CSS2 Appendix E);inline-table-width-002badditionally involves an unrenderedinline-table, with test/ref differing only in line spacing around the invisible box.CSS2 line-box strut interactions (12) — pass again with
set_compute_strut(false)list-style(-image/-type)-applies-to-012/014.xht(6 tests),contain-size-flexbox-002.html,font-colorization.html,intrinsic-percent-replaced-008.html,intrinsic-percent-replaced-dynamic-005.html,abs-pos-with-replaced-child.html, and (combined with the baseline wiring)flexbox-baseline-multi-line-vert-002.html.The strut itself is spec-correct (CSS2 §10.8) and is what fixes the much larger set of
vertical-align: topreference mismatches; these failures are cases where the strut interacts with other Blitz gaps rather than the strut being wrong. E.g. theliststests wrap a zero-height list-item inside an inline-block: the strut now reserves a full line for the otherwise-empty line box containing the zero-height inline box, pushing the outside marker one line below where the (strut-less) reference paints it — proper handling needs marker boxes to participate in the line like browsers do. Theintrinsic-percent-replaced/abs-pos-with-replaced-childrefs contain text next to replaced elements whose heights now differ by the strut's leading. Individual fixes here are follow-up work in Blitz's marker/replaced-element line handling, not in this PR's wiring.Parley 0.11.1 → git-main upgrade behavior changes (27) — fail regardless of strut/baseline wiring
These remain failing with the strut disabled and the baseline wiring neutralized, so they come from the Parley upgrade itself (new CSS2 per-side line metrics from linebender/parley#639, nbsp now rendering, changed trailing-whitespace/hanging behavior), or from pre-existing Blitz issues newly exposed by it:
css-ruby/br-clear-all-000,empty-ruby-base-container,empty-ruby-text-container-abs/-float,ruby-base-container-abs/-float— empty ruby structures now contribute line height where they previously collapsed.trailing-ideographic-space-013/014,hanging-whitespace-002.tentative,full-width-leading-spaces-004,hanging-punctuation-first-002— parley main changed trailing-space hang/measurement behavior (nearby siblings of these tests start passing, so it's a behavior trade within the same feature).font-family-applies-to-005,font-feature-settings-tibetan,small-caps-letter-spacing-002,font-colorization— shaping/metrics changes in parley main (several other shaping tests start passing).floats-149.xht: the green block collapses to zero height — its only in-flow content is an empty inline whose children are floats; a line box containing only an empty inline + floats no longer contributes height under the new line-metrics model (CSS2 §9.4.2 empty-inline handling needed).empty-span-size-002.html: empty spans' contribution to line height changed with the new per-side line metrics.t41-html4-keywords-a,contain-animation-001,custom-highlight-painting-inheritance-001/002,text-decoration-skip-spaces-001,css-break/ruby-003— small text-position shifts from the new line metrics cause these paint-comparison tests to mismatch.grid-layout-auto-tracks,column-subgrid-grid-gap-008— content-sized track sizing shifted by the changed text-line heights.Note:
flexbox-baseline-multi-item-horiz-001a.html-style tests still fail, but for orthogonal reasons (synthesized baselines of empty/no-text flex items), not the inline-box wiring.WPT results
144 newly passing, 43 newly failing (net +101).
Full diff (187 changed tests)
Generated by the WPT workflow.
Link to Devin session: https://dioxus.staging.devinenterprise.com/sessions/3817a3c1194f47afbc30e587ba79678f
Requested by: @nicoburns