[FragmentItem] Recalculate ink overflow for self-painting layers

This patch fixes the logic to recalculate ink overflow,
especially for self-painting layers.

|PaintLayer| calls |RecalcVisualOverflow| for its owner
|LayoutObject|. |LayoutBlockFlow| already redirects this to
|NGFragmentItems|, but inline boxes and atomic inlines did
not handle this. They have ink overflow stored in
corresponding |NGFragmentItem|s, and they need to be
recalculated. This patch handles this situation by adding
|LayoutBoxModelObject::RecalcVisualOverflow|.

Currently, there are multiple workarounds added to pass all
tests, but they are causing sometimes redundant, or sometimes
missing recalculations. This fix allows to remove one of such
workarounds in |NGFragmentItem::RecalcInkOverflowForCursor|,
that calls |RecalcInkOverflow| even if the child is
|HasSelfPaintingLayer|. Removing this workaround reduces
redundant recalculations.

Three sets of tests cover this change:
1. This patch adds a unit test that demonstrates when the
   current code fails to recalculate ink overflow, originally
   found while investigating <crbug.com/1128199>.
2. When |NGFragmentItem| is enabled, reading dirty self ink
   overflow hits DCHECK. Not hitting DCHECK can ensure that
   this change calculate ink overflow for all cases where we
   read self ink overflow.
3. This patch adds a DCHECK when reading dirty self-and-
   container ink overflow. 215 unit tests and 653 web tests
   fail this DCHECK <crrev.com/c/2432707> but pass with this
   patch (except when NG block fragmentation is enabled, see
   notes below.)

Note:
* Both legacy and |NGPaintFragment| have the same issue. This
  patch addresses only for |NGFragmentItem|.
* |LayoutBlockFlow| does not handle |RecalcVisualOverflow|
  correctly for multicol. This was fixed in r810561
  <crrev.com/c/2428516>. Many `virtual/layout_ng_block_frag`
  tests fail without that fix when this patch is applied.
* 3 `virtual/layout_ng_block_frag` tests still fail the new
  test #3 above, even after the fix above. crbug.com/1132619
  tracks this issue.

Bug: 1128199, 1130856
Change-Id: I42a75e1dad7c2b35146cca4ddaaf9763df4e7440
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2428530
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811125}

third_party/blink/renderer/core/layout/layout_box_model_object.cc

@@ -37,6 +37,7 @@
 #include "third_party/blink/renderer/core/layout/layout_geometry_map.h"
 #include "third_party/blink/renderer/core/layout/layout_inline.h"
 #include "third_party/blink/renderer/core/layout/layout_view.h"
+#include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_cursor.h"
 #include "third_party/blink/renderer/core/layout/ng/legacy_layout_tree_walking.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_constraint_space.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_result.h"
@@ -647,6 +648,22 @@ void LayoutBoxModelObject::AddOutlineRectsForDescendant(
   descendant.AddOutlineRects(rects, additional_offset, include_block_overflows);
 }
 
