Update offsetParent behavior in shadow trees

New behavior for offsetParent in shadow trees was discussed in [1] and [2]. This lead to a chromium patch [3] which changed the behavior. After [3] landed, the desired behavior in the discussions in [1] and [2] seemed to have changed. Unfortunately, the author of [3] was no longer working on chromium. This new behavior was then added to the spec and landed in webkit [4] and firefox [5], and a WPT was added for it [6]. This patch implements the new behavior to follow suit with webkit and firefox based on the WPT in [6]. Unfortunately, there are several tests which are either internal to chromium or are only passing in chromium which appear to oppose this new behavior and will have to be updated or removed: - external/wpt/css/css-contain/content-visibility/content-visibility-035.html - external/wpt/css/css-contain/content-visibility/content-visibility-044.html - fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html - shadow-dom/offsetParent.html For shadow-dom/offsetParent.html, I verified that firefox and safari both currently fail the same tests which this patch does. [1] WICG/webcomponents#497 [2] w3c/csswg-drafts#159 [3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f [4] https://trac.webkit.org/changeset/239313/webkit [5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074 [6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html Fixed: 920069 Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2775208 Reviewed-by: Mason Freed <[email protected]> Reviewed-by: vmpstr <[email protected]> Reviewed-by: Ian Kilpatrick <[email protected]> Commit-Queue: Joey Arhar <[email protected]> Cr-Commit-Position: refs/heads/master@{#872658} GitOrigin-RevId: a48d953625d82035201c12d8e92ad0f39bf85258
mjfroman · Apr 15, 2021 · 047e880 · 047e880
1 parent 9b63e41
commit 047e880
Show file tree

Hide file tree

