Fix vertical alignment at the root of an IFC (#31636)

At the root of an inline formatting context, we used its vertical-align in order to compute the strut. That was wrong, since vertical-align on a block container shouldn't affect the contents, it should only affect the alignment of the block container (if it's inline-level) within the parent IFC. This was only working well if the block container was block-level, since effective_vertical_align_for_inline_layout returned `baseline` for block-level boxes. Instead of the outer display type, this patch changes the logic to check whether we are at the root of the IFC.
servo · Mar 13, 2024 · 0860deb · 0860deb
1 parent 63527f5
commit 0860deb
Show file tree

Hide file tree

Showing 7 changed files with 223 additions and 58 deletions.
diff --git a/components/layout_2020/flow/inline.rs b/components/layout_2020/flow/inline.rs
@@ -76,12 +76,13 @@ use gfx::font::FontMetrics;
 use gfx::text::glyph::GlyphStore;
 use serde::Serialize;
 use servo_arc::Arc;
+use style::computed_values::vertical_align::T as VerticalAlign;
 use style::computed_values::white_space::T as WhiteSpace;
 use style::context::QuirksMode;
 use style::logical_geometry::WritingMode;
 use style::properties::ComputedValues;
-use style::values::computed::{Length, LengthPercentage};
-use style::values::generics::box_::{GenericVerticalAlign, VerticalAlignKeyword};
+use style::values::computed::Length;
+use style::values::generics::box_::VerticalAlignKeyword;
 use style::values::generics::text::LineHeight;
 use style::values::specified::text::{TextAlignKeyword, TextDecorationLine};
 use style::values::specified::{TextAlignLast, TextJustify};
