[Display Locking]: Highlight highest locked ancestor instead of highlighting...

[Display Locking]: Highlight highest locked ancestor instead of highlighting nodes in locked subtree

Previously DevTools might call getBoundingClientRect on nodes in a
locked subtree from within paint, which might trigger style/layout
because we skipped the locked subtree in those phases (so they
might be still dirty), leading to crashes.

For nodes in a locked subtree, when we want to highlight it, we
should instead highlight the highest locked ancestor instead,
because it will always have up-to-date style & layout values.
The tooltip for the overlay will indicate that we're highlighting
the locked ancestor in that case.
See https://bugs.chromium.org/p/chromium/issues/detail?id=934458#c12
for a screenshot of the UI.

Bug: 934458
Change-Id: I20d85104cbeed36af6ac98b157ab6da31ed73dc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1528123Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652682}

third_party/blink/renderer/core/display_lock/display_lock_utilities.cc

@@ -80,15 +80,31 @@ DisplayLockUtilities::ScopedChainForcedUpdate::ScopedChainForcedUpdate(
   }
 }
 
-Element* DisplayLockUtilities::NearestLockedInclusiveAncestor(
+Element* DisplayLockUtilities::NearestLockedInclusiveAncestor(Node& node) {
+  if (!RuntimeEnabledFeatures::DisplayLockingEnabled() ||
+      node.GetDocument().LockedDisplayLockCount() == 0 ||
+      !node.CanParticipateInFlatTree()) {
+    return nullptr;
+  }
+  if (!node.IsElementNode())
+    return NearestLockedExclusiveAncestor(node);
+  if (auto* context = ToElement(node).GetDisplayLockContext()) {
+    if (context->IsLocked())
+      return &ToElement(node);
+  }
+  return NearestLockedExclusiveAncestor(node);
+}
+
+Element* DisplayLockUtilities::NearestLockedExclusiveAncestor(
     const Node& node) {
   if (!RuntimeEnabledFeatures::DisplayLockingEnabled() ||
-      node.GetDocument().LockedDisplayLockCount() == 0) {
+      node.GetDocument().LockedDisplayLockCount() == 0 ||
+      !node.CanParticipateInFlatTree()) {
     return nullptr;
   }
   // TODO(crbug.com/924550): Once we figure out a more efficient way to
   // determine whether we're inside a locked subtree or not, change this.
-  for (Node& ancestor : FlatTreeTraversal::InclusiveAncestorsOf(node)) {
+  for (Node& ancestor : FlatTreeTraversal::AncestorsOf(node)) {
     if (!ancestor.IsElementNode())
       continue;
     if (auto* context = ToElement(ancestor).GetDisplayLockContext()) {
@@ -99,4 +115,35 @@ Element* DisplayLockUtilities::NearestLockedInclusiveAncestor(
   return nullptr;
 }
 
+Element* DisplayLockUtilities::HighestLockedInclusiveAncestor(
+    const Node& node) {
+  if (!RuntimeEnabledFeatures::DisplayLockingEnabled() ||
+      node.GetDocument().LockedDisplayLockCount() == 0 ||
+      !node.CanParticipateInFlatTree()) {
+    return nullptr;
+  }
+  Element* locked_ancestor = nullptr;
+  for (Node& ancestor : FlatTreeTraversal::InclusiveAncestorsOf(node)) {
+    if (!ancestor.IsElementNode())
+      continue;
+    if (auto* context = ToElement(ancestor).GetDisplayLockContext()) {
+      if (context->IsLocked())
+        locked_ancestor = &ToElement(ancestor);
+    }
+  }
+  return locked_ancestor;
+}
+
+Element* DisplayLockUtilities::HighestLockedExclusiveAncestor(
+    const Node& node) {
+  if (!RuntimeEnabledFeatures::DisplayLockingEnabled() ||
+      node.GetDocument().LockedDisplayLockCount() == 0 ||
+      !node.CanParticipateInFlatTree()) {
+    return nullptr;
+  }
+  if (Node* parent = FlatTreeTraversal::Parent(node))
+    return HighestLockedInclusiveAncestor(*parent);
+  return nullptr;
+}
+
 }  // namespace blink

third_party/blink/renderer/core/display_lock/display_lock_utilities.h

@@ -44,7 +44,17 @@ class CORE_EXPORT DisplayLockUtilities {
       Element& element);
 
   // Returns the nearest inclusive ancestor of |node| that is display locked.
-  static Element* NearestLockedInclusiveAncestor(const Node& node);
+  static Element* NearestLockedInclusiveAncestor(Node& node);
+
+  // Returns the nearest non-inclusive ancestor of |node| that is display
+  // locked.
+  static Element* NearestLockedExclusiveAncestor(const Node& node);
+
+  // Returns the highest inclusive ancestor of |node| that is display locked.
+  static Element* HighestLockedInclusiveAncestor(const Node& node);
+
+  // Returns the highest exclusive ancestor of |node| that is display locked.
+  static Element* HighestLockedExclusiveAncestor(const Node& node);
 };
 
 }  // namespace blink

third_party/blink/renderer/core/inspector/inspect_tool_highlight.html

@@ -477,6 +477,9 @@ function _createElementDescription(elementInfo)
     const style = elementInfo.style || {};
     let elementInfoBodyElement;
 
+    if (elementInfo.isLockedAncestor)
+       addTextRow("Showing the locked ancestor", "");
+
     const color = style["color"];
     if (color && color !== "#00000000")
         addColorRow("Color", color);

third_party/blink/renderer/core/inspector/inspect_tools.cc

@@ -11,6 +11,7 @@
 #include "third_party/blink/public/platform/web_pointer_event.h"
 #include "third_party/blink/renderer/core/css/css_color_value.h"
 #include "third_party/blink/renderer/core/css/css_computed_style_declaration.h"
+#include "third_party/blink/renderer/core/display_lock/display_lock_utilities.h"
 #include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/dom/shadow_root.h"
 #include "third_party/blink/renderer/core/dom/static_node_list.h"
@@ -127,7 +128,7 @@ void SearchingForNodeTool::Draw(float scale) {
                              node->GetLayoutObject() &&
                              node->GetDocument().GetFrame();
   InspectorHighlight highlight(node, *highlight_config_, contrast_info_,
-                               append_element_info);
+                               append_element_info, is_locked_ancestor_);
   if (event_target_node_) {
     highlight.AppendEventTargetQuads(event_target_node_.Get(),
                                      *highlight_config_);
@@ -170,6 +171,16 @@ bool SearchingForNodeTool::HandleMouseMove(const WebMouseEvent& event) {
   if (!node)
     return true;
 
+  // If |node| is in a display locked subtree, highlight the highest locked
+  // element instead.
+  if (Node* locked_ancestor =
+          DisplayLockUtilities::HighestLockedExclusiveAncestor(*node)) {
+    node = locked_ancestor;
+    is_locked_ancestor_ = true;
+  } else {
+    is_locked_ancestor_ = false;
+  }
+
   if (auto* frame_owner = DynamicTo<HTMLFrameOwnerElement>(node)) {
     if (!IsA<LocalFrame>(frame_owner->ContentFrame())) {
       // Do not consume event so that remote frame can handle it.
@@ -263,9 +274,15 @@ NodeHighlightTool::NodeHighlightTool(
     Member<Node> node,
     String selector_list,
     std::unique_ptr<InspectorHighlightConfig> highlight_config)
-    : node_(node),
-      selector_list_(selector_list),
+    : selector_list_(selector_list),
       highlight_config_(std::move(highlight_config)) {
+  if (Node* locked_ancestor =
+          DisplayLockUtilities::HighestLockedExclusiveAncestor(*node)) {
+    is_locked_ancestor_ = true;
+    node_ = locked_ancestor;
+  } else {
+    node_ = node;
+  }
   contrast_info_ = FetchContrast(node);
 }
 
@@ -288,7 +305,7 @@ void NodeHighlightTool::DrawNode() {
                              node_->GetLayoutObject() &&
                              node_->GetDocument().GetFrame();
   InspectorHighlight highlight(node_.Get(), *highlight_config_, contrast_info_,
-                               append_element_info);
+                               append_element_info, is_locked_ancestor_);
   std::unique_ptr<protocol::DictionaryValue> highlight_json =
       highlight.AsProtocolValue();
   overlay_->EvaluateInOverlay("drawHighlight", std::move(highlight_json));
@@ -308,8 +325,12 @@ void NodeHighlightTool::DrawMatchingSelector() {
 
   for (unsigned i = 0; i < elements->length(); ++i) {
     Element* element = elements->item(i);
+    // Skip elements in locked subtrees.
+    if (DisplayLockUtilities::NearestLockedExclusiveAncestor(*element))
+      continue;
     InspectorHighlight highlight(element, *highlight_config_, contrast_info_,
-                                 false);
+                                 false /* append_element_info */,
+                                 false /* is_locked_ancestor */);
     overlay_->EvaluateInOverlay("drawHighlight", highlight.AsProtocolValue());
   }
 }

third_party/blink/renderer/core/inspector/inspect_tools.h

@@ -37,6 +37,7 @@ class SearchingForNodeTool : public InspectTool {
 
   Member<InspectorDOMAgent> dom_agent_;
   bool ua_shadow_;
+  bool is_locked_ancestor_ = false;
   Member<Node> hovered_node_;
   Member<Node> event_target_node_;
   std::unique_ptr<InspectorHighlightConfig> highlight_config_;
@@ -79,6 +80,7 @@ class NodeHighlightTool : public InspectTool {
   void DrawMatchingSelector();
   void Trace(blink::Visitor* visitor) override;
 
+  bool is_locked_ancestor_ = false;
   Member<Node> node_;
   String selector_list_;
   std::unique_ptr<InspectorHighlightConfig> highlight_config_;

third_party/blink/renderer/core/inspector/inspector_highlight.cc

@@ -8,6 +8,7 @@
 #include "third_party/blink/renderer/core/css/css_color_value.h"
 #include "third_party/blink/renderer/core/css/css_computed_style_declaration.h"
 #include "third_party/blink/renderer/core/css/css_property_names.h"
+#include "third_party/blink/renderer/core/display_lock/display_lock_utilities.h"
 #include "third_party/blink/renderer/core/dom/pseudo_element.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/core/frame/visual_viewport.h"
@@ -289,6 +290,18 @@ std::unique_ptr<protocol::DictionaryValue> BuildElementInfo(Element* element) {
   if (!layout_object || !containing_view)
     return element_info;
 
+  if (auto* context = element->GetDisplayLockContext()) {
+    if (context->IsLocked()) {
+      // If it's a locked element, use the values from the locked frame rect.
+      const LayoutRect& locked_rect = context->GetLockedFrameRect();
+      element_info->setString("nodeWidth",
+                              String::Number((double)locked_rect.Width()));
+      element_info->setString("nodeHeight",
+                              String::Number((double)locked_rect.Height()));
+    }
+    return element_info;
+  }
+
   // layoutObject the getBoundingClientRect() data in the tooltip
   // to be consistent with the rulers (see http://crbug.com/262338).
   DOMRect* bounding_box = element->getBoundingClientRect();
@@ -398,11 +411,13 @@ InspectorHighlight::InspectorHighlight(
     Node* node,
     const InspectorHighlightConfig& highlight_config,
     const InspectorHighlightContrastInfo& node_contrast,
-    bool append_element_info)
+    bool append_element_info,
+    bool is_locked_ancestor)
     : highlight_paths_(protocol::ListValue::create()),
       show_rulers_(highlight_config.show_rulers),
       show_extension_lines_(highlight_config.show_extension_lines),
       scale_(1.f) {
+  DCHECK(!DisplayLockUtilities::NearestLockedExclusiveAncestor(*node));
   LocalFrameView* frame_view = node->GetDocument().View();
   if (frame_view)
     scale_ = 1.f / frame_view->GetChromeClient()->WindowToViewportScalar(1.f);
@@ -414,6 +429,9 @@ InspectorHighlight::InspectorHighlight(
     element_info_ = BuildTextNodeInfo(ToText(node));
   if (element_info_ && highlight_config.show_styles)
     AppendStyleInfo(node, element_info_.get(), node_contrast);
+
+  if (element_info_ && is_locked_ancestor)
+    element_info_->setString("isLockedAncestor", "true");
 }
 
 InspectorHighlight::~InspectorHighlight() = default;

third_party/blink/renderer/core/inspector/inspector_highlight.h

@@ -53,7 +53,8 @@ class CORE_EXPORT InspectorHighlight {
   InspectorHighlight(Node*,
                      const InspectorHighlightConfig&,
                      const InspectorHighlightContrastInfo&,
-                     bool append_element_info);
+                     bool append_element_info,
+                     bool is_locked_ancestor);
   explicit InspectorHighlight(float scale);
   ~InspectorHighlight();
 

third_party/blink/renderer/core/inspector/inspector_overlay_agent.cc

@@ -45,6 +45,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/script_source_code.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_inspector_overlay_host.h"
+#include "third_party/blink/renderer/core/display_lock/display_lock_utilities.h"
 #include "third_party/blink/renderer/core/dom/dom_node_ids.h"
 #include "third_party/blink/renderer/core/dom/node.h"
 #include "third_party/blink/renderer/core/dom/static_node_list.h"
@@ -601,8 +602,20 @@ Response InspectorOverlayAgent::getHighlightObjectForTest(
   Response response = dom_agent_->AssertNode(node_id, node);
   if (!response.isSuccess())
     return response;
+  bool is_locked_ancestor = false;
+
+  // If |node| is in a display locked subtree, highlight the highest locked
+  // ancestor element instead.
+  if (Node* locked_ancestor =
+          DisplayLockUtilities::HighestLockedExclusiveAncestor(*node)) {
+    node = locked_ancestor;
+    is_locked_ancestor = true;
+  }
+
   InspectorHighlight highlight(node, InspectorHighlight::DefaultConfig(),
-                               InspectorHighlightContrastInfo(), true);
+                               InspectorHighlightContrastInfo(),
+                               true /* append_element_info */,
+                               is_locked_ancestor);
   *result = highlight.AsProtocolValue();
   return Response::OK();
 }

third_party/blink/web_tests/TestExpectations

@@ -265,6 +265,8 @@ crbug.com/882663 display-lock [ Skip ]
 crbug.com/926276 virtual/display-lock/display-lock/lock-after-append/nested-update.html [ Timeout ]
 crbug.com/926276 virtual/display-lock/display-lock/lock-after-append/nested-update-and-commit.html [ Timeout ]
 crbug.com/933544 virtual/display-lock/display-lock/lock-after-append/acquire-on-composited-layer.html [ Crash Pass ]
+crbug.com/882663 http/tests/devtools/elements/highlight/highlight-display-locked.js [ Skip ]
+crbug.com/882663 virtual/nobinary-for-devtools/http/tests/devtools/elements/highlight/highlight-display-locked.js [ Skip ]
 
 # Sheriff 2018/05/25
 crbug.com/846747 http/tests/navigation/navigation-interrupted-by-fragment.html  [ Pass Timeout ]

third_party/blink/web_tests/VirtualTestSuites

@@ -762,6 +762,11 @@
     "base": "display-lock",
     "args": ["--enable-blink-features=DisplayLocking"]
   },
+  {
+    "prefix": "display-lock",
+    "base": "http/tests/devtools/elements/highlight",
+    "args": ["--enable-blink-features=DisplayLocking"]
+  },
   {
     "prefix" : "autoupgrade-optionally-blockable-mixed-content",
     "base": "http/tests/mixed-autoupgrade/optionally",

third_party/blink/web_tests/http/tests/devtools/elements/highlight/highlight-display-locked.js

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+(async function() {
+  TestRunner.addResult(`Tests highlights for display locking.\n`);
+  await TestRunner.loadModule('elements_test_runner');
+  await TestRunner.loadModule('console_test_runner');
+  await TestRunner.showPanel('elements');
+  await TestRunner.loadHTML(`
+      <div id="container"
+        style="contain: style layout;">
+        <div id="child" style="width: 50px; height: 50px;"></div>
+      </div>
+    `);
+  await TestRunner.evaluateInPagePromise(`
+    container.displayLock.acquire({timeout: Infinity, size: [10, 10]});
+    // force layout so that acquire finishes.
+    container.offsetTop;
+  `);
+
+  function dumpChild() {
+    ElementsTestRunner.dumpInspectorHighlightJSON('child', TestRunner.completeTest.bind(TestRunner));
+  }
+
+  function dumpContainerAndChild() {
+    ElementsTestRunner.dumpInspectorHighlightJSON('container', dumpChild);
+  }
+
+  dumpContainerAndChild();
+})();

third_party/blink/web_tests/virtual/display-lock/http/tests/devtools/elements/highlight/README.txt

+# Runs tests with --enable-blink-features=DisplayLocking flag.

third_party/blink/web_tests/virtual/display-lock/http/tests/devtools/elements/highlight/highlight-display-locked-expected.txt

+Tests highlights for display locking.
+
+container{
+  "paths": [
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(255, 0, 0, 0)",
+      "outlineColor": "rgba(128, 0, 0, 0)",
+      "name": "content"
+    },
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(0, 255, 0, 0)",
+      "name": "padding"
+    },
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(0, 0, 255, 0)",
+      "name": "border"
+    },
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(255, 255, 255, 0)",
+      "name": "margin"
+    }
+  ],
+  "showRulers": true,
+  "showExtensionLines": true,
+  "elementInfo": {
+    "tagName": "div",
+    "idValue": "container",
+    "nodeWidth": "10",
+    "nodeHeight": "10"
+  }
+}
+child{
+  "paths": [
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(255, 0, 0, 0)",
+      "outlineColor": "rgba(128, 0, 0, 0)",
+      "name": "content"
+    },
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(0, 255, 0, 0)",
+      "name": "padding"
+    },
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(0, 0, 255, 0)",
+      "name": "border"
+    },
+    {
+      "path": [
+        "M",
+        8,
+        8,
+        "L",
+        18,
+        8,
+        "L",
+        18,
+        18,
+        "L",
+        8,
+        18,
+        "Z"
+      ],
+      "fillColor": "rgba(255, 255, 255, 0)",
+      "name": "margin"
+    }
+  ],
+  "showRulers": true,
+  "showExtensionLines": true,
+  "elementInfo": {
+    "tagName": "div",
+    "idValue": "container",
+    "nodeWidth": "10",
+    "nodeHeight": "10",
+    "isLockedAncestor": "true"
+  }
+}
+