[LayoutNG] Fix list marker positioning when the first child is block

This patch fixes list marker positioning when the first child of
the list item is block-level, and the child does not produce a
baseline. Before this patch, list marker is positioned at the
synthesized baseline. This is not well-defined, but 3 browsers
positions the list marker at the first child that has a baseline.
This is filed to CSS WG at
https://github.com/w3c/csswg-drafts/issues/2417.

By doing so, a new case appears where there are no children that
has a baseline. This case is also not well-defined and non-
interoperable. Blink/Edge/WebKit creates an empty line box in
this case, but this isn't easy as we have to insert an empty
line box before the children we have already laid out. This
patch follows Gecko behavior to position the list marker at the
top of the list item.

Also, whether list marker should affect block size or not is not
defined and not interoperable. For now, this patch includes the
list marker block size into list item's block size because it
looks more reasonable to me, and Blink/Edge/WebKit do so. This
is filed as:
https://github.com/w3c/csswg-drafts/issues/2418

SetListMarkerPosition() is a left over in the last patch and
therefore this patch removed it.

Some tests turn to good, though they need to rebaseline due to
list marker image differences.

Bug: 725277
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: If47ab7daea214c0593a917a0c8f505274dae5ca2
Reviewed-on: https://chromium-review.googlesource.com/952843
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541864}

third_party/WebKit/Source/core/layout/ng/list/ng_list_layout_algorithm.cc

