SIA-R111 and SIA-R113 applicability improvements (#1594)

* Update applicability to reject paragraph subtrees * Add function for getting all targets needed for spacing calculation * Use generator function * Add cache again * Add changeset * Update slimy-socks-cry.md * Update .changeset/slimy-socks-cry.md Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com> * Factor target predicate * Add tests --------- Co-authored-by: Jean-Yves Moyen <jym@siteimprove.com>
Siteimprove · Apr 10, 2024 · 62e778e · 62e778e
1 parent ffcacbc
commit 62e778e
Show file tree

Hide file tree

Showing 5 changed files with 206 additions and 25 deletions.
diff --git a/.changeset/slimy-socks-cry.md b/.changeset/slimy-socks-cry.md
@@ -0,0 +1,5 @@
+---
+"@siteimprove/alfa-rules": patch
+---
+
+**Fixed:** R111 and R113 are no longer applicable to invisible targets and targets inside a paragraph
diff --git a/packages/alfa-rules/src/common/applicability/targets-of-pointer-events.ts b/packages/alfa-rules/src/common/applicability/targets-of-pointer-events.ts
@@ -1,38 +1,97 @@
 import { DOM } from "@siteimprove/alfa-aria";
 import { Cache } from "@siteimprove/alfa-cache";
 import { Device } from "@siteimprove/alfa-device";
-import { Document, Element, Node, Query } from "@siteimprove/alfa-dom";
+import { Document, Element, Node } from "@siteimprove/alfa-dom";
 import { Predicate } from "@siteimprove/alfa-predicate";
 import { Sequence } from "@siteimprove/alfa-sequence";
 import { Style } from "@siteimprove/alfa-style";
+import { Query } from "@siteimprove/alfa-dom";
 
 const { hasRole } = DOM;
-const { hasComputedStyle, isFocusable } = Style;
+const { hasComputedStyle, isFocusable, isVisible } = Style;
+
+const { and, not } = Predicate;
 
-const { and } = Predicate;
 const { getElementDescendants } = Query;
 
-const cache = Cache.empty<Document, Cache<Device, Sequence<Element>>>();
+const applicabilityCache = Cache.empty<
+  Document,
+  Cache<Device, Sequence<Element>>
+>();
 
 /**
  * @internal
  */
-export function targetsOfPointerEvents(
+export function applicableTargetsOfPointerEvents(
   document: Document,
   device: Device,
 ): Sequence<Element> {
-  return cache.get(document, Cache.empty).get(device, () =>
-    getElementDescendants(document, Node.fullTree).filter(
-      and(
-        hasComputedStyle(
-          "pointer-events",
-          (keyword) => keyword.value !== "none",
-          device,
-        ),
-        isFocusable(device),
-        hasRole(device, (role) => role.isWidget()),
-        (target) => target.getBoundingBox(device).isSome(),
-      ),
+  return applicabilityCache.get(document, Cache.empty).get(device, () => {
+    const isParagraph = hasRole(device, "paragraph");
+    const isArea = (element: Element) => element.name === "area";
+
+    function* visit(node: Node): Iterable<Element> {
+      if (Element.isElement(node)) {
+        if (isParagraph(node)) {
+          // If we encounter a paragraph, we can skip the entire subtree
+          return;
+        }
+
+        // TODO: It's not enough to reject paragraphs, we need to reject all text blocks in order to avoid false positives
+
+        if (and(isTarget(device), not(isArea))(node)) {
+          yield node;
+        }
+      }
+
+      for (const child of node.children(Node.fullTree)) {
+        yield* visit(child);
+      }
+    }
+
+    return Sequence.from(visit(document));
+  });
+}
+
+const allTargetsCache = Cache.empty<
+  Document,
+  Cache<Device, Sequence<Element>>
+>();
+
+/**
+ * @internal
+ *
+ * @privateRemarks
+ * This function is not used in the applicability of R111 or R113,
+ * but in the expectation of R113 since all other targets are needed
+ * to determine if an applicable target is underspaced.
+ * It's kept here since it's closely related to the applicability.
+ */
+export function allTargetsOfPointerEvents(
+  document: Document,
+  device: Device,
+): Sequence<Element> {
+  return allTargetsCache
+    .get(document, Cache.empty)
+    .get(device, () =>
+      getElementDescendants(document, Node.fullTree).filter(isTarget(device)),
+    );
+}
+
+function isTarget(device: Device): Predicate<Element> {
+  return and(
+    hasComputedStyle(
+      "pointer-events",
+      (keyword) => keyword.value !== "none",
+      device,
     ),
+    isFocusable(device),
+    isVisible(device),
+    hasRole(device, (role) => role.isWidget()),
+    hasBoundingBox(device),
   );
 }
+
+function hasBoundingBox(device: Device): Predicate<Element> {
+  return (element) => element.getBoundingBox(device).isSome();
+}
diff --git a/packages/alfa-rules/src/sia-r111/rule.ts b/packages/alfa-rules/src/sia-r111/rule.ts
@@ -8,7 +8,7 @@ import { Page } from "@siteimprove/alfa-web";
 
 import { expectation } from "../common/act/expectation";
 
-import { targetsOfPointerEvents } from "../common/applicability/targets-of-pointer-events";
+import { applicableTargetsOfPointerEvents } from "../common/applicability/targets-of-pointer-events";
 
 import { WithBoundingBox, WithName } from "../common/diagnostic";
 
@@ -21,7 +21,7 @@ export default Rule.Atomic.of<Page, Element>({
   evaluate({ device, document }) {
     return {
       applicability() {
-        return targetsOfPointerEvents(document, device);
+        return applicableTargetsOfPointerEvents(document, device);
       },
 
       expectations(target) {

diff --git a/packages/alfa-rules/src/sia-r113/rule.ts b/packages/alfa-rules/src/sia-r113/rule.ts
@@ -12,7 +12,10 @@ import { Page } from "@siteimprove/alfa-web";
 
 import { expectation } from "../common/act/expectation";
 
-import { targetsOfPointerEvents } from "../common/applicability/targets-of-pointer-events";
+import {
+  applicableTargetsOfPointerEvents,
+  allTargetsOfPointerEvents,
+} from "../common/applicability/targets-of-pointer-events";
 
 import { WithBoundingBox, WithName } from "../common/diagnostic";
 
@@ -25,7 +28,7 @@ export default Rule.Atomic.of<Page, Element>({
   evaluate({ device, document }) {
     return {
       applicability() {
-        return targetsOfPointerEvents(document, device);
+        return applicableTargetsOfPointerEvents(document, device);
       },
 
       expectations(target) {
@@ -126,7 +129,6 @@ const undersizedCache = Cache.empty<
   Document,
   Cache<Device, Sequence<Element>>
 >();
-
 /**
  * Yields all elements that have insufficient spacing to the target.
  *
@@ -148,15 +150,15 @@ function* findElementsWithInsufficientSpacingToTarget(
   const undersizedTargets = undersizedCache
     .get(document, Cache.empty)
     .get(device, () =>
-      targetsOfPointerEvents(document, device).reject(
+      allTargetsOfPointerEvents(document, device).reject(
         hasSufficientSize(24, device),
       ),
     );
 
   // TODO: This needs to be optimized, we should be able to use some spatial data structure like a quadtree to reduce the number of comparisons
-  for (const candidate of targetsOfPointerEvents(document, device)) {
+  for (const candidate of allTargetsOfPointerEvents(document, device)) {
     if (target !== candidate) {
-      // Existence of a bounding box is guaranteed by applicability
+      // Existence of a bounding box should be guaranteed by implementation of allTargetsOfPointerEvents
       const candidateRect = candidate.getBoundingBox(device).getUnsafe();
 
       if (

diff --git a/packages/alfa-rules/test/sia-r113/rule.spec.tsx b/packages/alfa-rules/test/sia-r113/rule.spec.tsx
@@ -114,6 +114,74 @@ test("evaluate() passes button with clickable area of less than 24x24 pixels and
   ]);
 });
 
+test("evaluate() passes undersized button with vertically adjacent undersized button that is not displayed", async (t) => {
+  const device = Device.standard();
+
+  // The 24px diameter circles of the targets does not intersect with the bounding box of the other target, but the circles do intersect
+  const target1 = (
+    <button
+      style={{ position: "absolute", top: "80px", left: "80px", width: "20px", height: "20px", borderRadius: "0" }}
+      box={{ device, x: 80, y: 80, width: 20, height: 20 }}
+    >
+      Hello
+    </button>
+  );
+
+  const target2 = (
+    <button
+      style={{ position: "absolute", top: "58px", left: "80px", width: "20px", height: "20px", borderRadius: "0", display: "none" }}
+      box={{ device, x: 80, y: 58, width: 20, height: 20 }}
+    >
+      World
+    </button>
+  );
+
+  const document = h.document([target1, target2]);
+
+  t.deepEqual(await evaluate(R113, { document, device }), [
+    passed(R113, target1, {
+      1: Outcomes.HasSufficientSpacing(
+        "Hello",
+        target1.getBoundingBox(device).getUnsafe(),
+      ),
+    }),
+  ]);
+});
+
+test("evaluate() passes undersized button with vertically adjacent undersized button that is hidden", async (t) => {
+  const device = Device.standard();
+
+  // The 24px diameter circles of the targets does not intersect with the bounding box of the other target, but the circles do intersect
+  const target1 = (
+    <button
+      style={{ position: "absolute", top: "80px", left: "80px", width: "20px", height: "20px", borderRadius: "0" }}
+      box={{ device, x: 80, y: 80, width: 20, height: 20 }}
+    >
+      Hello
+    </button>
+  );
+
+  const target2 = (
+    <button
+      style={{ position: "absolute", top: "58px", left: "80px", width: "20px", height: "20px", borderRadius: "0", visibility: "hidden" }}
+      box={{ device, x: 80, y: 58, width: 20, height: 20 }}
+    >
+      World
+    </button>
+  );
+
+  const document = h.document([target1, target2]);
+
+  t.deepEqual(await evaluate(R113, { document, device }), [
+    passed(R113, target1, {
+      1: Outcomes.HasSufficientSpacing(
+        "Hello",
+        target1.getBoundingBox(device).getUnsafe(),
+      ),
+    }),
+  ]);
+});
+
 test("evaluate() fails undersized button with vertically adjacent undersized button", async (t) => {
   const device = Device.standard();
 
@@ -250,4 +318,51 @@ test("evaluate() is inapplicable when there is no layout information", async (t)
   t.deepEqual(await evaluate(R113, { document, device }), [inapplicable(R113)]);
 });
 
+test("evaluate() is inapplicable to <area> elements", async (t) => {
+  const device = Device.standard();
+
+  const img = <img src="foo.jpg" alt="foo" usemap="#bar" width="500" />;
+  const map = <map name="bar"><area shape="rect" coords="8,8,31,31" href="foo.html" box={{ device, x: 8, y: 8, width: 0, height: 0 }} /></map>;
+
+  const document = h.document([img, map]);
+
+  t.deepEqual(await evaluate(R113, { document, device }), [inapplicable(R113)]);
+});
+
+test("evaluate() is inapplicable to button with display: none", async (t) => {
+  const device = Device.standard();
+
+  const target = (
+    <button
+      style={{ width: "24px", height: "24px", display: "none" }}
+      box={{ device, x: 8, y: 8, width: 24, height: 24 }}
+    >
+      Hello
+    </button>
+  );
+
+  const document = h.document([target]);
+
+  t.deepEqual(await evaluate(R113, { document, device }), [
+    inapplicable(R113),
+  ]);
+});
+
+test("evaluate() is inapplicable to button with visibility: hidden", async (t) => {
+  const device = Device.standard();
 
+  const target = (
+    <button
+      style={{ width: "24px", height: "24px", visibility: "hidden" }}
+      box={{ device, x: 8, y: 8, width: 24, height: 24 }}
+    >
+      Hello
+    </button>
+  );
+
+  const document = h.document([target]);
+
+  t.deepEqual(await evaluate(R113, { document, device }), [
+    inapplicable(R113),
+  ]);
+});