diff --git a/.changeset/slimy-socks-cry.md b/.changeset/slimy-socks-cry.md new file mode 100644 index 0000000000..f61a0d1274 --- /dev/null +++ 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 index bbced103ed..01a7fb493a 100644 --- 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>>(); +const applicabilityCache = Cache.empty< + Document, + Cache> +>(); /** * @internal */ -export function targetsOfPointerEvents( +export function applicableTargetsOfPointerEvents( document: Document, device: Device, ): Sequence { - 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 { + 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> +>(); + +/** + * @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 { + return allTargetsCache + .get(document, Cache.empty) + .get(device, () => + getElementDescendants(document, Node.fullTree).filter(isTarget(device)), + ); +} + +function isTarget(device: Device): Predicate { + 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 { + 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 index 4808d305ed..ac79a8a574 100644 --- 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({ 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 index 6f0d9d33e7..b430dabaec 100644 --- 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({ 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> >(); - /** * 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 index 344c1baf9b..22628a0904 100644 --- 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 = ( + + ); + + const target2 = ( + + ); + + 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 = ( + + ); + + const target2 = ( + + ); + + 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 elements", async (t) => { + const device = Device.standard(); + + const img = foo; + const 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 = ( + + ); + + 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 = ( + + ); + + const document = h.document([target]); + + t.deepEqual(await evaluate(R113, { document, device }), [ + inapplicable(R113), + ]); +});