From 3b683eeec6dff41044de1ddc324fb64be4becafd Mon Sep 17 00:00:00 2001
From: Jean-Yves Moyen <jym@siteimprove.com>
Date: Wed, 10 Jan 2024 09:43:51 +0100
Subject: [PATCH] Fix tree traversal of flat tree (#1547)

* Fix tree traversal of flat tree

* Remove unused import

* Extract API

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
---
 .changeset/dirty-lizards-explode.md           |  5 ++
 docs/review/api/alfa-dom.api.md               |  4 --
 packages/alfa-dom/src/node.ts                 | 28 ++++++++
 packages/alfa-dom/src/node/element.ts         | 20 ------
 packages/alfa-dom/src/node/shadow.ts          |  2 +
 packages/alfa-dom/src/node/text.ts            | 22 -------
 .../src/node/traversal/get-nodes-between.ts   |  5 +-
 packages/alfa-dom/test/traversal.spec.tsx     | 66 +++++++++++++++++++
 packages/alfa-dom/tsconfig.json               |  5 +-
 9 files changed, 106 insertions(+), 51 deletions(-)
 create mode 100644 .changeset/dirty-lizards-explode.md
 create mode 100644 packages/alfa-dom/test/traversal.spec.tsx

diff --git a/.changeset/dirty-lizards-explode.md b/.changeset/dirty-lizards-explode.md
new file mode 100644
index 0000000000..eb799df63f
--- /dev/null
+++ b/.changeset/dirty-lizards-explode.md
@@ -0,0 +1,5 @@
+---
+"@siteimprove/alfa-dom": patch
+---
+
+**Fixed:** Parents of `Comment` inside a shadow tree now correctly skip over the shadow root when traversing the flat tree.
diff --git a/docs/review/api/alfa-dom.api.md b/docs/review/api/alfa-dom.api.md
index b758b4d793..385c50523b 100644
--- a/docs/review/api/alfa-dom.api.md
+++ b/docs/review/api/alfa-dom.api.md
@@ -289,8 +289,6 @@ export class Element<N extends string = string> extends Node<"element"> implemen
     // (undocumented)
     static of<N extends string = string>(namespace: Option<Namespace>, prefix: Option<string>, name: N, attributes?: Iterable_2<Attribute>, children?: Iterable_2<Node>, style?: Option<Block>, box?: Option<Rectangle>, device?: Option<Device>, externalId?: string, extraData?: any): Element<N>;
     // (undocumented)
-    parent(options?: Node.Traversal): Option<Node>;
-    // (undocumented)
     get prefix(): Option<string>;
     // (undocumented)
     get qualifiedName(): string;
@@ -1149,8 +1147,6 @@ export class Text extends Node<"text"> implements Slotable {
     // (undocumented)
     static of(data: string, externalId?: string, extraData?: any): Text;
     // (undocumented)
-    parent(options?: Node.Traversal): Option<Node>;
-    // (undocumented)
     toJSON(): Text.JSON;
     // (undocumented)
     toString(): string;
diff --git a/packages/alfa-dom/src/node.ts b/packages/alfa-dom/src/node.ts
index 9aecde7f7c..9c85730e97 100644
--- a/packages/alfa-dom/src/node.ts
+++ b/packages/alfa-dom/src/node.ts
@@ -22,6 +22,7 @@ import {
   Fragment,
   Shadow,
   Slot,
+  Slotable,
   Text,
   Type,
 } from ".";
@@ -135,6 +136,33 @@ export abstract class Node<T extends string = string>
       });
   }
 
