LibWeb: Vertical alignment fixes for (text-)top and (text-)bottom - #11052
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe line builder adds line-relative aligned subtree tracking for horizontal writing modes. It supports top, bottom, text-top, and text-bottom alignment. Tests cover nested subtrees, atomic inline elements, writing modes, and updated layout geometry. Vertical alignment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LineBuilder
participant LineRelativeAlignedSubtree
participant Fragments
participant StaticPositionMarkers
LineBuilder->>LineRelativeAlignedSubtree: record subtree extents and alignment
LineBuilder->>LineRelativeAlignedSubtree: compute deferred top or bottom shift
LineBuilder->>Fragments: apply resolved subtree shift
LineBuilder->>StaticPositionMarkers: apply resolved subtree shift
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibWeb/Rust/src/layout/line_builder.rs`:
- Around line 514-525: Update the text-top and text-bottom calculations in the
relevant line-box alignment method to use the parent inline box’s font metrics
instead of self.containing_style(), and pass those parent metrics through
ancestor offset resolution as well. Preserve the existing offsets and sizing
calculations, then add a regression test covering nested inline boxes with
different parent and containing-block font metrics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27aa561c-6d93-409c-a4f8-32146c318154
📒 Files selected for processing (14)
Libraries/LibWeb/Rust/src/layout/line_builder.rsTests/LibWeb/Layout/expected/abspos-inline-containing-block-first-last-line-rule.txtTests/LibWeb/Layout/expected/vertical-align-bottom-atomic-inline-line-height.txtTests/LibWeb/Layout/expected/vertical-align-line-relative-subtrees.txtTests/LibWeb/Layout/expected/vertical-align-text-top-bottom.txtTests/LibWeb/Layout/input/vertical-align-bottom-atomic-inline-line-height.htmlTests/LibWeb/Layout/input/vertical-align-line-relative-subtrees.htmlTests/LibWeb/Layout/input/vertical-align-text-top-bottom.htmlTests/LibWeb/Ref/expected/wpt-import/css/CSS2/linebox/vertical-align-nested-top-001-ref.htmlTests/LibWeb/Ref/expected/wpt-import/css/CSS2/linebox/vertical-align-top-bottom-padding-ref.htmlTests/LibWeb/Ref/input/wpt-import/css/CSS2/linebox/vertical-align-nested-top-001.htmlTests/LibWeb/Ref/input/wpt-import/css/CSS2/linebox/vertical-align-top-bottom-padding.htmlTests/LibWeb/Text/expected/wpt-import/css/CSS2/linebox/vertical-align-top-bottom-001.txtTests/LibWeb/Text/input/wpt-import/css/CSS2/linebox/vertical-align-top-bottom-001.html
2464025 to
2e25d7a
Compare
Boxes with vertical-align: text-top or text-bottom were placed on the alphabetic baseline, because both keywords shared the fallback match arm that treats them as baseline. Align them with the top and bottom of the strut's content area instead, derived from the font ascent and descent around the line box baseline.
Boxes with vertical-align: top or bottom took part in choosing the line box baseline and were then positioned against the line box start, so a box taller than the rest of the line pushed the other content around and produced a line box of the wrong size. Per CSS2 such a box is the root of an aligned subtree that does not influence the baseline; collect those subtrees while positioning the line, grow the line box to fit the tallest of them, and shift each subtree and the remaining content into place once the final size is known.
2e25d7a to
f01c515
Compare
vertical-align: text-topandtext-bottomshared the fallback match arm that treats them asbaseline, so they were never really implemented. They now align with the top and bottom of the strut's content area.Separately, per CSS2 §10.8.1 a box with
vertical-align: toporbottomis the root of an aligned subtree that does not participate in choosing the line box baseline. We did the opposite, then positioned it against the line box block start, so a box taller than the rest of the line dragged the surrounding content around and produced a line box of the wrong size. The line builder now collects aligned subtrees, grows the line box to fit the tallest one, and shifts each subtree into place once the final size is known. Imports the WPTvertical-align-top-bottom-001test (20 subtests) plus two ref tests, and adds three layout tests; one existing expectation is rebaselined because a 60px bottom-aligned box with a 20px strut correctly grows the line box upward to 60px rather than 65px.Fixes the vertical alignment in the hero section on https://www.zeroramp.com/: