layout: Ensure empty list items are at least as tall as outside marke…

…rs (#32152)

While <https://drafts.csswg.org/css-lists/#list-style-position-property> says:

> The size or contents of the marker box may affect the height of the
> principal block box and/or the height of its first line box, and in some
> cases may cause the creation of a new line box; this interaction is also
> not defined.

All other browsers ensure that the first line of list item content is
the same block size as the marker. Doing this is complicated, but we can
ensure that the entire list item is at least as tall as the marker. This
should handle the majority of cases and we can make refinements later
for stranger situations, such as when the marker is very tall.

Co-authored-by: Oriol Brufau <obrufau@igalia.com>

components/layout_2020/flow/construct.rs

@@ -790,17 +790,17 @@ where
                         &PseudoElement::ServoLegacyText, // FIMXE: use `PseudoElement::Marker` when we add it
                         &info.style,
                     );
-                let info = info.new_replacing_style(marker_style.clone());
                 let contents = NonReplacedContents::OfPseudoElement(contents);
                 let block_container = BlockContainer::construct(
                     context,
-                    &info,
+                    &info.new_replacing_style(marker_style.clone()),
                     contents,
                     TextDecorationLine::empty(),
                     false, /* is_list_item */
                 );
                 ArcRefCell::new(BlockLevelBox::OutsideMarker(OutsideMarker {
-                    style: marker_style,
+                    marker_style,
+                    list_item_style: info.style.clone(),
                     block_container,
                 }))
             },

components/layout_2020/flow/mod.rs

@@ -27,7 +27,7 @@ use crate::formatting_contexts::{
     Baselines, IndependentFormattingContext, IndependentLayout, NonReplacedFormattingContext,
 };
 use crate::fragment_tree::{
-    BaseFragmentInfo, BoxFragment, CollapsedBlockMargins, CollapsedMargin, Fragment,
+    BaseFragmentInfo, BoxFragment, CollapsedBlockMargins, CollapsedMargin, Fragment, FragmentFlags,
 };
 use crate::geom::{AuOrAuto, LogicalRect, LogicalSides, LogicalVec2};
 use crate::positioned::{AbsolutelyPositionedBox, PositioningContext, PositioningContextLength};
@@ -212,7 +212,9 @@ struct CollapsibleWithParentStartMargin(bool);
 #[derive(Debug, Serialize)]
 pub(crate) struct OutsideMarker {
     #[serde(skip_serializing)]
-    pub style: Arc<ComputedValues>,
+    pub marker_style: Arc<ComputedValues>,
+    #[serde(skip_serializing)]
+    pub list_item_style: Arc<ComputedValues>,
     pub block_container: BlockContainer,
 }
 