+  public parent(options: Node.Traversal = Node.Traversal.empty): Option<Node> {
+    const parent = this._parent as Option<Node>;
+
+    // If we traverse the flat tree, we want to jump over shadow roots.
+    if (options.isSet(Node.Traversal.flattened)) {
+      return parent.flatMap((parent) => {
+        if (Shadow.isShadow(parent)) {
+          return parent.host;
+        }
+
+        // Additionally, if this is a slottable light child of a shadow host, we want
+        // to search for where it is slotted, and return that parent instead.
+        if (
+          Element.isElement(parent) &&
+          parent.shadow.isSome() &&
+          Slotable.isSlotable(this)
+        ) {
+          return this.assignedSlot().flatMap((slot) => slot.parent(options));
+        }
+
+        return Option.of(parent);
+      });
+    }
+
+    return parent;
+  }
+
   private _path: Array<string> = [];
 
   /**
diff --git a/packages/alfa-dom/src/node/element.ts b/packages/alfa-dom/src/node/element.ts
index c02c9c7162..a981f2d359 100644
--- a/packages/alfa-dom/src/node/element.ts
+++ b/packages/alfa-dom/src/node/element.ts
@@ -161,26 +161,6 @@ export class Element<N extends string = string>
     return this._boxes.get(device);
   }
 
-  public parent(options: Node.Traversal = Node.Traversal.empty): Option<Node> {
-    const parent = this._parent as Option<Node>;
-
-    if (options.isSet(Node.Traversal.flattened)) {
-      return parent.flatMap((parent) => {
-        if (Shadow.isShadow(parent)) {
-          return parent.host;
-        }
-
-        if (Element.isElement(parent) && parent.shadow.isSome()) {
-          return this.assignedSlot().flatMap((slot) => slot.parent(options));
-        }
-
-        return Option.of(parent);
-      });
-    }
-
-    return parent;
-  }
-
   public children(
     options: Node.Traversal = Node.Traversal.empty,
   ): Sequence<Node> {
diff --git a/packages/alfa-dom/src/node/shadow.ts b/packages/alfa-dom/src/node/shadow.ts
index ee6ed5fed0..ea28639981 100644
--- a/packages/alfa-dom/src/node/shadow.ts
+++ b/packages/alfa-dom/src/node/shadow.ts
@@ -61,6 +61,8 @@ export class Shadow extends Node<"shadow"> {
   }
 
   public parent(options: Node.Traversal = Node.Traversal.empty): Option<Node> {
+    // We only "land" on Shadow roots when traversing the composed tree.
+    // Notably, flattening the tree "jumps" over them.
     if (options.isSet(Node.Traversal.composed)) {
       return this._host;
     }
diff --git a/packages/alfa-dom/src/node/text.ts b/packages/alfa-dom/src/node/text.ts
index db4a2e86b0..1267ce6bb9 100644
--- a/packages/alfa-dom/src/node/text.ts
+++ b/packages/alfa-dom/src/node/text.ts
@@ -2,8 +2,6 @@ import { Option } from "@siteimprove/alfa-option";
 import { Trampoline } from "@siteimprove/alfa-trampoline";
 
 import { Node } from "../node";
-import { Element } from "./element";
-import { Shadow } from "./shadow";
 import { Slot } from "./slot";
 import { Slotable } from "./slotable";
 
@@ -31,26 +29,6 @@ export class Text extends Node<"text"> implements Slotable {
     return this._data;
   }
 
-  public parent(options: Node.Traversal = Node.Traversal.empty): Option<Node> {
-    const parent = this._parent as Option<Node>;
-
-    if (options.isSet(Node.Traversal.flattened)) {
-      return parent.flatMap((parent) => {
-        if (Shadow.isShadow(parent)) {
-          return parent.host;
-        }
-
-        if (Element.isElement(parent) && parent.shadow.isSome()) {
-          return this.assignedSlot().flatMap((slot) => slot.parent(options));
-        }
-
-        return Option.of(parent);
-      });
-    }
-
-    return parent;
-  }
-
   public assignedSlot(): Option<Slot> {
     return Slotable.findSlot(this);
   }
diff --git a/packages/alfa-dom/src/node/traversal/get-nodes-between.ts b/packages/alfa-dom/src/node/traversal/get-nodes-between.ts
index 105279fc0e..0897bf9448 100755
--- a/packages/alfa-dom/src/node/traversal/get-nodes-between.ts
+++ b/packages/alfa-dom/src/node/traversal/get-nodes-between.ts
@@ -1,6 +1,6 @@
-import { Node } from "../..";
 import { Predicate } from "@siteimprove/alfa-predicate";
 import { Sequence } from "@siteimprove/alfa-sequence";
+import { Node } from "../..";
 
 import { lowestCommonAncestor } from "./lowest-common-ancestor";
 
@@ -43,7 +43,7 @@ export function getNodesBetween(
     // of the closest ancestor having one.
     between = first
       // Closest ancestor with a next sibling.
-      .closest((ancestor) => ancestor.next(treeOptions).isSome())
+      .closest((ancestor) => ancestor.next(treeOptions).isSome(), treeOptions)
       // Get that sibling.
       .flatMap((node) => node.next(treeOptions))
       // Skip everything until next.
@@ -71,7 +71,6 @@ function getNodesInclusivelyBetween(
 ): Sequence<Node> {
   const isFrontier = or(equals(node1), equals(node2));
 
-  // Get descendants of the LCA, and skip everything before and after both nodes.
   return lowestCommonAncestor(node1, node2, treeOptions)
     .map((context) =>
       context
diff --git a/packages/alfa-dom/test/traversal.spec.tsx b/packages/alfa-dom/test/traversal.spec.tsx
new file mode 100644
index 0000000000..33c1b1922a
--- /dev/null
+++ b/packages/alfa-dom/test/traversal.spec.tsx
@@ -0,0 +1,66 @@
+import { None } from "@siteimprove/alfa-option";
+import { test } from "@siteimprove/alfa-test";
+
+import { h, Comment, Node } from "../src";
+
+const targets = [<span />, h.text("hello"), Comment.of("world")];
+const shadow = h.shadow(targets);
+const div = <div>{shadow}</div>;
+
+// Start with no flag.
+// For each of the three flags, duplicate existing list and add the flag to one side.
+// This ends up giving the eight possible options.
+const flags = [
+  Node.Traversal.composed,
+  Node.Traversal.flattened,
+  Node.Traversal.nested,
+].reduce(
+  (old, cur) => old.flatMap((flag) => [flag, flag.add(cur)]),
+  [Node.Traversal.of(Node.Traversal.none)],
+);
+
+test(".parent() of a shadow host returns None in composed traversal, the shadow root otherwise", (t) => {
+  for (const traversal of flags) {
+    if (traversal.has(Node.Traversal.composed)) {
+      t.deepEqual(shadow.parent(traversal).getUnsafe(), div);
+    } else {
+      t.deepEqual(shadow.parent(traversal), None);
+    }
+  }
+});
+
+test(".parent() of the children of a shadow root returns the shadow host in flat traversal, the shadow root otherwise", (t) => {
+  for (const target of targets) {
+    for (const traversal of flags) {
+      if (traversal.has(Node.Traversal.flattened)) {
+        t.deepEqual(target.parent(traversal).getUnsafe(), div);
+      } else {
+        t.deepEqual(target.parent(traversal).getUnsafe(), shadow);
+      }
+    }
+  }
+});
+
+test(".parent() of a slottable child of a shadow host returns the slot's parent in flat traversal, the light parent otherwise", (t) => {
+  const target = <span slot="foo" />;
+  const shadowDiv = (
+    <div>
+      <slot name="foo"></slot>
+    </div>
+  );
+  const shadow = h.shadow([shadowDiv]);
+  const lightDiv = (
+    <div>
+      {shadow}
+      {target}
+    </div>
+  );
+
+  for (const traversal of flags) {
+    if (traversal.has(Node.Traversal.flattened)) {
+      t.deepEqual(target.parent(traversal).getUnsafe(), shadowDiv);
+    } else {
+      t.deepEqual(target.parent(traversal).getUnsafe(), lightDiv);
+    }
+  }
+});
diff --git a/packages/alfa-dom/tsconfig.json b/packages/alfa-dom/tsconfig.json
index 6eba504794..a096aaf300 100644
--- a/packages/alfa-dom/tsconfig.json
+++ b/packages/alfa-dom/tsconfig.json
@@ -86,7 +86,8 @@
     "test/node/traversal/get-nodes-between.spec.tsx",
     "test/node/traversal/lowest-common-ancestor.spec.tsx",
     "test/node/query/element-descendants.spec.tsx",
-    "test/node/query/element-id-map.spec.tsx"
+    "test/node/query/element-id-map.spec.tsx",
+    "test/traversal.spec.tsx"
   ],
   "references": [
     { "path": "../alfa-array" },
@@ -103,7 +104,7 @@
     { "path": "../alfa-map" },
     { "path": "../alfa-option" },
     { "path": "../alfa-predicate" },
-    { "path": "../alfa-rectangle"},
+    { "path": "../alfa-rectangle" },
     { "path": "../alfa-refinement" },
     { "path": "../alfa-sarif" },
     { "path": "../alfa-selective" },