Auto merge of #14839 - Permutatrix:iss-12939, r=emilio

Make offset parent queries less buggy.

<!-- Please describe your changes on the following line: -->
Offset parent queries, which are used in the getters for HTMLElement's `offsetParent`, `offsetTop`, `offsetLeft`, `offsetWidth`, and `offsetHeight`, are pretty busted. The most egregious bug is that, as reported in #12939, inline elements are treated as if they're not present in the document. This PR fixes that and all of the other bugs I could trace directly to the offset parent query code, but `offsetTop` and `offsetLeft` are still unreliable in certain circumstances for reasons I haven't looked into (#13708). Inline elements with no content are still treated as not present due to #13982, so #13944 isn't fixed with this PR, either.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12939 and fix #12595

<!-- Either: -->
- [X] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14839)
<!-- Reviewable:end -->

components/layout/query.rs

@@ -14,6 +14,7 @@ use flow::{self, Flow};
 use fragment::{Fragment, FragmentBorderBoxIterator, SpecificFragmentInfo};
 use gfx::display_list::{DisplayItemMetadata, DisplayList, OpaqueNode, ScrollOffsetMap};
 use gfx_traits::ScrollRootId;
+use inline::LAST_FRAGMENT_OF_ELEMENT;
 use ipc_channel::ipc::IpcSender;
 use opaque_node::OpaqueNodeMethods;
 use script_layout_interface::rpc::{ContentBoxResponse, ContentBoxesResponse};
@@ -437,16 +438,20 @@ impl UnioningFragmentScrollAreaIterator {
     }
 }
 
+struct NodeOffsetBoxInfo {
+    offset: Point2D<Au>,
+    rectangle: Rect<Au>,
+}
+
 struct ParentBorderBoxInfo {
     node_address: OpaqueNode,
-    border_box: Rect<Au>,
+    origin: Point2D<Au>,
 }
 
 struct ParentOffsetBorderBoxIterator {
     node_address: OpaqueNode,
-    last_level: i32,
+    has_processed_node: bool,
-    has_found_node: bool,
+    node_offset_box: Option<NodeOffsetBoxInfo>,
-    node_border_box: Rect<Au>,
     parent_nodes: Vec<Option<ParentBorderBoxInfo>>,
 }
 
