Don't promote implicit root scroller with multiple matches

chrishtr@ pointed out in https://crrev.com/c/1038648 that the paint order selection used when multiple elements on a page meet all the criteria of an implicit root scroller isn't correct. On reflection, it's not immediately clear to me that this is at all useful so, rather than adding complexity, this CL removes the ordering and just avoids making any decision at all in this case. We can revisit if the need comes up in real usage. Bug: 844534 Change-Id: I4e05da981eace4062f3898a3a7fe1d4cffdec658 Reviewed-on: https://chromium-review.googlesource.com/c/1288905Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#601161}

Don't promote implicit root scroller with multiple matches
chrishtr@ pointed out in https://crrev.com/c/1038648 that the paint order selection used when multiple elements on a page meet all the criteria of an implicit root scroller isn't correct. On reflection, it's not immediately clear to me that this is at all useful so, rather than adding complexity, this CL removes the ordering and just avoids making any decision at all in this case. We can revisit if the need comes up in real usage. Bug: 844534 Change-Id: I4e05da981eace4062f3898a3a7fe1d4cffdec658 Reviewed-on: https://chromium-review.googlesource.com/c/1288905Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: David Bokan <bokan@chromium.org> Cr-Commit-Position: refs/heads/master@{#601161}
bd27a728 · David Bokan · Commit Bot · 0d2b1013 · bd27a728 · bd27a728
Commit bd27a728 authored Oct 19, 2018 by David Bokan Committed by Commit Bot Oct 19, 2018
2 changed files
--- a/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
+++ b/third_party/blink/renderer/core/page/scrolling/root_scroller_controller.cc
@@ -393,8 +393,7 @@ void RootScrollerController::ProcessImplicitCandidates() {
  if (ScrollsVerticalOverflow(*document_->GetLayoutView()))
    return;

-  Element* highest_z_element = nullptr;
-  bool highest_is_ambiguous = false;
+  bool multiple_matches = false;

  HeapHashSet<WeakMember<Element>> copy(implicit_candidates_);
  for (auto& element : copy) {
@@ -404,25 +403,15 @@ void RootScrollerController::ProcessImplicitCandidates() {
      continue;
    }

-    if (!highest_z_element) {
-      highest_z_element = element;
-    } else {
-      int element_z = element->GetLayoutObject()->Style()->ZIndex();
-      int highest_z = highest_z_element->GetLayoutObject()->Style()->ZIndex();
-
-      if (element_z > highest_z) {
-        highest_z_element = element;
-        highest_is_ambiguous = false;
-      } else if (element_z == highest_z) {
-        highest_is_ambiguous = true;
-      }
-    }
+    if (implicit_root_scroller_)
+      multiple_matches = true;
+
+    implicit_root_scroller_ = element;
  }

-  if (highest_is_ambiguous)
+  // Only promote an implicit root scroller if we have a unique match.
+  if (multiple_matches)
    implicit_root_scroller_ = nullptr;
-  else
-    implicit_root_scroller_ = highest_z_element;
 }

 PaintLayer* RootScrollerController::RootScrollerPaintLayer() const {

--- a/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
+++ b/third_party/blink/renderer/core/page/scrolling/root_scroller_test.cc
@@ -1937,6 +1937,60 @@ TEST_F(ImplicitRootScrollerSimTest, ImplicitRootScrollerIframe) {
            GetDocument().GetRootScrollerController().EffectiveRootScroller());
 }

+// Tests that if we have multiple valid candidates for implicit promotion, we
+// don't promote either.
+TEST_F(ImplicitRootScrollerSimTest, DontPromoteWhenMultipleAreValid) {
+  WebView().Resize(WebSize(800, 600));
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  request.Complete(R"HTML(
+          <!DOCTYPE html>
+          <style>
+            ::-webkit-scrollbar {
+              width: 0px;
+              height: 0px;
+            }
+            body, html {
+              width: 100%;
+              height: 100%;
+              margin: 0px;
+            }
+            iframe {
+              position: absolute;
+              left: 0;
+              top: 0;
+              width: 100%;
+              height: 100%;
+              border: 0;
+            }
+          </style>
+          <iframe id="container"
+                  srcdoc="<!DOCTYPE html><style>html {height: 300%;}</style>">
+          </iframe>
+          <iframe id="container2"
+                  srcdoc="<!DOCTYPE html><style>html {height: 300%;}</style>">
+          </iframe>
+      )HTML");
+  // srcdoc iframe loads via posted tasks.
+  RunPendingTasks();
+  Compositor().BeginFrame();
+
+  // Since both iframes are valid candidates, neither should be promoted.
+  ASSERT_EQ(&GetDocument(),
+            GetDocument().GetRootScrollerController().EffectiveRootScroller());
+
+  // Now make the second one invalid, that should cause the first to be
+  // promoted.
+  Element* container2 = GetDocument().getElementById("container2");
+  container2->style()->setProperty(&GetDocument(), "height", "95%", String(),
+                                   ASSERT_NO_EXCEPTION);
+  Compositor().BeginFrame();
+
+  Element* container = GetDocument().getElementById("container");
+  ASSERT_EQ(container,
+            GetDocument().GetRootScrollerController().EffectiveRootScroller());
+}
+
 // Test that when a valid iframe becomes loaded and thus should be promoted, it
 // becomes the root scroller, without needing an intervening layout.
 TEST_F(ImplicitRootScrollerSimTest, IframeLoadedWithoutLayout) {