Showing 16 changed files with 104 additions and 2 deletions.
diff --git a/blink/common/features.cc b/blink/common/features.cc
@@ -577,6 +577,12 @@ const base::Feature kWebviewAccelerateSmallCanvases{
 const base::Feature kDiscardCodeCacheAfterFirstUse{
     "DiscardCodeCacheAfterFirstUse", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// Kill switch for the new element.offsetParent behavior.
+// TODO(crbug.com/920069): Remove this once the feature has
+// landed and no compat issues are reported.
+const base::Feature kOffsetParentNewSpecBehavior{
+    "OffsetParentNewSpecBehavior", base::FEATURE_ENABLED_BY_DEFAULT};
+
 // Slightly delays rendering if there are fonts being preloaded, so that
 // they don't miss the first paint if they can be loaded fast enough (e.g.,
 // from the disk cache)

diff --git a/blink/public/common/features.h b/blink/public/common/features.h
@@ -176,6 +176,10 @@ BLINK_COMMON_EXPORT extern const base::Feature kWebviewAccelerateSmallCanvases;
 
 BLINK_COMMON_EXPORT extern const base::Feature kDiscardCodeCacheAfterFirstUse;
 
+// TODO(crbug.com/920069): Remove OffsetParentNewSpecBehavior after the feature
+// is in stable with no issues.
+BLINK_COMMON_EXPORT extern const base::Feature kOffsetParentNewSpecBehavior;
+
 BLINK_COMMON_EXPORT extern const base::Feature kFontPreloadingDelaysRendering;
 BLINK_COMMON_EXPORT extern const base::FeatureParam<int>
     kFontPreloadingDelaysRenderingParam;

diff --git a/blink/renderer/core/dom/node.cc b/blink/renderer/core/dom/node.cc
@@ -1483,6 +1483,17 @@ bool Node::IsDescendantOf(const Node* other) const {
   return false;
 }
 
+bool Node::IsDescendantOrShadowDescendantOf(const Node* other) const {
+  if (IsDescendantOf(other))
+    return true;
+
+  for (auto* host = OwnerShadowHost(); host; host = host->OwnerShadowHost()) {
+    if (other->contains(host))
+      return true;
+  }
+  return false;
+}
+
 bool Node::contains(const Node* node) const {
   if (!node)
     return false;

diff --git a/blink/renderer/core/dom/node.h b/blink/renderer/core/dom/node.h
@@ -642,6 +642,7 @@ class CORE_EXPORT Node : public EventTarget {
   unsigned CountChildren() const;
 
   bool IsDescendantOf(const Node*) const;
+  bool IsDescendantOrShadowDescendantOf(const Node*) const;
   bool contains(const Node*) const;
   // https://dom.spec.whatwg.org/#concept-shadow-including-inclusive-ancestor
   bool IsShadowIncludingInclusiveAncestorOf(const Node&) const;

diff --git a/blink/renderer/core/layout/layout_object.cc b/blink/renderer/core/layout/layout_object.cc
@@ -4160,6 +4160,17 @@ Element* LayoutObject::OffsetParent(const Element* base) const {
     if (!node)
       continue;
 
+    // In the case where |base| is getting slotted into a shadow root, we
+    // shouldn't return anything inside the shadow root. The returned node must
+    // be in the same shadow root or document as |base|.
+    // https://github.com/w3c/csswg-drafts/issues/159
+    // TODO(crbug.com/920069): Remove the feature check here when the feature
+    // has gotten to stable without any issues.
+    if (RuntimeEnabledFeatures::OffsetParentNewSpecBehaviorEnabled() && base &&
+        !base->IsDescendantOrShadowDescendantOf(&node->TreeRoot())) {
+      continue;
+    }
+
     // TODO(kochi): If |base| or |node| is nested deep in shadow roots, this
     // loop may get expensive, as isUnclosedNodeOf() can take up to O(N+M) time
     // (N and M are depths).

diff --git a/blink/renderer/platform/runtime_enabled_features.json5 b/blink/renderer/platform/runtime_enabled_features.json5
@@ -1415,6 +1415,12 @@
       name: "OffscreenCanvasCommit",
       status: "experimental",
     },
+    {
+      // TODO(crbug.com/920069): Remove OffsetParentNewSpecBehavior after the
+      // feature is in stable with no issues.
+      name: "OffsetParentNewSpecBehavior",
+      status: "stable"
+    },
     {
       name: "OnDeviceChange",
       // Android does not yet support SystemMonitor.

diff --git a/blink/web_tests/VirtualTestSuites b/blink/web_tests/VirtualTestSuites
@@ -788,6 +788,15 @@
     "bases": [ "js/weakrefs" ],
     "args": [ "--js-flags=--harmony-weak-refs" ]
   },
+  {
+    "prefix": "offsetparent-old-behavior",
+    "bases": [
+      "external/wpt/css/css-contain/content-visibility",
+      "external/wpt/shadow-dom",
+      "fast/dom/shadow"
+    ],
+    "args": ["--disable-features=OffsetParentNewSpecBehavior"]
+  },
   {
     "prefix": "document-domain-disabled-by-default",
     "bases": [

diff --git a/blink/web_tests/external/wpt/css/css-contain/content-visibility/content-visibility-035.html b/blink/web_tests/external/wpt/css/css-contain/content-visibility/content-visibility-035.html
@@ -7,6 +7,8 @@
 <meta name="assert" content="content-visibility hidden element's subtree cannot be focused">
 <meta name="assert" content="content-visibility hidden element's subtree can access layout values">
 
+<body style="margin: 0">
+
 <div id="host">
   <input id="slotted" type="text">
 </div>

diff --git a/blink/web_tests/external/wpt/css/css-contain/content-visibility/content-visibility-044.html b/blink/web_tests/external/wpt/css/css-contain/content-visibility/content-visibility-044.html
@@ -9,6 +9,8 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 
+<body style="margin: 0">
+
 <div id=host>
 <input id=slotted>
 <script>

diff --git a/blink/web_tests/fast/dom/shadow/offset-parent-does-not-leak-ua-shadow-expected.txt b/blink/web_tests/fast/dom/shadow/offset-parent-does-not-leak-ua-shadow-expected.txt
@@ -2,7 +2,7 @@ offsetParent should not leak nodes in user agent Shadow DOM.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
-PASS child.offsetParent is shadow.positionedElement
+PASS child.offsetParent is container
 PASS child.offsetParent is container
 PASS child.offsetParent is container
 PASS child.offsetParent is container

diff --git a/blink/web_tests/fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html b/blink/web_tests/fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
@@ -32,7 +32,7 @@
 host.appendChild(child);
 var shadow = host.attachShadow({mode: 'open'});
 configureShadowRoot(shadow);
-shouldBe('child.offsetParent', 'shadow.positionedElement');
+shouldBe('child.offsetParent', 'container');
 
 // Test that offsetParent works 'through' Shadow DOM when there is no
 // positioned ancestor in user agent Shadow DOM

diff --git a/blink/web_tests/shadow-dom/offsetParent-expected.txt b/blink/web_tests/shadow-dom/offsetParent-expected.txt
@@ -0,0 +1,24 @@
+This is a testharness.js-based test.
+PASS offsetParent should return node in the same node tree in open shadow root.
+PASS offsetParent should return node in the same node tree in closed shadow root.
+PASS offsetParent should return an unclosed node in a open shadow from closed shadow.
+PASS offsetParent should return an unclosed node in a closed shadow from open shadow.
+FAIL offsetParent should return an unclosed node. assert_equals: expected "container_open" but got "host_open2"
+PASS offsetParent should skip any non-unclosed nodes.
+FAIL offsetParent should return node in the open shadow root because it is unclosed. assert_equals: expected Element node <div id="container" style="position: relative;">
+      <s... but got Element node <div id="host_open4" class="container">
+
+  <div id="hos...
+FAIL offsetParent should return null in case position:fixed offsetParent is not unclosed. assert_equals: expected null but got Element node <body><div id="host_open0">
+
+</div>
+
+<div id="host_clos...
+FAIL All position:static elements should be skipped for offsetParent. assert_equals: expected Element node <div id="relative" style="position: relative;">
+        <... but got Element node <body><div id="host_open0">
+
+</div>
+
+<div id="host_clos...
+Harness: the test ran to completion.
+
diff --git a/blink/web_tests/shadow-dom/offsetParent.html b/blink/web_tests/shadow-dom/offsetParent.html
@@ -111,6 +111,8 @@
 </div>
 
 <script>
+// TODO(crbug.com/920069): Remove this test when the
+// OffsetParentNewSpecBehavior feature has landed in stable with no issues.
 test(() => {
   convertTemplatesToShadowRootsWithin(host_open2);
   convertTemplatesToShadowRootsWithin(host_closed2);
@@ -169,6 +171,8 @@
 </div>
 
 <script>
+// TODO(crbug.com/920069): Remove this test when the
+// OffsetParentNewSpecBehavior feature has landed in stable with no issues.
 test(() => {
   convertTemplatesToShadowRootsWithin(host_open4);
   let container = root_open4.querySelector('#container');
@@ -201,6 +205,8 @@
 </div>
 
 <script>
+// TODO(crbug.com/920069): Remove this test when the
+// OffsetParentNewSpecBehavior feature has landed in stable with no issues.
 test(() => {
   convertTemplatesToShadowRootsWithin(host_closed5);
   assert_equals(inner_node5.offsetParent, null);
@@ -235,6 +241,8 @@
 </div>
 
 <script>
+// TODO(crbug.com/920069): Remove this test when the
+// OffsetParentNewSpecBehavior feature has landed in stable with no issues.
 test(() => {
   convertTemplatesToShadowRootsWithin(host_open6);
   assert_not_equals(inner_node6.offsetParent, null);

diff --git a/blink/web_tests/virtual/offsetparent-old-behavior/README.md b/blink/web_tests/virtual/offsetparent-old-behavior/README.md
@@ -0,0 +1,6 @@
+This is a virtual test suite for the *old* element.offsetParent behavior,
+to ensure it respects the Feature flag, in case this behavior needs to be
+disabled via Finch.
+
+Flag: --disable-features=OffsetParentNewSpecBehavior
+Bug: crbug.com/920069
diff --git a/...ent-across-shadow-boundaries-expected.txt → ...ent-across-shadow-boundaries-expected.txt b/...ent-across-shadow-boundaries-expected.txt → ...ent-across-shadow-boundaries-expected.txt
diff --git a/...setparent-old-behavior/fast/dom/shadow/offset-parent-does-not-leak-ua-shadow-expected.txt b/...setparent-old-behavior/fast/dom/shadow/offset-parent-does-not-leak-ua-shadow-expected.txt
@@ -0,0 +1,12 @@
+offsetParent should not leak nodes in user agent Shadow DOM.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+FAIL child.offsetParent should be [object HTMLDivElement]. Was [object HTMLDivElement].
+PASS child.offsetParent is container
+PASS child.offsetParent is container
+PASS child.offsetParent is container
+PASS successfullyParsed is true
+
+TEST COMPLETE
+