@@ -228,15 +230,16 @@ impl OutsideMarker {
         let content_sizes = self
             .block_container
             .inline_content_sizes(layout_context, containing_block.style.writing_mode);
-        let containing_block = ContainingBlock {
+        let containing_block_for_children = ContainingBlock {
             inline_size: content_sizes.max_content,
             block_size: AuOrAuto::auto(),
-            style: &self.style,
+            style: &self.marker_style,
         };
+
         let flow_layout = self.block_container.layout(
             layout_context,
             positioning_context,
-            &containing_block,
+            &containing_block_for_children,
             sequential_layout_state,
             collapsible_with_parent_start_margin.unwrap_or(CollapsibleWithParentStartMargin(false)),
         );
@@ -257,20 +260,33 @@ impl OutsideMarker {
             },
         );
 
+        // Position the marker beyond the inline start of the border box list item. This needs to
+        // take into account the border and padding of the item.
+        //
+        // TODO: This is the wrong containing block, as it should be the containing block of the
+        // parent of this list item. What this means in practice is that the writing mode could be
+        // wrong and padding defined as a percentage will be resolved incorrectly.
+        let pbm_of_list_item = self.list_item_style.padding_border_margin(containing_block);
         let content_rect = LogicalRect {
             start_corner: LogicalVec2 {
-                inline: -max_inline_size,
+                inline: -max_inline_size -
+                    (pbm_of_list_item.border.inline_start +
+                        pbm_of_list_item.padding.inline_start)
+                        .into(),
                 block: Zero::zero(),
             },
             size: LogicalVec2 {
                 inline: max_inline_size,
-                block: Zero::zero(),
+                block: flow_layout.content_block_size,
             },
         };
 
+        let mut base_fragment_info = BaseFragmentInfo::anonymous();
+        base_fragment_info.flags |= FragmentFlags::IS_OUTSIDE_LIST_ITEM_MARKER;
+
         Fragment::Box(BoxFragment::new(
-            BaseFragmentInfo::anonymous(),
-            self.style.clone(),
+            base_fragment_info,
+            self.marker_style.clone(),
             flow_layout.fragments,
             content_rect,
             LogicalSides::zero(),
@@ -1604,6 +1620,12 @@ struct PlacementState {
     current_block_direction_position: Au,
     inflow_baselines: Baselines,
     is_inline_block_context: bool,
+
+    /// If this [`PlacementState`] is laying out a list item with an outside marker. Record the
+    /// block size of that marker, because the content block size of the list item needs to be at
+    /// least as tall as the marker size -- even though the marker doesn't advance the block
+    /// position of the placement.
+    marker_block_size: Option<Au>,
 }
 
 impl PlacementState {
@@ -1622,6 +1644,7 @@ impl PlacementState {
             current_block_direction_position: Au::zero(),
             inflow_baselines: Baselines::default(),
             is_inline_block_context,
+            marker_block_size: None,
         }
     }
 
@@ -1665,10 +1688,29 @@ impl PlacementState {
     ) {
         match fragment {
             Fragment::Box(fragment) => {
+                // If this child is a marker positioned outside of a list item, then record its
+                // size, but also ensure that it doesn't advance the block position of the placment.
+                // This ensures item content is placed next to the marker.
+                //
+                // This is a pretty big hack because it doesn't properly handle all interactions
+                // between the marker and the item. For instance the marker should be positioned at
+                // the baseline of list item content and the first line of the item content should
+                // be at least as tall as the marker -- not the entire list item itself.
+                let is_outside_marker = fragment
+                    .base
+                    .flags
+                    .contains(FragmentFlags::IS_OUTSIDE_LIST_ITEM_MARKER);
+                if is_outside_marker {
+                    assert!(self.marker_block_size.is_none());
+                    self.marker_block_size = Some(fragment.content_rect.size.block.into());
+                    return;
+                }
+
                 let fragment_block_margins = &fragment.block_margins_collapsed_with_children;
                 let mut fragment_block_size = fragment.padding.block_sum() +
                     fragment.border.block_sum() +
                     fragment.content_rect.size.block.into();
+
                 // We use `last_in_flow_margin_collapses_with_parent_end_margin` to implement
                 // this quote from https://drafts.csswg.org/css2/#collapsing-margins
                 // > If the top and bottom margins of an element with clearance are adjoining,
@@ -1747,10 +1789,23 @@ impl PlacementState {
             self.current_block_direction_position += self.current_margin.solve();
             self.current_margin = CollapsedMargin::zero();
         }
+        let (total_block_size, collapsed_through) = match self.marker_block_size {
+            Some(marker_block_size) => (
+                self.current_block_direction_position.max(marker_block_size),
+                // If this is a list item (even empty) with an outside marker, then it
+                // should not collapse through.
+                false,
+            ),
+            None => (
+                self.current_block_direction_position,
+                self.next_in_flow_margin_collapses_with_parent_start_margin,
+            ),
+        };
+
         (
-            self.current_block_direction_position.into(),
+            total_block_size.into(),
             CollapsedBlockMargins {
-                collapsed_through: self.next_in_flow_margin_collapses_with_parent_start_margin,
+                collapsed_through,
                 start: self.start_margin,
                 end: self.current_margin,
             },

components/layout_2020/fragment_tree/base_fragment.rs

@@ -91,6 +91,9 @@ bitflags! {
         /// Whether or not this Fragment was created to contain a replaced element or is
         /// a replaced element.
         const IS_REPLACED = 0b00000100;
+        /// Whether or not this Fragment was created to contain a list item marker
+        /// with a used value of `list-style-position: outside`.
+        const IS_OUTSIDE_LIST_ITEM_MARKER = 0b00001000;
     }
 }
 

tests/wpt/meta/css/CSS2/text/text-indent-applies-to-003.xht.ini

@@ -0,0 +1,2 @@
+[text-indent-applies-to-003.xht]
+  expected: FAIL

tests/wpt/meta/css/css-align/blocks/align-content-block-002.html.ini

@@ -29,5 +29,20 @@
   [.test 16: space-around]
     expected: FAIL
 
-  [.test 17: normal]
+  [.test 1: start]
+    expected: FAIL
+
+  [.test 4: baseline]
+    expected: FAIL
+
+  [.test 6: flex-start]
+    expected: FAIL
+
+  [.test 8: unsafe start]
+    expected: FAIL
+
+  [.test 11: safe start]
+    expected: FAIL
+
+  [.test 15: space-between]
     expected: FAIL

tests/wpt/meta/html/rendering/widgets/button-layout/display-other.html.ini

@@ -46,3 +46,6 @@
 
   [display: table-caption]
     expected: FAIL
+
+  [display: list-item]
+    expected: FAIL