Implement Element::has_css_layout_box()

Calling scroll() on an element which is not rendered (by a parent with display: none) would previously cause a crash. In fact, we should terminate the algorithm [https://drafts.csswg.org/cssom-view/#dom-element-scroll] at step 10 in this situation. The fix hinges on implementing Element::has_css_layout_box() correctly, rather than just returning true in all cases as we did previously. Fixes #19430.
servo · Jan 18, 2018 · 902edb9 · 902edb9
1 parent 9f2b33c
commit 902edb9
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 4 deletions.
diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs
@@ -349,11 +349,12 @@ impl Element {
     }
 
     // https://drafts.csswg.org/cssom-view/#css-layout-box
-    // Elements that have a computed value of the display property
-    // that is table-column or table-column-group
-    // FIXME: Currently, it is assumed to be true always
+    //
+    // We'll have no content box if there's no fragment for the node, and we use
+    // bounding_content_box, for simplicity, to detect this (rather than making a more specific
+    // query to the layout thread).
     fn has_css_layout_box(&self) -> bool {
-        true
+        self.upcast::<Node>().bounding_content_box().is_some()
     }
 
     // https://drafts.csswg.org/cssom-view/#potentially-scrollable

diff --git a/tests/wpt/metadata/MANIFEST.json b/tests/wpt/metadata/MANIFEST.json
@@ -308101,6 +308101,12 @@
      {}
     ]
    ],
+   "css/cssom-view/scroll-no-layout-box.html": [
+    [
+     "/css/cssom-view/scroll-no-layout-box.html",
+     {}
+    ]
+   ],
    "css/cssom-view/scrollIntoView-shadow.html": [
     [
      "/css/cssom-view/scrollIntoView-shadow.html",
@@ -516726,6 +516732,10 @@
    "966ebff69f91c0ea92f4bc2f943df9ef9dc3bcf9",
    "testharness"
   ],
+  "css/cssom-view/scroll-no-layout-box.html": [
+   "8f423a757bd716c9c22e82fd2f35007d05982c08",
+   "testharness"
+  ],
   "css/cssom-view/scrollIntoView-shadow.html": [
    "3c4a18992105fd7bf19cbf29f0b6d80cb12ca98c",
    "testharness"

diff --git a/tests/wpt/web-platform-tests/css/cssom-view/scroll-no-layout-box.html b/tests/wpt/web-platform-tests/css/cssom-view/scroll-no-layout-box.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<title>cssom-view - Scrolling element with no layout box</title>
+<link rel="help" href="https://drafts.csswg.org/cssom-view/#dom-element-scroll">
+<link rel="help" href="https://drafts.csswg.org/cssom-view/#css-layout-box">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<div style="display: none">
+  <div id="elem"></div>
+</div>
+
+<script>
+test(() => {
+  const elem = document.getElementById('elem');
+  elem.scroll(1, 2);
+
+  assert_equals(elem.scrollTop, 0, "scrollTop should be unchanged");
+  assert_equals(elem.scrollLeft, 0, "scrollLeft should be unchanged");
+}, "scrolling an element with no CSS layout box should have no effect");
+</script>