[LayoutNG] Fix line-height quirks for atomic inline

This patch fixes line-height quirks mode to include strut
for close tags only when the end-side of the inline box is
not empty. It used to include when |needs_box_fragment|,
which is set under many more conditions.

Normally, when the emptiness (and thus the strut) matters,
open and close are in the same line. <span><br></span> or
<span><div></div></span> can make this situation.

It involved two more changes:
1. NGInlineItem.IsEmptyItem is used to determine the block
   emptiness, while NGLineBreaker had another code to
   determine line emptiness, using slightly different
   conditions. The two logics were merged, stored in
   NGInlineItem.
2. It revealed a problem in 'vertical-align' where it does
   not include inline boxes for 'top' and 'bottom' align.
   This is fixed in this CL.

Bug: 636993
Change-Id: I2d609580c96371e702dd365737e8d704b0fcccd8
Reviewed-on: https://chromium-review.googlesource.com/c/1302173Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603642}

third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

@@ -265,7 +265,6 @@ crbug.com/591099 external/wpt/picture-in-picture/picture-in-picture-window.html
 crbug.com/591099 external/wpt/picture-in-picture/request-picture-in-picture-twice.html [ Pass ]
 crbug.com/591099 external/wpt/picture-in-picture/request-picture-in-picture.html [ Pass ]
 crbug.com/591099 external/wpt/picture-in-picture/shadow-dom.html [ Pass ]
-crbug.com/591099 external/wpt/quirks/line-height-calculation.html [ Failure ]
 crbug.com/591099 external/wpt/requestidlecallback/callback-iframe.html [ Pass Timeout ]
 crbug.com/591099 external/wpt/service-workers/service-worker/navigation-preload/broken-chunked-encoding.https.html [ Pass ]
 crbug.com/591099 external/wpt/service-workers/service-worker/update-after-navigation-redirect.https.html [ Pass ]

third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.cc

