Skip empty-clip fragments when creating flow thread contexts.

This patch ensures that when the fragmentainer iterator returns a fragment that has an empty clip, we skip generating a fragment for it. This is important since a fragment that has an empty clip can share the logical top with the subsequent fragment, meaning that we cannot use the logical top to uniquely identify a fragment. This patch addresses this issue. Furthermore, we typically skip painting fragments that have empty clips anyway (for obvious reasons), so this should not impact painted contents. R=chrishtr@chromium.org, wangxianzhu@chromium.org Bug: 870521 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I9f2f6311e0c3c75e4a8d114f13476ce8774e8218 Reviewed-on: https://chromium-review.googlesource.com/1187234 Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/master@{#585671}

Skip empty-clip fragments when creating flow thread contexts.
This patch ensures that when the fragmentainer iterator returns a fragment that has an empty clip, we skip generating a fragment for it. This is important since a fragment that has an empty clip can share the logical top with the subsequent fragment, meaning that we cannot use the logical top to uniquely identify a fragment. This patch addresses this issue. Furthermore, we typically skip painting fragments that have empty clips anyway (for obvious reasons), so this should not impact painted contents. R=chrishtr@chromium.org, wangxianzhu@chromium.org Bug: 870521 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I9f2f6311e0c3c75e4a8d114f13476ce8774e8218 Reviewed-on: https://chromium-review.googlesource.com/1187234 Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/heads/master@{#585671}
2c7bd7f4 · Vladimir Levin · Commit Bot · c3b78856 · 2c7bd7f4 · 2c7bd7f4
Commit 2c7bd7f4 authored Aug 24, 2018 by Vladimir Levin Committed by Commit Bot Aug 24, 2018
2 changed files
--- a/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
+++ b/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
@@ -2389,6 +2389,14 @@ void PaintPropertyTreeBuilder::CreateFragmentContextsInFlowThread(
    if (object_.HasLayer()) {
      // 1. Compute clip in flow thread space.
      fragment_clip = iterator.ClipRectInFlowThread();
+
+      // We skip empty clip fragments, since they can share the same logical top
+      // with the subsequent fragments. Since we skip drawing empty fragments
+      // anyway, it doesn't affect the paint output, but it allows us to use
+      // logical top to uniquely identify fragments in an object.
+      if (fragment_clip->IsEmpty())
+        continue;
+
      // 2. Convert #1 to visual coordinates in the space of the flow thread.
      fragment_clip->MoveBy(pagination_offset);
      // 3. Adjust #2 to visual coordinates in the containing "paint offset"

--- a/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
+++ b/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
@@ -7,6 +7,7 @@
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
 #include "third_party/blink/renderer/core/frame/visual_viewport.h"
 #include "third_party/blink/renderer/core/html/html_iframe_element.h"
+#include "third_party/blink/renderer/core/layout/layout_flow_thread.h"
 #include "third_party/blink/renderer/core/layout/layout_image.h"
 #include "third_party/blink/renderer/core/layout/layout_table_cell.h"
 #include "third_party/blink/renderer/core/layout/layout_table_section.h"
@@ -5958,4 +5959,39 @@ TEST_P(PaintPropertyTreeBuilderTest,
            fragment.PaintProperties()->OverflowClip());
 }

+TEST_P(PaintPropertyTreeBuilderTest, SkipEmptyClipFragments) {
+  SetBodyInnerHTML(R"HTML(
+    <!doctype HTML>
+    <style>h4 { column-span: all; }</style>
+    <div id="container" style="columns:1;">
+      lorem
+      <h4>hi</h4>
+      <div><h4>hello</h4></div>
+      ipsum
+    </div>
+  )HTML");
+
+  const auto* flow_thread = GetDocument()
+                                .getElementById("container")
+                                ->GetLayoutObject()
+                                ->SlowFirstChild();
+  EXPECT_TRUE(flow_thread->IsLayoutFlowThread());
+  EXPECT_TRUE(ToLayoutFlowThread(flow_thread)->IsLayoutMultiColumnFlowThread());
+
+  // FragmentainerIterator would return 3 things:
+  // 1. A fragment that contains "lorem" and is interrupted by the first h4,
+  //    since it's column-span: all.
+  // 2. A fragment that starts at the inner div of height 0 and is immediately
+  //    interrupted by a nested h4.
+  // 3. A fragment that contains "ipsum".
+  //
+  // The second fragment would have an empty clip and the same logical top as
+  // the third fragment. This test ensures that this fragment is not present in
+  // the LayoutMultiColumnFlowThread's fragments.
+  EXPECT_EQ(2u, NumFragments(flow_thread));
+  EXPECT_NE(
+      flow_thread->FirstFragment().LogicalTopInFlowThread(),
+      flow_thread->FirstFragment().NextFragment()->LogicalTopInFlowThread());
+}
+
 }  // namespace blink