@@ -18,38 +18,42 @@ namespace blink {
 namespace {
 
 std::pair<LayoutUnit, LayoutUnit> InlineMarginsForOutside(
-    const NGFragment& list_marker,
-    const NGConstraintSpace& constraint_space) {
-  DCHECK(&list_marker);
+    const ComputedStyle& style,
+    LayoutUnit list_marker_inline_size) {
   bool is_image = false;  // TODO(kojii): implement
-  return LayoutListMarker::InlineMarginsForOutside(
-      list_marker.Style(), is_image, list_marker.InlineSize());
+  return LayoutListMarker::InlineMarginsForOutside(style, is_image,
+                                                   list_marker_inline_size);
 }
 
 }  // namespace
 
-void NGListLayoutAlgorithm::SetListMarkerPosition(
-    const NGConstraintSpace& constraint_space,
-    const NGLineInfo& line_info,
-    LayoutUnit line_width,
-    NGLineBoxFragmentBuilder::Child* list_marker_child) {
-  DCHECK(list_marker_child->PhysicalFragment());
-  NGFragment list_marker_fragment(constraint_space.GetWritingMode(),
-                                  *list_marker_child->PhysicalFragment());
-  auto margins =
-      InlineMarginsForOutside(list_marker_fragment, constraint_space);
-  LayoutUnit line_offset = IsLtr(line_info.BaseDirection())
-                               ? margins.first
-                               : line_width + margins.second;
-  list_marker_child->offset.inline_offset = line_offset;
-}
-
-void NGListLayoutAlgorithm::AddListMarkerForBlockContent(
+bool NGListLayoutAlgorithm::AddListMarkerForBlockContent(
     NGBlockNode list_marker_node,
     const NGConstraintSpace& constraint_space,
     const NGPhysicalFragment& content,
     NGLogicalOffset offset,
     NGFragmentBuilder* container_builder) {
+  // Compute the baseline of the child content.
+  FontBaseline baseline_type =
+      IsHorizontalWritingMode(constraint_space.GetWritingMode())
+          ? kAlphabeticBaseline
+          : kIdeographicBaseline;
+  NGLineHeightMetrics content_metrics;
+  if (content.IsLineBox()) {
+    content_metrics = ToNGPhysicalLineBoxFragment(content).Metrics();
+  } else {
+    NGBoxFragment content_fragment(constraint_space.GetWritingMode(),
+                                   ToNGPhysicalBoxFragment(content));
+    content_metrics = content_fragment.BaselineMetricsWithoutSynthesize(
+        {NGBaselineAlgorithmType::kFirstLine, baseline_type}, constraint_space);
+
+    // If this child content does not have any line boxes, the list marker
+    // should be aligned to the first line box of next child.
+    // https://github.com/w3c/csswg-drafts/issues/2417
+    if (content_metrics.IsEmpty())
+      return false;
+  }
+
   // Layout the list marker.
   scoped_refptr<NGLayoutResult> list_marker_layout_result =
       list_marker_node.LayoutAtomicInline(constraint_space,
@@ -62,29 +66,16 @@ void NGListLayoutAlgorithm::AddListMarkerForBlockContent(
   // Compute the inline offset of the marker from its margins.
   // The marker is relative to the border box of the list item and has nothing
   // to do with the content offset.
-  auto margins =
-      InlineMarginsForOutside(list_marker_fragment, constraint_space);
+  auto margins = InlineMarginsForOutside(list_marker_fragment.Style(),
+                                         list_marker_fragment.InlineSize());
   offset.inline_offset = margins.first;
 
   // Compute the block offset of the marker by aligning the baseline of the
   // marker to the first baseline of the content.
-  FontBaseline baseline_type =
-      IsHorizontalWritingMode(constraint_space.GetWritingMode())
-          ? kAlphabeticBaseline
-          : kIdeographicBaseline;
   NGLineHeightMetrics list_marker_metrics =
       list_marker_fragment.BaselineMetrics(
           {NGBaselineAlgorithmType::kAtomicInline, baseline_type},
           constraint_space);
-  NGLineHeightMetrics content_metrics;
-  if (content.IsLineBox()) {
-    content_metrics = ToNGPhysicalLineBoxFragment(content).Metrics();
-  } else {
-    NGBoxFragment content_fragment(constraint_space.GetWritingMode(),
-                                   ToNGPhysicalBoxFragment(content));
-    content_metrics = content_fragment.BaselineMetrics(
-        {NGBaselineAlgorithmType::kFirstLine, baseline_type}, constraint_space);
-  }
 
   // |offset.block_offset| is at the top of the content. Adjust it to the top of
   // the list marker by adding the differences of the ascent between content's
@@ -93,6 +84,35 @@ void NGListLayoutAlgorithm::AddListMarkerForBlockContent(
 
   DCHECK(container_builder);
   container_builder->AddChild(std::move(list_marker_layout_result), offset);
+  return true;
+}
+
+LayoutUnit NGListLayoutAlgorithm::AddListMarkerWithoutLineBoxes(
+    NGBlockNode list_marker_node,
+    const NGConstraintSpace& constraint_space,
+    NGFragmentBuilder* container_builder) {
+  // Layout the list marker.
+  scoped_refptr<NGLayoutResult> list_marker_layout_result =
+      list_marker_node.LayoutAtomicInline(constraint_space,
+                                          constraint_space.UseFirstLineStyle());
+  DCHECK(list_marker_layout_result->PhysicalFragment());
+  const NGPhysicalBoxFragment& list_marker_physical_fragment =
+      ToNGPhysicalBoxFragment(*list_marker_layout_result->PhysicalFragment());
+  NGLogicalSize size = list_marker_physical_fragment.Size().ConvertToLogical(
+      constraint_space.GetWritingMode());
+
+  // Compute the inline offset of the marker from its margins.
+  auto margins = InlineMarginsForOutside(list_marker_physical_fragment.Style(),
+                                         size.inline_size);
+
+  // When there are no line boxes, marker is top-aligned to the list item.
+  // https://github.com/w3c/csswg-drafts/issues/2417
+  NGLogicalOffset offset(margins.first, LayoutUnit());
+
+  DCHECK(container_builder);
+  container_builder->AddChild(std::move(list_marker_layout_result), offset);
+
+  return size.block_size;
 }
 
 }  // namespace blink

third_party/WebKit/Source/core/layout/ng/list/ng_list_layout_algorithm.h

@@ -11,29 +11,29 @@
 
 namespace blink {
 
-class LayoutUnit;
 class NGConstraintSpace;
-class NGLineBoxFragmentBuilder;
-class NGLineInfo;
 
 // Algorithm to layout lists and list-items.
 // TODO(kojii): This isn't a real NGLayoutAlgorithm yet. Consider restructuring
 // or renaming.
 class CORE_EXPORT NGListLayoutAlgorithm final {
  public:
-  // Compute and set the inline position to an outside list marker for a line
-  // box.
-  static void SetListMarkerPosition(const NGConstraintSpace&,
-                                    const NGLineInfo&,
-                                    LayoutUnit line_width,
-                                    NGLineBoxFragmentBuilder::Child*);
-
   // Add a fragment for an outside list marker for a block content.
-  static void AddListMarkerForBlockContent(NGBlockNode,
+  // Returns true if the list marker was successfully added. False indicates
+  // that the child content does not have a baseline to align to, and that
+  // caller should try next child, or "WithoutLineBoxes" version.
+  static bool AddListMarkerForBlockContent(NGBlockNode,
                                            const NGConstraintSpace&,
                                            const NGPhysicalFragment&,
                                            NGLogicalOffset,
                                            NGFragmentBuilder*);
+
+  // Add a fragment for an outside list marker when the list item has no line
+  // boxes.
+  // Returns the block size of the list marker.
+  static LayoutUnit AddListMarkerWithoutLineBoxes(NGBlockNode,
+                                                  const NGConstraintSpace&,
+                                                  NGFragmentBuilder*);
 };
 
 }  // namespace blink

third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc

@@ -529,6 +529,12 @@ scoped_refptr<NGLayoutResult> NGBlockLayoutAlgorithm::Layout() {
     }
   }
 
+  // List markers should have been positioned if we had line boxes, or boxes
+  // that have line boxes. If there were no line boxes, position without line
+  // boxes.
+  if (container_builder_.UnpositionedListMarker() && node_.IsListItem())
+    PositionListMarkerWithoutLineBoxes();
+
   container_builder_.SetEndMarginStrut(end_margin_strut);
   container_builder_.SetIntrinsicBlockSize(intrinsic_block_size_);
   container_builder_.SetPadding(ComputePadding(ConstraintSpace(), Style()));
@@ -1830,9 +1836,43 @@ void NGBlockLayoutAlgorithm::PositionListMarker(
       return;
     container_builder_.SetUnpositionedListMarker(NGBlockNode(nullptr));
   }
-  NGListLayoutAlgorithm::AddListMarkerForBlockContent(
-      list_marker_node, constraint_space_, *layout_result.PhysicalFragment(),
-      content_offset, &container_builder_);
+  if (NGListLayoutAlgorithm::AddListMarkerForBlockContent(
+          list_marker_node, constraint_space_,
+          *layout_result.PhysicalFragment(), content_offset,
+          &container_builder_))
+    return;
+
+  // If the list marker could not be positioned against this child because it
+  // does not have the baseline to align to, keep it as unpositioned and try
+  // the next child.
+  container_builder_.SetUnpositionedListMarker(list_marker_node);
+}
+
+void NGBlockLayoutAlgorithm::PositionListMarkerWithoutLineBoxes() {
+  DCHECK(node_.IsListItem());
+  DCHECK(container_builder_.UnpositionedListMarker());
+
+  // Position the list marker without aligning to line boxes.
+  LayoutUnit marker_block_size =
+      NGListLayoutAlgorithm::AddListMarkerWithoutLineBoxes(
+          container_builder_.UnpositionedListMarker(), constraint_space_,
+          &container_builder_);
+  container_builder_.SetUnpositionedListMarker(NGBlockNode(nullptr));
+
+  // Whether the list marker should affect the block size or not is not
+  // well-defined, but 3 out of 4 impls do.
+  // https://github.com/w3c/csswg-drafts/issues/2418
+  //
+  // TODO(kojii): Since this makes this block non-empty, it's probably better to
+  // resolve BFC offset if not done yet, but that involves additional complexity
+  // without knowing how much this is needed. For now, include the marker into
+  // the block size only if BFC was resolved.
+  if (container_builder_.BfcOffset()) {
+    intrinsic_block_size_ = std::max(marker_block_size, intrinsic_block_size_);
+    container_builder_.SetIntrinsicBlockSize(intrinsic_block_size_);
+    container_builder_.SetBlockSize(
+        std::max(marker_block_size, container_builder_.Size().block_size));
+  }
 }
 
 }  // namespace blink

third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h

@@ -191,6 +191,9 @@ class CORE_EXPORT NGBlockLayoutAlgorithm
   // Positions a list marker for the specified block content.
   void PositionListMarker(const NGLayoutResult&, const NGLogicalOffset&);
 
+  // Positions a list marker when the block does not have any line boxes.
+  void PositionListMarkerWithoutLineBoxes();
+
   // Calculates logical offset for the current fragment using either {@code
   // intrinsic_block_size_} when the fragment doesn't know it's offset or
   // {@code known_fragment_offset} if the fragment knows it's offset

third_party/WebKit/Source/core/layout/ng/ng_box_fragment.cc

@@ -13,16 +13,13 @@
 
 namespace blink {
 
-NGLineHeightMetrics NGBoxFragment::BaselineMetrics(
+NGLineHeightMetrics NGBoxFragment::BaselineMetricsWithoutSynthesize(
     const NGBaselineRequest& request,
     const NGConstraintSpace& constraint_space) const {
   const auto& physical_fragment = ToNGPhysicalBoxFragment(physical_fragment_);
-
-  LayoutBox* layout_box = ToLayoutBox(physical_fragment_.GetLayoutObject());
   bool is_parallel_writing_mode =
-      IsHorizontalWritingMode(constraint_space.GetWritingMode()) ==
-      layout_box->IsHorizontalWritingMode();
-
+      IsParallelWritingMode(constraint_space.GetWritingMode(),
+                            physical_fragment.Style().GetWritingMode());
   if (is_parallel_writing_mode) {
     // Find the baseline from the computed results.
     if (const NGBaseline* baseline = physical_fragment.Baseline(request)) {
@@ -32,6 +29,7 @@ NGLineHeightMetrics NGBoxFragment::BaselineMetrics(
       // For replaced elements, inline-block elements, and inline-table
       // elements, the height is the height of their margin box.
       // https://drafts.csswg.org/css2/visudet.html#line-height
+      LayoutBox* layout_box = ToLayoutBox(physical_fragment_.GetLayoutObject());
       if (layout_box->IsAtomicInlineLevel()) {
         ascent += layout_box->MarginOver();
         descent += layout_box->MarginUnder();
@@ -41,12 +39,25 @@ NGLineHeightMetrics NGBoxFragment::BaselineMetrics(
     }
   }
 
+  return NGLineHeightMetrics();
+}
+
+NGLineHeightMetrics NGBoxFragment::BaselineMetrics(
+    const NGBaselineRequest& request,
+    const NGConstraintSpace& constraint_space) const {
+  NGLineHeightMetrics metrics =
+      BaselineMetricsWithoutSynthesize(request, constraint_space);
+  if (!metrics.IsEmpty())
+    return metrics;
+
   // The baseline type was not found. This is either this box should synthesize
   // box-baseline without propagating from children, or caller forgot to add
   // baseline requests to constraint space when it called Layout().
   LayoutUnit block_size = BlockSize();
 
+  const auto& physical_fragment = ToNGPhysicalBoxFragment(physical_fragment_);
   const ComputedStyle& style = physical_fragment.Style();
+  LayoutBox* layout_box = ToLayoutBox(physical_fragment_.GetLayoutObject());
   if (style.HasAppearance() &&
       !LayoutTheme::GetTheme().IsControlContainer(style.Appearance())) {
     return NGLineHeightMetrics(
@@ -57,6 +68,9 @@ NGLineHeightMetrics NGBoxFragment::BaselineMetrics(
 
   // If atomic inline, use the margin box. See above.
   if (layout_box->IsAtomicInlineLevel()) {
+    bool is_parallel_writing_mode =
+        IsParallelWritingMode(constraint_space.GetWritingMode(),
+                              physical_fragment.Style().GetWritingMode());
     if (is_parallel_writing_mode)
       block_size += layout_box->MarginLogicalHeight();
     else

third_party/WebKit/Source/core/layout/ng/ng_box_fragment.h

@@ -26,6 +26,13 @@ class CORE_EXPORT NGBoxFragment final : public NGFragment {
   //
   // Baseline requests must be added to constraint space when this fragment was
   // laid out.
+  //
+  // The "WithoutSynthesize" version returns an empty metrics if this box does
+  // not have any baselines, while the other version synthesize the baseline
+  // from the box.
+  NGLineHeightMetrics BaselineMetricsWithoutSynthesize(
+      const NGBaselineRequest&,
+      const NGConstraintSpace&) const;
   NGLineHeightMetrics BaselineMetrics(const NGBaselineRequest&,
                                       const NGConstraintSpace&) const;
 };