@@ -533,7 +533,7 @@ NGInlineLayoutStateStack::ApplyBaselineShift(
   // |pending_descendants|.
   LayoutUnit baseline_shift;
   if (!box->pending_descendants.IsEmpty()) {
-    NGLineHeightMetrics max = box->MetricsForTopAndBottomAlign();
+    NGLineHeightMetrics max = MetricsForTopAndBottomAlign(*box, *line_box);
     for (NGPendingPositions& child : box->pending_descendants) {
       // In quirks mode, metrics is empty if no content.
       if (child.metrics.IsEmpty())
@@ -646,30 +646,42 @@ NGInlineLayoutStateStack::ApplyBaselineShift(
   return kPositionNotPending;
 }
 
-NGLineHeightMetrics NGInlineBoxState::MetricsForTopAndBottomAlign() const {
+NGLineHeightMetrics NGInlineLayoutStateStack::MetricsForTopAndBottomAlign(
+    const NGInlineBoxState& box,
+    const NGLineBoxFragmentBuilder::ChildList& line_box) const {
+  DCHECK(!box.pending_descendants.IsEmpty());
+
   // |metrics| is the bounds of "aligned subtree", that is, bounds of
   // descendants that are not 'vertical-align: top' nor 'bottom'.
   // https://drafts.csswg.org/css2/visudet.html#propdef-vertical-align
-  NGLineHeightMetrics box = metrics;
+  NGLineHeightMetrics metrics = box.metrics;
+
+  // BoxData contains inline boxes to be created later. Take them into account.
+  for (const BoxData& box_data : box_data_list_) {
+    LayoutUnit box_ascent =
+        -line_box[box_data.fragment_end].offset.block_offset;
+    metrics.Unite(
+        NGLineHeightMetrics(box_ascent, box_data.size.block_size - box_ascent));
+  }
 
   // In quirks mode, metrics is empty if no content.
-  if (box.IsEmpty())
-    box = NGLineHeightMetrics::Zero();
+  if (metrics.IsEmpty())
+    metrics = NGLineHeightMetrics::Zero();
 
   // If the height of a box that has 'vertical-align: top' or 'bottom' exceeds
   // the height of the "aligned subtree", align the edge to the "aligned
   // subtree" and extend the other edge.
-  NGLineHeightMetrics max = box;
-  for (const NGPendingPositions& child : pending_descendants) {
+  NGLineHeightMetrics max = metrics;
+  for (const NGPendingPositions& child : box.pending_descendants) {
     if ((child.vertical_align == EVerticalAlign::kTop ||
          child.vertical_align == EVerticalAlign::kBottom) &&
         child.metrics.LineHeight() > max.LineHeight()) {
       if (child.vertical_align == EVerticalAlign::kTop) {
-        max = NGLineHeightMetrics(box.ascent,
-                                  child.metrics.LineHeight() - box.ascent);
+        max = NGLineHeightMetrics(metrics.ascent,
+                                  child.metrics.LineHeight() - metrics.ascent);
       } else if (child.vertical_align == EVerticalAlign::kBottom) {
-        max = NGLineHeightMetrics(child.metrics.LineHeight() - box.descent,
-                                  box.descent);
+        max = NGLineHeightMetrics(child.metrics.LineHeight() - metrics.descent,
+                                  metrics.descent);
       }
     }
   }

third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.h

@@ -95,10 +95,6 @@ struct NGInlineBoxState {
   // inline box.
   bool CanAddTextOfStyle(const ComputedStyle&) const;
 
-  // Compute the metrics for when 'vertical-align' is 'top' and 'bottom' from
-  // |pending_descendants|.
-  NGLineHeightMetrics MetricsForTopAndBottomAlign() const;
-
 #if DCHECK_IS_ON()
   void CheckSame(const NGInlineBoxState&) const;
 #endif
@@ -183,6 +179,12 @@ class CORE_EXPORT NGInlineLayoutStateStack {
                                      NGLineBoxFragmentBuilder::ChildList*,
                                      FontBaseline);
 
+  // Compute the metrics for when 'vertical-align' is 'top' and 'bottom' from
+  // |pending_descendants|.
+  NGLineHeightMetrics MetricsForTopAndBottomAlign(
+      const NGInlineBoxState&,
+      const NGLineBoxFragmentBuilder::ChildList&) const;
+
   // Data for a box fragment. See AddBoxFragmentPlaceholder().
   // This is a transient object only while building a line box.
   struct BoxData {

third_party/blink/renderer/core/layout/ng/inline/ng_inline_item.cc

@@ -23,15 +23,33 @@ const char* kNGInlineItemTypeStrings[] = {
 // While the spec defines "non-zero margins, padding, or borders" prevents
 // line boxes to be zero-height, tests indicate that only inline direction
 // of them do so. https://drafts.csswg.org/css2/visuren.html
-bool IsInlineBoxEmpty(const ComputedStyle& style,
-                      const LayoutObject& layout_object) {
-  if (style.BorderStart().NonZero() || !style.PaddingStart().IsZero() ||
-      style.BorderEnd().NonZero() || !style.PaddingEnd().IsZero())
+bool IsInlineBoxStartEmpty(const ComputedStyle& style,
+                           const LayoutObject& layout_object) {
+  if (style.BorderStartWidth() || !style.PaddingStart().IsZero())
     return false;
 
   // Non-zero margin can prevent "empty" only in non-quirks mode.
   // https://quirks.spec.whatwg.org/#the-line-height-calculation-quirk
-  if ((!style.MarginStart().IsZero() || !style.MarginEnd().IsZero()) &&
+  if (!style.MarginStart().IsZero() &&
+      !layout_object.GetDocument().InLineHeightQuirksMode())
+    return false;
+
+  return true;
+}
+
+// Determines if the end of a box is "empty" as defined above.
+//
+// Keeping the "empty" state for start and end separately is important when they
+// belong to different lines, as non-empty item can force the line it belongs to
+// as non-empty.
+bool IsInlineBoxEndEmpty(const ComputedStyle& style,
+                         const LayoutObject& layout_object) {
+  if (style.BorderEndWidth() || !style.PaddingEnd().IsZero())
+    return false;
+
+  // Non-zero margin can prevent "empty" only in non-quirks mode.
+  // https://quirks.spec.whatwg.org/#the-line-height-calculation-quirk
+  if (!style.MarginEnd().IsZero() &&
       !layout_object.GetDocument().InLineHeightQuirksMode())
     return false;
 
@@ -118,7 +136,7 @@ void NGInlineItem::ComputeBoxProperties() {
     DCHECK(style_ && layout_object_ && layout_object_->IsLayoutInline());
     if (style_->HasBoxDecorationBackground() || style_->HasPadding() ||
         style_->HasMargin()) {
-      is_empty_item_ = IsInlineBoxEmpty(*style_, *layout_object_);
+      is_empty_item_ = IsInlineBoxStartEmpty(*style_, *layout_object_);
       should_create_box_fragment_ = true;
     } else {
       is_empty_item_ = true;
@@ -137,6 +155,12 @@ void NGInlineItem::ComputeBoxProperties() {
     return;
   }
 
+  if (type_ == NGInlineItem::kCloseTag) {
+    DCHECK(style_ && layout_object_ && layout_object_->IsLayoutInline());
+    is_empty_item_ = IsInlineBoxEndEmpty(*style_, *layout_object_);
+    return;
+  }
+
   if (type_ == kListMarker) {
     is_empty_item_ = false;
     return;

third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc

@@ -129,6 +129,17 @@ NGInlineBoxState* NGInlineLayoutAlgorithm::HandleOpenTag(
   return box;
 }
 
+NGInlineBoxState* NGInlineLayoutAlgorithm::HandleCloseTag(
+    const NGInlineItem& item,
+    const NGInlineItemResult& item_result,
+    NGInlineBoxState* box) {
+  if (UNLIKELY(quirks_mode_ && !item.IsEmptyItem()))
+    box->EnsureTextMetrics(*item.Style(), baseline_type_);
+  box = box_states_->OnCloseTag(&line_box_, box, baseline_type_,
+                                item.HasEndEdge());
+  return box;
+}
+
 // Prepare NGInlineLayoutStateStack for a new line.
 void NGInlineLayoutAlgorithm::PrepareBoxStates(
     const NGLineInfo& line_info,
@@ -248,7 +259,7 @@ void NGInlineLayoutAlgorithm::CreateLine(NGLineInfo* line_info,
              item.GetLayoutObject()->IsLayoutNGListItem());
       DCHECK(item_result.shape_result);
 
-      if (quirks_mode_)
+      if (UNLIKELY(quirks_mode_))
         box->EnsureTextMetrics(*item.Style(), baseline_type_);
 
       // Take all used fonts into account if 'line-height: normal'.
@@ -273,10 +284,7 @@ void NGInlineLayoutAlgorithm::CreateLine(NGLineInfo* line_info,
     } else if (item.Type() == NGInlineItem::kOpenTag) {
       box = HandleOpenTag(item, item_result, box_states_);
     } else if (item.Type() == NGInlineItem::kCloseTag) {
-      if (quirks_mode_ && box->needs_box_fragment)
-        box->EnsureTextMetrics(*item.Style(), baseline_type_);
-      box = box_states_->OnCloseTag(&line_box_, box, baseline_type_,
-                                    item.HasEndEdge());
+      box = HandleCloseTag(item, item_result, box);
     } else if (item.Type() == NGInlineItem::kAtomicInline) {
       box = PlaceAtomicInline(item, &item_result, *line_info);
     } else if (item.Type() == NGInlineItem::kListMarker) {
@@ -399,7 +407,7 @@ void NGInlineLayoutAlgorithm::PlaceControlItem(const NGInlineItem& item,
   DCHECK(item.GetLayoutObject());
   DCHECK(item.GetLayoutObject()->IsText());
 
-  if (quirks_mode_ && !box->HasMetrics())
+  if (UNLIKELY(quirks_mode_ && !box->HasMetrics()))
     box->EnsureTextMetrics(*item.Style(), baseline_type_);
 
   NGTextFragmentBuilder text_builder(Node(),
@@ -420,7 +428,7 @@ void NGInlineLayoutAlgorithm::PlaceGeneratedContent(
                                                      : fragment->Size().height;
   const ComputedStyle& style = fragment->Style();
   if (box->CanAddTextOfStyle(style)) {
-    if (quirks_mode_)
+    if (UNLIKELY(quirks_mode_))
       box->EnsureTextMetrics(style, baseline_type_);
     DCHECK(!box->text_metrics.IsEmpty());
     line_box_.AddChild(std::move(fragment), box->text_top, inline_size,
@@ -537,7 +545,7 @@ void NGInlineLayoutAlgorithm::PlaceOutOfFlowObjects(
 void NGInlineLayoutAlgorithm::PlaceListMarker(const NGInlineItem& item,
                                               NGInlineItemResult* item_result,
                                               const NGLineInfo& line_info) {
-  if (quirks_mode_) {
+  if (UNLIKELY(quirks_mode_)) {
     box_states_->LineBoxState().EnsureTextMetrics(*item.Style(),
                                                   baseline_type_);
   }

third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.h

@@ -69,6 +69,9 @@ class CORE_EXPORT NGInlineLayoutAlgorithm final
   NGInlineBoxState* HandleOpenTag(const NGInlineItem&,
                                   const NGInlineItemResult&,
                                   NGInlineLayoutStateStack*) const;
+  NGInlineBoxState* HandleCloseTag(const NGInlineItem&,
+                                   const NGInlineItemResult&,
+                                   NGInlineBoxState*);
 
   void BidiReorder();
 

third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.h

@@ -139,6 +139,7 @@ class CORE_EXPORT NGLineBoxFragmentBuilder final
     }
 
     Child& operator[](wtf_size_t i) { return children_[i]; }
+    const Child& operator[](wtf_size_t i) const { return children_[i]; }
 
     wtf_size_t size() const { return children_.size(); }
     bool IsEmpty() const { return children_.IsEmpty(); }

third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc

@@ -943,9 +943,7 @@ void NGLineBreaker::HandleOpenTag(const NGInlineItem& item) {
     // line boxes to be zero-height, tests indicate that only inline direction
     // of them do so. See should_create_line_box_.
     // Force to create a box, because such inline boxes affect line heights.
-    if (!item_result->should_create_line_box &&
-        (item_result->inline_size ||
-         (item_result->margins.inline_start && !in_line_height_quirks_mode_)))
+    if (!item_result->should_create_line_box && !item.IsEmptyItem())
       item_result->should_create_line_box = true;
   }
 
@@ -968,9 +966,7 @@ void NGLineBreaker::HandleCloseTag(const NGInlineItem& item) {
         margins.inline_end + borders.inline_end + paddings.inline_end;
     position_ += item_result->inline_size;
 
-    if (!item_result->should_create_line_box &&
-        (item_result->inline_size ||
-         (margins.inline_end && !in_line_height_quirks_mode_)))
+    if (!item_result->should_create_line_box && !item.IsEmptyItem())
       item_result->should_create_line_box = true;
   }
   DCHECK(item.GetLayoutObject() && item.GetLayoutObject()->Parent());

third_party/blink/renderer/core/style/border_value.h

@@ -60,10 +60,6 @@ class BorderValue {
     SetWidth(width);
   }
 
-  bool NonZero() const {
-    return Width() && (style_ != static_cast<unsigned>(EBorderStyle::kNone));
-  }
-
   bool IsTransparent() const {
     return !color_is_current_color_ && !color_.Alpha();
   }