@@ -454,9 +459,8 @@ impl ParentOffsetBorderBoxIterator {
     fn new(node_address: OpaqueNode) -> ParentOffsetBorderBoxIterator {
         ParentOffsetBorderBoxIterator {
             node_address: node_address,
-            last_level: -1,
+            has_processed_node: false,
-            has_found_node: false,
+            node_offset_box: None,
-            node_border_box: Rect::zero(),
             parent_nodes: Vec::new(),
         }
     }
@@ -533,21 +537,81 @@ impl FragmentBorderBoxIterator for UnioningFragmentScrollAreaIterator {
 // https://drafts.csswg.org/cssom-view/#extensions-to-the-htmlelement-interface
 impl FragmentBorderBoxIterator for ParentOffsetBorderBoxIterator {
     fn process(&mut self, fragment: &Fragment, level: i32, border_box: &Rect<Au>) {
+        if self.node_offset_box.is_none() {
+            // We haven't found the node yet, so we're still looking
+            // for its parent. Remove all nodes at this level or
+            // higher, as they can't be parents of this node.
+            self.parent_nodes.truncate(level as usize);
+            assert_eq!(self.parent_nodes.len(), level as usize,
+                "Skipped at least one level in the flow tree!");
+        }
+
+        if !fragment.is_primary_fragment() {
+            // This fragment doesn't correspond to anything worth
+            // taking measurements from.
+
+            if self.node_offset_box.is_none() {
+                // If this is the only fragment in the flow, we need to
+                // do this to avoid failing the above assertion.
+                self.parent_nodes.push(None);
+            }
+
+            return;
+        }
+
         if fragment.node == self.node_address {
             // Found the fragment in the flow tree that matches the
             // DOM node being looked for.
-            self.has_found_node = true;
+
-            self.node_border_box = *border_box;
+            assert!(self.node_offset_box.is_none(),
+                "Node was being treated as inline, but it has an associated fragment!");
+
+            self.has_processed_node = true;
+            self.node_offset_box = Some(NodeOffsetBoxInfo {
+                offset: border_box.origin,
+                rectangle: *border_box,
+            });
 
             // offsetParent returns null if the node is fixed.
             if fragment.style.get_box().position == computed_values::position::T::fixed {
                 self.parent_nodes.clear();
             }
-        } else if level > self.last_level {
+        } else if let Some(node) = fragment.inline_context.as_ref().and_then(|inline_context| {
+            inline_context.nodes.iter().find(|node| node.address == self.node_address)
+        }) {
+            // TODO: Handle cases where the `offsetParent` is an inline
+            // element. This will likely be impossible until
+            // https://github.com/servo/servo/issues/13982 is fixed.
+
+            // Found a fragment in the flow tree whose inline context
+            // contains the DOM node we're looking for, i.e. the node
+            // is inline and contains this fragment.
+            match self.node_offset_box {
+                Some(NodeOffsetBoxInfo { ref mut rectangle, .. }) => {
+                    *rectangle = rectangle.union(border_box);
+                },
+                None => {
+                    // https://github.com/servo/servo/issues/13982 will
+                    // cause this assertion to fail sometimes, so it's
+                    // commented out for now.
+                    /*assert!(node.flags.contains(FIRST_FRAGMENT_OF_ELEMENT),
+                        "First fragment of inline node found wasn't its first fragment!");*/
+
+                    self.node_offset_box = Some(NodeOffsetBoxInfo {
+                        offset: border_box.origin,
+                        rectangle: *border_box,
+                    });
+                },
+            }
+
+            if node.flags.contains(LAST_FRAGMENT_OF_ELEMENT) {
+                self.has_processed_node = true;
+            }
+        } else if self.node_offset_box.is_none() {
             // TODO(gw): Is there a less fragile way of checking whether this
             // fragment is the body element, rather than just checking that
-            // the parent nodes stack contains the root node only?
+            // it's at level 1 (below the root node)?
-            let is_body_element = self.parent_nodes.len() == 1;
+            let is_body_element = level == 1;
 
             let is_valid_parent = match (is_body_element,
                                          fragment.style.get_box().position,
@@ -568,22 +632,22 @@ impl FragmentBorderBoxIterator for ParentOffsetBorderBoxIterator {
             };
 
             let parent_info = if is_valid_parent {
+                let border_width = fragment.border_width().to_physical(fragment.style.writing_mode);
+
                 Some(ParentBorderBoxInfo {
-                    border_box: *border_box,
                     node_address: fragment.node,
+                    origin: border_box.origin + Point2D::new(border_width.left, border_width.top),
                 })
             } else {
                 None
             };
 
             self.parent_nodes.push(parent_info);
-        } else if level < self.last_level {
-            self.parent_nodes.pop();
         }
     }
 
     fn should_process(&mut self, _: &Fragment) -> bool {
-        !self.has_found_node
+        !self.has_processed_node
     }
 }
 
@@ -804,18 +868,19 @@ pub fn process_offset_parent_query<N: LayoutNode>(requested_node: N, layout_root
         -> OffsetParentResponse {
     let mut iterator = ParentOffsetBorderBoxIterator::new(requested_node.opaque());
     sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root, &mut iterator);
-    let parent_info_index = iterator.parent_nodes.iter().rposition(|info| info.is_some());
+
-    match parent_info_index {
+    let node_offset_box = iterator.node_offset_box;
-        Some(parent_info_index) => {
+    let parent_info = iterator.parent_nodes.into_iter().rev().filter_map(|info| info).next();
-            let parent = iterator.parent_nodes[parent_info_index].as_ref().unwrap();
+    match (node_offset_box, parent_info) {
-            let origin = iterator.node_border_box.origin - parent.border_box.origin;
+        (Some(node_offset_box), Some(parent_info)) => {
-            let size = iterator.node_border_box.size;
+            let origin = node_offset_box.offset - parent_info.origin;
+            let size = node_offset_box.rectangle.size;
             OffsetParentResponse {
-                node_address: Some(parent.node_address.to_untrusted_node_address()),
+                node_address: Some(parent_info.node_address.to_untrusted_node_address()),
                 rect: Rect::new(origin, size),
             }
         }
-        None => {
+        _ => {
             OffsetParentResponse::empty()
         }
     }

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-001.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-001.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-002.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-002.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-003.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-003.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-004.htm.ini

@@ -0,0 +1,38 @@
+[seg-break-transformation-004.htm]
+  type: testharness
+  [linebreak only ₩２４]
+    expected: FAIL
+
+  [spaces linebreak ₩２４]
+    expected: FAIL
+
+  [linebreak spaces ₩２４]
+    expected: FAIL
+
+  [spaces linebreak spaces ₩２４]
+    expected: FAIL
+
+  [multiple linebreaks ₩２４]
+    expected: FAIL
+
+  [multiple linebreaks + spaces ₩２４]
+    expected: FAIL
+
+  [linebreak only ２４₩]
+    expected: FAIL
+
+  [spaces linebreak ２４₩]
+    expected: FAIL
+
+  [linebreak spaces ２４₩]
+    expected: FAIL
+
+  [spaces linebreak spaces ２４₩]
+    expected: FAIL
+
+  [multiple linebreaks ２４₩]
+    expected: FAIL
+
+  [multiple linebreaks + spaces ２４₩]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-006.htm.ini

@@ -0,0 +1,5 @@
+[seg-break-transformation-006.htm]
+  type: testharness
+  [spaces linebreak]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-008.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-008.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-009.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-009.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-016.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-016.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/seg-break-transformation-017.htm.ini

@@ -0,0 +1,20 @@
+[seg-break-transformation-017.htm]
+  type: testharness
+  [linebreak only]
+    expected: FAIL
+
+  [spaces linebreak]
+    expected: FAIL
+
+  [linebreak spaces]
+    expected: FAIL
+
+  [spaces linebreak spaces]
+    expected: FAIL
+
+  [multiple linebreaks]
+    expected: FAIL
+
+  [multiple linebreaks + spaces]
+    expected: FAIL
+

tests/wpt/metadata-css/css-text-3_dev/html/word-break-break-all-001.htm.ini

@@ -1,3 +0,0 @@
-[word-break-break-all-001.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-break-all-002.htm.ini

@@ -1,3 +0,0 @@
-[word-break-break-all-002.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-break-all-003.htm.ini

@@ -1,3 +0,0 @@
-[word-break-break-all-003.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-break-all-005.htm.ini

@@ -1,3 +0,0 @@
-[word-break-break-all-005.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-break-all-007.htm.ini

@@ -1,3 +1,3 @@
-[word-break-break-all-000.htm]
+[word-break-break-all-007.htm]
   type: reftest
   expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-keep-all-000.htm.ini

@@ -1,3 +0,0 @@
-[word-break-keep-all-000.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-keep-all-001.htm.ini

@@ -1,3 +0,0 @@
-[word-break-keep-all-001.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-keep-all-002.htm.ini

@@ -1,3 +0,0 @@
-[word-break-keep-all-002.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-001.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-001.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-bo-000.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-bo-000.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-en-000.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-en-000.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-hi-000.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-hi-000.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-ja-000.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-ja-000.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-ja-001.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-ja-001.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-ja-002.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-ja-002.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-ja-004.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-ja-004.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-ko-000.htm.ini

@@ -1,3 +0,0 @@
-[word-break-normal-ko-000.htm]
-  type: reftest
-  expected: FAIL

tests/wpt/metadata-css/css-text-3_dev/html/word-break-normal-zh-000.htm.ini

@@ -1,3 +1,4 @@
 [word-break-normal-zh-000.htm]
   type: reftest
-  expected: FAIL
+  expected:
+    if os == "mac": FAIL

tests/wpt/mozilla/meta/MANIFEST.json

@@ -6734,6 +6734,12 @@
             "url": "/_mozilla/css/meta_viewport_resize.html"
           }
         ],
+        "css/offset_properties_inline.html": [
+          {
+            "path": "css/offset_properties_inline.html",
+            "url": "/_mozilla/css/offset_properties_inline.html"
+          }
+        ],
         "css/test_variable_legal_values.html": [
           {
             "path": "css/test_variable_legal_values.html",

tests/wpt/mozilla/tests/css/offset_properties_inline.html

@@ -0,0 +1,83 @@
+<!DOCTYPE html>
+<meta charset="utf-8" />
+<title>cssom-view - offsetParent, offsetTop, offsetLeft, offsetWidth, and offsetHeight on inline elements</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  #real-offset-parent {
+    font: 10px/1 Ahem;
+    margin: 0;
+    border: none;
+    padding: 0;
+  }
+  #real-offset-parent, #decoy-offset-parent {
+    position: relative;
+  }
+</style>
+<body>
+<div id="real-offset-parent">
+  <span id="inline-1">ABC</span>
+  <span id="inline-2">ABC<br />ABC</span>
+  <span id="inline-3">ABC</span>
+</div>
+<div id="decoy-offset-parent">
+  <!--
+    Servo used to simply return the last valid offsetParent in the
+    document for inline nodes. This was often coincidentally the
+    correct result in contrived test cases such as this one. This
+    element is here to catch such bad behavior in the unlikely event
+    that it ever arises again.
+  -->
+</div>
+<script>
+  var realOffsetParent = document.getElementById('real-offset-parent');
+  var inline1 = document.getElementById('inline-1');
+  var inline2 = document.getElementById('inline-2');
+  var inline3 = document.getElementById('inline-3');
+
+  test(function() {
+    assert_equals(inline1.offsetParent, realOffsetParent,
+      "offsetParent of #inline-1 should be #real-offset-parent.");
+    assert_equals(inline2.offsetParent, realOffsetParent,
+      "offsetParent of #inline-2 should be #real-offset-parent.");
+    assert_equals(inline3.offsetParent, realOffsetParent,
+      "offsetParent of #inline-3 should be #real-offset-parent.");
+  }, "offsetParent");
+
+  test(function() {
+    assert_equals(inline1.offsetTop, 0,
+      "offsetTop of #inline-1 should be 0.");
+    assert_equals(inline2.offsetTop, 0,
+      "offsetTop of #inline-2 should be 0.");
+    assert_equals(inline3.offsetTop, 10,
+      "offsetTop of #inline-3 should be 10.");
+  }, "offsetTop");
+
+  test(function() {
+    assert_equals(inline1.offsetLeft, 0,
+      "offsetLeft of #inline-1 should be 0.");
+    assert_equals(inline2.offsetLeft, 40,
+      "offsetLeft of #inline-2 should be 40.");
+    assert_equals(inline3.offsetLeft, 40,
+      "offsetLeft of #inline-3 should be 40.");
+  }, "offsetLeft");
+
+  test(function() {
+    assert_equals(inline1.offsetWidth, 30,
+      "offsetWidth of #inline-1 should be 30.");
+    assert_equals(inline2.offsetWidth, 70,
+      "offsetWidth of #inline-2 should be 70.");
+    assert_equals(inline3.offsetWidth, 30,
+      "offsetWidth of #inline-3 should be 30.");
+  }, "offsetWidth");
+
+  test(function() {
+    assert_equals(inline1.offsetHeight, 10,
+      "offsetHeight of #inline-1 should be 10.");
+    assert_equals(inline2.offsetHeight, 20,
+      "offsetHeight of #inline-2 should be 20.");
+    assert_equals(inline3.offsetHeight, 10,
+      "offsetHeight of #inline-3 should be 10.");
+  }, "offsetHeight");
+</script>
+</body>