@@ -1259,7 +1260,12 @@ impl<'a, 'b> InlineFormattingContextState<'a, 'b> {
         let strut_size = if using_fallback_font {
             // TODO(mrobinson): This value should probably be cached somewhere.
             let container_state = self.current_inline_container_state();
-            let mut block_size = container_state.get_block_size_contribution(&font_metrics);
+            let vertical_align = effective_vertical_align(
+                &container_state.style,
+                self.inline_box_state_stack.last().map(|c| &c.base),
+            );
+            let mut block_size =
+                container_state.get_block_size_contribution(vertical_align, &font_metrics);
             block_size.adjust_for_baseline_offset(container_state.baseline_offset);
             block_size
         } else if quirks_mode && !is_collapsible_whitespace {
@@ -1725,13 +1731,17 @@ impl InlineContainerState {
         let line_height = line_height(&style, &font_metrics);
 
         let mut baseline_offset = Au::zero();
-        let mut strut_block_sizes =
-            Self::get_block_sizes_with_style(&style, &font_metrics, line_height);
+        let mut strut_block_sizes = Self::get_block_sizes_with_style(
+            effective_vertical_align(&style, parent_container),
+            &style,
+            &font_metrics,
+            line_height,
+        );
         if let Some(parent_container) = parent_container {
             // The baseline offset from `vertical-align` might adjust where our block size contribution is
             // within the line.
             baseline_offset = parent_container.get_cumulative_baseline_offset_for_child(
-                style.effective_vertical_align_for_inline_layout(),
+                style.clone_vertical_align(),
                 &strut_block_sizes,
             );
             strut_block_sizes.adjust_for_baseline_offset(baseline_offset);
@@ -1756,11 +1766,11 @@ impl InlineContainerState {
     }
 
     fn get_block_sizes_with_style(
+        vertical_align: VerticalAlign,
         style: &ComputedValues,
         font_metrics: &FontMetrics,
         line_height: Length,
     ) -> LineBlockSizes {
-        let vertical_align = style.effective_vertical_align_for_inline_layout();
         if !is_baseline_relative(vertical_align) {
             return LineBlockSizes {
                 line_height,
@@ -1820,8 +1830,13 @@ impl InlineContainerState {
         }
     }
 
-    fn get_block_size_contribution(&self, font_metrics: &FontMetrics) -> LineBlockSizes {
+    fn get_block_size_contribution(
+        &self,
+        vertical_align: VerticalAlign,
+        font_metrics: &FontMetrics,
+    ) -> LineBlockSizes {
         Self::get_block_sizes_with_style(
+            vertical_align,
             &self.style,
             font_metrics,
             line_height(&self.style, font_metrics),
@@ -1830,47 +1845,48 @@ impl InlineContainerState {
 
     fn get_cumulative_baseline_offset_for_child(
         &self,
-        child_vertical_align: GenericVerticalAlign<LengthPercentage>,
+        child_vertical_align: VerticalAlign,
         child_block_size: &LineBlockSizes,
     ) -> Au {
-        let block_size = self.get_block_size_contribution(&self.font_metrics);
+        let block_size =
+            self.get_block_size_contribution(child_vertical_align.clone(), &self.font_metrics);
         self.baseline_offset +
             match child_vertical_align {
                 // `top` and `bottom are not actually relative to the baseline, but this value is unused
                 // in those cases.
                 // TODO: We should distinguish these from `baseline` in order to implement "aligned subtrees" properly.
                 // See https://drafts.csswg.org/css2/#aligned-subtree.
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Baseline) |
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Top) |
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Bottom) => Au::zero(),
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Sub) => Au::from_f32_px(
+                VerticalAlign::Keyword(VerticalAlignKeyword::Baseline) |
+                VerticalAlign::Keyword(VerticalAlignKeyword::Top) |
+                VerticalAlign::Keyword(VerticalAlignKeyword::Bottom) => Au::zero(),
+                VerticalAlign::Keyword(VerticalAlignKeyword::Sub) => Au::from_f32_px(
                     block_size
                         .resolve()
                         .scale_by(FONT_SUBSCRIPT_OFFSET_RATIO)
                         .px(),
                 ),
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Super) => -Au::from_f32_px(
+                VerticalAlign::Keyword(VerticalAlignKeyword::Super) => -Au::from_f32_px(
                     block_size
                         .resolve()
                         .scale_by(FONT_SUPERSCRIPT_OFFSET_RATIO)
                         .px(),
                 ),
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::TextTop) => {
+                VerticalAlign::Keyword(VerticalAlignKeyword::TextTop) => {
                     child_block_size.size_for_baseline_positioning.ascent - self.font_metrics.ascent
                 },
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Middle) => {
+                VerticalAlign::Keyword(VerticalAlignKeyword::Middle) => {
                     // "Align the vertical midpoint of the box with the baseline of the parent
                     // box plus half the x-height of the parent."
                     (child_block_size.size_for_baseline_positioning.ascent -
                         child_block_size.size_for_baseline_positioning.descent -
                         self.font_metrics.x_height)
                         .scale_by(0.5)
                 },
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::TextBottom) => {
+                VerticalAlign::Keyword(VerticalAlignKeyword::TextBottom) => {
                     self.font_metrics.descent -
                         child_block_size.size_for_baseline_positioning.descent
                 },
-                GenericVerticalAlign::Length(length_percentage) => {
+                VerticalAlign::Length(length_percentage) => {
                     Au::from_f32_px(-length_percentage.resolve(child_block_size.line_height).px())
                 },
             }
@@ -2156,11 +2172,25 @@ fn line_height(parent_style: &ComputedValues, font_metrics: &FontMetrics) -> Len
     }
 }
 
-fn is_baseline_relative(vertical_align: GenericVerticalAlign<LengthPercentage>) -> bool {
+fn effective_vertical_align(
+    style: &ComputedValues,
+    container: Option<&InlineContainerState>,
+) -> VerticalAlign {
+    if container.is_none() {
+        // If we are at the root of the inline formatting context, we shouldn't use the
+        // computed `vertical-align`, since it has no effect on the contents of this IFC
+        // (it can just affect how the block container is aligned within the parent IFC).
+        VerticalAlign::Keyword(VerticalAlignKeyword::Baseline)
+    } else {
+        style.clone_vertical_align()
+    }
+}
+
+fn is_baseline_relative(vertical_align: VerticalAlign) -> bool {
     !matches!(
         vertical_align,
-        GenericVerticalAlign::Keyword(VerticalAlignKeyword::Top) |
-            GenericVerticalAlign::Keyword(VerticalAlignKeyword::Bottom)
+        VerticalAlign::Keyword(VerticalAlignKeyword::Top) |
+            VerticalAlign::Keyword(VerticalAlignKeyword::Bottom)
     )
 }
 

diff --git a/components/layout_2020/flow/line.rs b/components/layout_2020/flow/line.rs
@@ -10,7 +10,7 @@ use gfx::font::FontMetrics;
 use gfx::text::glyph::GlyphStore;
 use servo_arc::Arc;
 use style::properties::ComputedValues;
-use style::values::computed::{Length, LengthPercentage};
+use style::values::computed::Length;
 use style::values::generics::box_::{GenericVerticalAlign, VerticalAlignKeyword};
 use style::values::generics::text::LineHeight;
 use style::values::specified::box_::DisplayOutside;
@@ -28,7 +28,7 @@ use crate::geom::{LogicalRect, LogicalVec2};
 use crate::positioned::{
     relative_adjustement, AbsolutelyPositionedBox, PositioningContext, PositioningContextLength,
 };
-use crate::style_ext::{ComputedValuesExt, PaddingBorderMargin};
+use crate::style_ext::PaddingBorderMargin;
 use crate::ContainingBlock;
 
 pub(super) struct LineMetrics {
@@ -226,16 +226,10 @@ impl TextRunLineItem {
         // The block start of the TextRun is often zero (meaning it has the same font metrics as the
         // inline box's strut), but for children of the inline formatting context root or for
         // fallback fonts that use baseline relatve alignment, it might be different.
-        let mut start_corner = &LogicalVec2 {
+        let start_corner = &LogicalVec2 {
             inline: state.inline_position,
             block: (state.baseline_offset - self.font_metrics.ascent).into(),
         } - &state.parent_offset;
-        if !is_baseline_relative(
-            self.parent_style
-                .effective_vertical_align_for_inline_layout(),
-        ) {
-            start_corner.block = Length::zero();
-        }
 
         let rect = LogicalRect {
             start_corner,
@@ -428,12 +422,11 @@ impl InlineBoxLineItem {
     /// Given the state for a line item layout and the space above the baseline for this inline
     /// box, find the block start position relative to the line block start position.
     fn calculate_block_start(&self, state: &LineItemLayoutState, space_above_baseline: Au) -> Au {
-        let vertical_align = self.style.effective_vertical_align_for_inline_layout();
         let line_gap = self.font_metrics.line_gap;
 
         // The baseline offset that we have in `Self::baseline_offset` is relative to the line
         // baseline, so we need to make it relative to the line block start.
-        match vertical_align {
+        match self.style.clone_vertical_align() {
             GenericVerticalAlign::Keyword(VerticalAlignKeyword::Top) => {
                 let line_height: Au = line_height(&self.style, &self.font_metrics).into();
                 (line_height - line_gap).scale_by(0.5)
@@ -586,14 +579,6 @@ impl FloatLineItem {
     }
 }
 
-fn is_baseline_relative(vertical_align: GenericVerticalAlign<LengthPercentage>) -> bool {
-    !matches!(
-        vertical_align,
-        GenericVerticalAlign::Keyword(VerticalAlignKeyword::Top) |
-            GenericVerticalAlign::Keyword(VerticalAlignKeyword::Bottom)
-    )
-}
-
 fn line_height(parent_style: &ComputedValues, font_metrics: &FontMetrics) -> Length {
     let font_size = parent_style.get_font().font_size.computed_size();
     match parent_style.get_inherited_text().line_height {

diff --git a/components/layout_2020/style_ext.rs b/components/layout_2020/style_ext.rs
@@ -13,9 +13,8 @@ use style::properties::longhands::column_span::computed_value::T as ColumnSpan;
 use style::properties::ComputedValues;
 use style::values::computed::image::Image as ComputedImageLayer;
 use style::values::computed::{Length, LengthPercentage, NonNegativeLengthPercentage, Size};
-use style::values::generics::box_::{GenericVerticalAlign, Perspective, VerticalAlignKeyword};
+use style::values::generics::box_::Perspective;
 use style::values::generics::length::MaxSize;
-use style::values::specified::box_::DisplayOutside as StyloDisplayOutside;
 use style::values::specified::{box_ as stylo, Overflow};
 use style::Zero;
 use webrender_api as wr;
@@ -189,7 +188,6 @@ pub(crate) trait ComputedValuesExt {
     fn establishes_containing_block_for_all_descendants(&self) -> bool;
     fn background_is_transparent(&self) -> bool;
     fn get_webrender_primitive_flags(&self) -> wr::PrimitiveFlags;
-    fn effective_vertical_align_for_inline_layout(&self) -> GenericVerticalAlign<LengthPercentage>;
 }
 
 impl ComputedValuesExt for ComputedValues {
@@ -560,18 +558,6 @@ impl ComputedValuesExt for ComputedValues {
             BackfaceVisiblity::Hidden => wr::PrimitiveFlags::empty(),
         }
     }
-
-    /// Get the effective `vertical-align` property for inline layout. Essentially, if this style
-    /// has outside block display, this is the inline formatting context root and `vertical-align`
-    /// doesn't come into play for inline layout.
-    fn effective_vertical_align_for_inline_layout(&self) -> GenericVerticalAlign<LengthPercentage> {
-        match self.clone_display().outside() {
-            StyloDisplayOutside::Block => {
-                GenericVerticalAlign::Keyword(VerticalAlignKeyword::Baseline)
-            },
-            _ => self.clone_vertical_align(),
-        }
-    }
 }
 
 impl From<stylo::Display> for Display {

diff --git a/tests/wpt/meta/MANIFEST.json b/tests/wpt/meta/MANIFEST.json
@@ -63058,6 +63058,19 @@
         {}
        ]
       ],
+      "vertical-align-122.xht": [
+       "1d7c180f197b9262dd98aa162595b3eb914f60d1",
+       [
+        null,
+        [
+         [
+          "/css/CSS2/linebox/vertical-align-122-ref.xht",
+          "=="
+         ]
+        ],
+        {}
+       ]
+      ],
       "vertical-align-applies-to-001.xht": [
        "fca1480f7367209223f370ff01acbc45ef7b3778",
        [
@@ -371510,6 +371523,10 @@
        "c0db9e0e8bbc2bbec91ffaa564e326b4744dd2fd",
        []
       ],
+      "vertical-align-122-ref.xht": [
+       "86b4856266285a3a9b66b5c426384d2699cbaceb",
+       []
+      ],
       "vertical-align-applies-to-001-ref.xht": [
        "8ed51e0c21a0613e67c49e3e36e488e176027165",
        []

diff --git a/tests/wpt/meta/css/CSS2/linebox/inline-formatting-context-011.xht.ini b/tests/wpt/meta/css/CSS2/linebox/inline-formatting-context-011.xht.ini
diff --git a/tests/wpt/tests/css/CSS2/linebox/vertical-align-122-ref.xht b/tests/wpt/tests/css/CSS2/linebox/vertical-align-122-ref.xht
@@ -0,0 +1,87 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml">
+ <head>
+  <title>CSS Reftest Reference</title>
+  <link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com"/>
+
+  <link rel="stylesheet" type="text/css" href="/fonts/ahem.css" />
+  <style type="text/css">
+  <![CDATA[
+  body {
+    position: relative;
+    font-size: 20px;
+  }
+  .person {
+    position: absolute;
+    width: 6em;
+    height: 8.9em;
+  }
+  .face {
+    position: relative;
+    height: 4.2em;
+    background: currentcolor;
+  }
+  .face > div {
+    position: absolute;
+    background: white;
+  }
+  .face > .mouth {
+    left: 2em;
+    top: 3em;
+    width: 2em;
+    height: .2em;
+  }
+  .face > .eye {
+    top: 1em;
+    width: 1em;
+    height: 1.2em;
+  }
+  .face > .eye.left {
+    left: 1em;
+  }
+  .face > .eye.right {
+    right: 1em;
+  }
+  .torso {
+    margin-top: 0.5em;
+    height: 2.2em;
+    border: 1em solid;
+  }
+  ]]>
+  </style>
+ </head>
+ <body>
+  <div class="person" style="left: 0em; top: 0em">
+    <div class="face">
+      <div class="eye left"></div>
+      <div class="eye right"></div>
+      <div class="mouth"></div>
+    </div>
+    <div class="torso"></div>
+  </div>
+  <div class="person" style="left: 7em; top: 0em">
+    <div class="face">
+      <div class="eye left"></div>
+      <div class="eye right"></div>
+      <div class="mouth"></div>
+    </div>
+    <div class="torso"></div>
+  </div>
+  <div class="person" style="left: 14em; top: .5em">
+    <div class="face">
+      <div class="eye left"></div>
+      <div class="eye right"></div>
+      <div class="mouth"></div>
+    </div>
+    <div class="torso"></div>
+  </div>
+  <div class="person" style="left: 21em; top: .5em">
+    <div class="face">
+      <div class="eye left"></div>
+      <div class="eye right"></div>
+      <div class="mouth"></div>
+    </div>
+    <div class="torso"></div>
+  </div>
+ </body>
+</html>