+void LayoutBoxModelObject::RecalcVisualOverflow() {
+  // |PaintLayer| calls this function when |HasSelfPaintingLayer|. When |this|
+  // is an inline box or an atomic inline, its ink overflow is stored in
+  // |NGFragmentItem| in the inline formatting context.
+  if (IsInline() && IsInLayoutNGInlineFormattingContext() &&
+      RuntimeEnabledFeatures::LayoutNGFragmentItemEnabled()) {
+    DCHECK(HasSelfPaintingLayer());
+    NGInlineCursor cursor;
+    for (cursor.MoveTo(*this); cursor; cursor.MoveToNextForSameLayoutObject())
+      cursor.Current().RecalcInkOverflow(cursor);
+    return;
+  }
+
+  LayoutObject::RecalcVisualOverflow();
+}
+
 void LayoutBoxModelObject::AbsoluteQuadsForSelf(
     Vector<FloatQuad>& quads,
     MapCoordinatesFlags mode) const {

third_party/blink/renderer/core/layout/layout_box_model_object.h

@@ -574,6 +574,8 @@ class CORE_EXPORT LayoutBoxModelObject : public LayoutObject {
   // public only for NGOutOfFlowLayoutPart, otherwise protected.
   LayoutBoxModelObject* Continuation() const;
 
+  void RecalcVisualOverflow() override;
+
  protected:
   // Compute absolute quads for |this|, but not any continuations. May only be
   // called for objects which can be or have continuations, i.e. LayoutInline or

third_party/blink/renderer/core/layout/layout_text.cc

@@ -2762,4 +2762,18 @@ const base::span<NGInlineItem>& LayoutText::InlineItems() const {
   return *GetNGInlineItems();
 }
 
+#if DCHECK_IS_ON()
+void LayoutText::RecalcVisualOverflow() {
+  // We should never reach here, because |PaintLayer| calls
+  // |RecalcVisualOverflow| for each layer, and the containing |LayoutObject|
+  // should recalculate its |NGFragmentItem|s without traversing descendant
+  // |LayoutObject|s.
+  if (IsInline() && IsInLayoutNGInlineFormattingContext() &&
+      RuntimeEnabledFeatures::LayoutNGFragmentItemEnabled())
+    NOTREACHED();
+
+  LayoutObject::RecalcVisualOverflow();
+}
+#endif
+
 }  // namespace blink

third_party/blink/renderer/core/layout/layout_text.h

@@ -428,6 +428,10 @@ class CORE_EXPORT LayoutText : public LayoutObject {
     return {LayoutUnit::Max(), LayoutUnit::Max()};
   }
 
+#if DCHECK_IS_ON()
+  void RecalcVisualOverflow() override;
+#endif
+
  protected:
   void WillBeDestroyed() override;
 

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

@@ -520,11 +520,11 @@ PhysicalRect NGFragmentItem::RecalcInkOverflowForCursor(
       NOTREACHED();
       continue;
     }
+    if (UNLIKELY(item->HasSelfPaintingLayer()))
+      continue;
 
     PhysicalRect child_rect;
     item->GetMutableForPainting().RecalcInkOverflow(*cursor, &child_rect);
-    if (item->HasSelfPaintingLayer())
-      continue;
     if (!child_rect.IsEmpty()) {
       child_rect.offset += item->OffsetInContainerBlock();
       contents_ink_overflow.Unite(child_rect);

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

@@ -384,6 +384,7 @@ class CORE_EXPORT NGFragmentItem {
 
  private:
   FRIEND_TEST_ALL_PREFIXES(NGFragmentItemTest, CopyMove);
+  FRIEND_TEST_ALL_PREFIXES(NGFragmentItemTest, SelfPaintingInlineBox);
   FRIEND_TEST_ALL_PREFIXES(StyleChangeTest, NeedsCollectInlinesOnStyle);
 
   // Create a text item.

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

@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_cursor.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_test.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.h"
+#include "third_party/blink/renderer/core/paint/paint_layer.h"
 
 using testing::ElementsAre;
 
@@ -345,6 +346,35 @@ TEST_F(NGFragmentItemTest, CulledInlineBox) {
   EXPECT_EQ(IntRect(0, 20, 80, 10), span2->AbsoluteBoundingBoxRect());
 }
 
+TEST_F(NGFragmentItemTest, SelfPaintingInlineBox) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+    #self_painting_inline_box {
+      opacity: .2;
+    }
+    </style>
+    <div>
+      <span id="self_painting_inline_box">self painting inline box</span>
+    </div>
+  )HTML");
+
+  // Invalidate the ink overflow of a child in `#self_painting_inline_box`.
+  auto* self_painting_inline_box =
+      ToLayoutInline(GetLayoutObjectByElementId("self_painting_inline_box"));
+  ASSERT_TRUE(self_painting_inline_box->HasSelfPaintingLayer());
+  auto* text = ToLayoutText(self_painting_inline_box->FirstChild());
+  text->InvalidateVisualOverflow();
+
+  // Mark the |PaintLayer| to need to recalc visual overflow.
+  self_painting_inline_box->Layer()->SetNeedsVisualOverflowRecalc();
+  RunDocumentLifecycle();
+
+  // Test if it recalculated the ink overflow.
+  NGInlineCursor cursor;
+  for (cursor.MoveTo(*text); cursor; cursor.MoveToNextForSameLayoutObject())
+    EXPECT_TRUE(cursor.Current()->IsInkOverflowComputed());
+}
+
 // Various nodes/elements to test insertions.
 using CreateNode = Node* (*)(Document&);
 static CreateNode node_creators[] = {

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

@@ -573,6 +573,15 @@ const PhysicalRect NGInlineCursorPosition::SelfInkOverflow() const {
   return PhysicalRect();
 }
 
+void NGInlineCursorPosition::RecalcInkOverflow(
+    const NGInlineCursor& cursor) const {
+  DCHECK(item_);
+  DCHECK_EQ(item_, cursor.Current().Item());
+  PhysicalRect self_and_contents_rect;
+  item_->GetMutableForPainting().RecalcInkOverflow(cursor,
+                                                   &self_and_contents_rect);
+}
+
 TextDirection NGInlineCursorPosition::ResolvedDirection() const {
   if (paint_fragment_)
     return paint_fragment_->PhysicalFragment().ResolvedDirection();

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

@@ -132,6 +132,8 @@ class CORE_EXPORT NGInlineCursorPosition {
   const PhysicalRect InkOverflow() const;
   const PhysicalRect SelfInkOverflow() const;
 
+  void RecalcInkOverflow(const NGInlineCursor& cursor) const;
+
   // Returns start/end of offset in text content of current text fragment.
   // It is error when this cursor doesn't point to text fragment.
   NGTextOffset TextOffset() const;

third_party/blink/renderer/core/layout/ng/ng_block_node.cc

@@ -67,6 +67,7 @@
 #include "third_party/blink/renderer/core/mathml/mathml_scripts_element.h"
 #include "third_party/blink/renderer/core/mathml/mathml_space_element.h"
 #include "third_party/blink/renderer/core/mathml/mathml_under_over_element.h"
+#include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/text/writing_mode.h"
@@ -1319,6 +1320,8 @@ void NGBlockNode::CopyFragmentItemsToLayoutBox(
         }
         layout_box->SetLocationAndUpdateOverflowControlsIfNeeded(
             maybe_flipped_offset.ToLayoutPoint());
+        if (UNLIKELY(layout_box->HasSelfPaintingLayer()))
+          layout_box->Layer()->SetNeedsVisualOverflowRecalc();
         continue;
       }
 
@@ -1330,6 +1333,8 @@ void NGBlockNode::CopyFragmentItemsToLayoutBox(
             layout_inline->Continuation()) {
           box_->SetContainsInlineWithOutlineAndContinuation(true);
         }
+        if (UNLIKELY(layout_inline->HasSelfPaintingLayer()))
+          layout_inline->Layer()->SetNeedsVisualOverflowRecalc();
       }
     }
   }

