[css-pseudo] Fix hit-testing for nested ::marker

Since r731964, if you clicked a ::marker originated by a ::before or
::after, and you read the 'path' property of the event, the first node
in the array might be the ::before or ::after pseudo-element.

Actually it was fine if you clicked the contents (text or image) of the
::marker. But it's possible to make the ::marker taller than its
contents by setting a big 'line-height'. Then you could click inside the
::marker but outside its contents, and reproduce the problem.

That was wrong, as described in pseudo_element.h,
> Pseudo element are not allowed to be the inner node for hit testing.

This patch fixes it by making PseudoElement::InnerNodeForHitTesting
iterate ancestors until it finds one which is not a pseudo-element.

Bug: 1048000, 457718

TEST=external/wpt/css/css-pseudo/marker-hit-testing.html

Some checks fail in legacy because the marker ignores 'line-height'.

Change-Id: I7bed7d0824638b0c7f4b63a744a3ca4978285445
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030973Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#737720}

third_party/blink/renderer/core/dom/pseudo_element.cc

@@ -240,7 +240,10 @@ bool PseudoElement::CanGeneratePseudoElement(PseudoId pseudo_id) const {
 }
 
 Node* PseudoElement::InnerNodeForHitTesting() const {
-  return ParentOrShadowHostNode();
+  Node* parent = ParentOrShadowHostNode();
+  if (parent && parent->IsPseudoElement())
+    return To<PseudoElement>(parent)->InnerNodeForHitTesting();
+  return parent;
 }
 
 bool PseudoElementLayoutObjectIsNeeded(const ComputedStyle* pseudo_style,

third_party/blink/renderer/core/layout/layout_object.cc

@@ -3426,17 +3426,12 @@ Node* LayoutObject::NodeForHitTest() const {
   // If we hit the anonymous layoutObjects inside generated content we should
   // actually hit the generated content so walk up to the PseudoElement.
   if (const LayoutObject* parent = Parent()) {
-    if (parent->IsBeforeOrAfterContent() ||
+    if (parent->IsBeforeOrAfterContent() || parent->IsMarkerContent() ||
         parent->StyleRef().StyleType() == kPseudoIdFirstLetter) {
       for (; parent; parent = parent->Parent()) {
         if (Node* node = parent->GetNode())
           return node;
       }
-    } else if (const LayoutNGListItem* list_item =
-                   LayoutNGListItem::FromMarkerOrMarkerContent(*this)) {
-      // If this is a list marker, or is inside of a list marker, return the
-      // list item.
-      return list_item->GetNode();
     }
   }
 

third_party/blink/renderer/core/layout/ng/list/layout_ng_list_item.cc

@@ -91,22 +91,6 @@ void LayoutNGListItem::UpdateMarkerTextIfNeeded() {
     list_marker->UpdateMarkerTextIfNeeded(*marker);
 }
 
-LayoutNGListItem* LayoutNGListItem::FromMarkerOrMarkerContent(
-    const LayoutObject& object) {
-  DCHECK(object.IsAnonymous());
-
-  if (const ListMarker* marker = ListMarker::Get(&object))
-    return marker->ListItem(object);
-
-  // Check if this is a marker content.
-  if (const LayoutObject* parent = object.Parent()) {
-    if (const ListMarker* marker = ListMarker::Get(parent))
-      return marker->ListItem(*parent);
-  }
-
-  return nullptr;
-}
-
 int LayoutNGListItem::Value() const {
   DCHECK(GetNode());
   return ordinal_.Value(*GetNode());

third_party/blink/renderer/core/layout/ng/list/layout_ng_list_item.h

@@ -35,9 +35,6 @@ class CORE_EXPORT LayoutNGListItem final : public LayoutNGBlockFlow {
 
   static const LayoutObject* FindSymbolMarkerLayoutText(const LayoutObject*);
 
-  // Find the LayoutNGListItem from a marker.
-  static LayoutNGListItem* FromMarkerOrMarkerContent(const LayoutObject&);
-
   const char* GetName() const override { return "LayoutNGListItem"; }
 
  private:

third_party/blink/renderer/core/layout/ng/list/layout_ng_list_marker_image.cc

@@ -24,18 +24,6 @@ bool LayoutNGListMarkerImage::IsOfType(LayoutObjectType type) const {
   return type == kLayoutObjectNGListMarkerImage || LayoutImage::IsOfType(type);
 }
 
-Node* LayoutNGListMarkerImage::NodeForHitTest() const {
-  // In LayoutNG tree, image list marker is structured like this:
-  // <li> (LayoutListItem)
-  //   ::marker (LayoutNGListMarker or LayoutNGInsideListMarker)
-  //     <anonymous img> (LayoutNGListMarkerImage)
-  // Hit testing should return the list-item node.
-  DCHECK(!GetNode());
-  const LayoutNGListItem* list_item =
-      LayoutNGListItem::FromMarkerOrMarkerContent(*this);
-  return list_item ? list_item->GetNode() : nullptr;
-}
-
 // Because ImageResource() is always LayoutImageResourceStyleImage. So we could
 // use StyleImage::ImageSize to determine the concrete object size with
 // default object size(ascent/2 x ascent/2).

third_party/blink/renderer/core/layout/ng/list/layout_ng_list_marker_image.h

@@ -19,8 +19,6 @@ class CORE_EXPORT LayoutNGListMarkerImage final : public LayoutImage {
 
   bool IsLayoutNGObject() const override { return true; }
 
-  Node* NodeForHitTest() const final;
-
  private:
   bool IsOfType(LayoutObjectType) const override;
 

third_party/blink/web_tests/external/wpt/css/css-pseudo/marker-hit-testing.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Pseudo-Elements Test: Hit testing ::marker</title>
+<link rel="help" href="https://drafts.csswg.org/css-pseudo-4/#marker-pseudo">
+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com">
+<meta name="assert" content="This test checks that hit-testing a ::marker, the APIs provide the nearest element ancestor." />
+<link rel="stylesheet" type="text/css" href="/fonts/ahem.css" />
+<style>
+ol {
+  display: inline-block;
+  padding-left: 100px;
+}
+li {
+  font: 50px/100px Ahem;
+  width: 50px;
+}
+.inside {
+  list-style-position: inside;
+}
+.image {
+  list-style-image: url("/images/green-100x50.png");
+}
+.string {
+  list-style-type: "X";
+}
+.marker::marker {
+  content: "X";
+}
+.nested {
+  display: block;
+}
+.nested::before {
+  content: '';
+  display: list-item;
+}
+</style>
+<!-- The <li> are 50px wide, and the ::marker are at least 50px wide.
+     Since they are outside, try to locate them at -40px to the left of
+     the <li>, i.e. -65px from the center of the <li> -->
+<ol class="outside" data-x="-65">
+  <li class="image"></li>
+  <li class="string"></li>
+  <li class="marker"></li>
+  <li class="nested image"></li>
+  <li class="nested string"></li>
+</ol>
+<!-- The <li> are 50px wide, and the inside ::marker are at least 50px,
+     so locate them at the horizontal center of the <li> -->
+<ol class="inside" data-x="0">
+  <li class="image"></li>
+  <li class="string"></li>
+  <li class="marker"></li>
+  <li class="nested image"></li>
+  <li class="nested string"></li>
+</ol>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-actions.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+<script>
+function check(event, li) {
+  assert_equals(event.target, li, "event.target");
+  if (event.path) {
+    assert_equals(event.path[0], li, "event.path");
+  }
+  const el = document.elementFromPoint(event.clientX, event.clientY);
+  assert_equals(el, li, "elementFromPoint");
+}
+(async function() {
+  setup({ explicit_done: true });
+  for (let list of document.querySelectorAll("ol")) {
+    for (let li of list.querySelectorAll("li")) {
+      const listener = e => check(e, li);
+      async_test(function(t) {
+        document.addEventListener("mousedown", t.step_func_done(listener));
+      }, list.className + " " + li.className + " ::marker's content");
+      async_test(function(t) {
+        document.addEventListener("mouseup", t.step_func_done(listener));
+      }, list.className + " " + li.className + " ::marker");
+      await new test_driver.Actions()
+        // Send an event at the vertical middle of the <li>, this should
+        // hit the contents of the ::marker
+        .pointerMove(+list.dataset.x, 0, {origin: li})
+        .pointerDown()
+        // The ::marker is 100px tall but its contents only 50px tall.
+        // Send an event inside the ::marker but above its contents.
+        .pointerMove(+list.dataset.x, -40, {origin: li})
+        .pointerUp()
+        .send();
+    }
+  }
+  done();
+})();
+</script>
+

third_party/blink/web_tests/flag-specific/disable-layout-ng/external/wpt/css/css-pseudo/marker-hit-testing-expected.txt

+This is a testharness.js-based test.
+PASS outside image ::marker's content
+FAIL outside image ::marker assert_equals: event.target expected Element node <li class="image"></li> but got Element node <ol class="outside" data-x="-65">
+  <li class="image"></l...
+PASS outside string ::marker's content
+FAIL outside string ::marker assert_equals: event.target expected Element node <li class="string"></li> but got Element node <ol class="outside" data-x="-65">
+  <li class="image"></l...
+PASS outside marker ::marker's content
+FAIL outside marker ::marker assert_equals: event.target expected Element node <li class="marker"></li> but got Element node <ol class="outside" data-x="-65">
+  <li class="image"></l...
+PASS outside nested image ::marker's content
+FAIL outside nested image ::marker assert_equals: event.target expected Element node <li class="nested image"></li> but got Element node <ol class="outside" data-x="-65">
+  <li class="image"></l...
+PASS outside nested string ::marker's content
+FAIL outside nested string ::marker assert_equals: event.target expected Element node <li class="nested string"></li> but got Element node <ol class="outside" data-x="-65">
+  <li class="image"></l...
+PASS inside image ::marker's content
+PASS inside image ::marker
+PASS inside string ::marker's content
+PASS inside string ::marker
+PASS inside marker ::marker's content
+PASS inside marker ::marker
+PASS inside nested image ::marker's content
+PASS inside nested image ::marker
+PASS inside nested string ::marker's content
+PASS inside nested string ::marker
+Harness: the test ran to completion.
+