third_party/blink/renderer/core/layout/ng/ng_ink_overflow.cc

@@ -33,6 +33,8 @@ inline bool HasOverflow(const PhysicalRect& rect, const PhysicalSize& size) {
 }  // namespace
 
 #if DCHECK_IS_ON()
+unsigned NGInkOverflow::read_unset_as_none_ = 0;
+
 NGInkOverflow::~NGInkOverflow() {
   // Because |Type| is kept outside of the instance, callers must call |Reset|
   // before destructing.
@@ -129,7 +131,7 @@ PhysicalRect NGInkOverflow::FromOutsets(const PhysicalSize& size) const {
 PhysicalRect NGInkOverflow::Self(Type type, const PhysicalSize& size) const {
   CheckType(type);
 #if DCHECK_IS_ON()
-  // TODO(crbug.com/829028): Should compute all ink overflow when
+  // TODO(crbug.com/1132619): Should compute all ink overflow when
   // NGBlockFragmentation is enabled.
   if (!RuntimeEnabledFeatures::LayoutNGBlockFragmentationEnabled())
     DCHECK_NE(type, kNotSet);
@@ -156,9 +158,14 @@ PhysicalRect NGInkOverflow::SelfAndContents(Type type,
   CheckType(type);
   switch (type) {
     case kNotSet:
-      // It is fine to read |kNotSet|, because
-      // |PaintLayer::UpdateDescendantDependentFlags| needs to know the old
-      // value before it computes ink overflow.
+#if DCHECK_IS_ON()
+      if (!read_unset_as_none_ &&
+          // TODO(crbug.com/1132619): Should compute all ink overflow when
+          // NGBlockFragmentation is enabled.
+          !RuntimeEnabledFeatures::LayoutNGBlockFragmentationEnabled())
+        NOTREACHED();
+      FALLTHROUGH;
+#endif
     case kNone:
       return {PhysicalOffset(), size};
     case kSmallSelf:

third_party/blink/renderer/core/layout/ng/ng_ink_overflow.h

@@ -116,6 +116,16 @@ class CORE_EXPORT NGInkOverflow {
       const ComputedStyle& style,
       const PhysicalSize& size);
 
+#if DCHECK_IS_ON()
+  struct ReadUnsetAsNoneScope {
+    STACK_ALLOCATED();
+
+   public:
+    ReadUnsetAsNoneScope() { ++read_unset_as_none_; }
+    ~ReadUnsetAsNoneScope() { --read_unset_as_none_; }
+  };
+#endif
+
  private:
   PhysicalRect FromOutsets(const PhysicalSize& size) const;
 
@@ -160,6 +170,8 @@ class CORE_EXPORT NGInkOverflow {
 
 #if DCHECK_IS_ON()
   Type type_ = Type::kNotSet;
+
+  static unsigned read_unset_as_none_;
 #endif
 };
 

third_party/blink/renderer/core/paint/paint_layer.cc

@@ -129,6 +129,14 @@ struct SameSizeAsPaintLayer : DisplayItemClient {
 ASSERT_SIZE(PaintLayer, SameSizeAsPaintLayer);
 #endif
 
+inline PhysicalRect PhysicalVisualOverflowRectAllowingUnset(
+    const LayoutBoxModelObject& layout_object) {
+#if DCHECK_IS_ON()
+  NGInkOverflow::ReadUnsetAsNoneScope read_unset_as_none;
+#endif
+  return layout_object.PhysicalVisualOverflowRect();
+}
+
 }  // namespace
 
 PaintLayerRareData::PaintLayerRareData()
@@ -683,7 +691,8 @@ void PaintLayer::UpdateDescendantDependentFlags() {
     needs_descendant_dependent_flags_update_ = false;
 
     if (IsSelfPaintingLayer() && needs_visual_overflow_recalc_) {
-      auto old_visual_rect = GetLayoutObject().PhysicalVisualOverflowRect();
+      PhysicalRect old_visual_rect =
+          PhysicalVisualOverflowRectAllowingUnset(GetLayoutObject());
       GetLayoutObject().RecalcVisualOverflow();
       if (old_visual_rect != GetLayoutObject().PhysicalVisualOverflowRect()) {
         SetNeedsCompositingInputsUpdateInternal();