From e4373855871b7b47fcf5a4a81acb967fe7aae921 Mon Sep 17 00:00:00 2001 From: Jean-Yves Moyen Date: Wed, 14 Feb 2024 09:51:55 +0100 Subject: [PATCH] Fix detection of empty titles (#1573) * Add breaking case * Flatten text content * Add isWhitespace predicate * Fix discarding of whitespace title * Use String.isWhitespace * Fix usage of hasTextContent * Add changeset * Clean up * Extract API --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- .changeset/sharp-chefs-glow.md | 5 + docs/review/api/alfa-string.api.md | 2 + packages/alfa-dom/src/node.ts | 11 ++- packages/alfa-rules/src/common/predicate.ts | 1 - .../src/common/predicate/is-whitespace.ts | 4 - packages/alfa-rules/src/sia-r1/rule.ts | 5 +- packages/alfa-rules/src/sia-r14/rule.ts | 3 +- packages/alfa-rules/src/sia-r4/rule.ts | 8 +- packages/alfa-rules/src/sia-r5/rule.ts | 8 +- packages/alfa-rules/src/sia-r57/rule.ts | 8 +- packages/alfa-rules/src/sia-r62/rule.ts | 96 +++++++++---------- packages/alfa-rules/src/sia-r7/rule.ts | 11 +-- packages/alfa-rules/src/sia-r74/rule.ts | 13 +-- packages/alfa-rules/src/sia-r80/rule.ts | 8 +- packages/alfa-rules/test/sia-r1/rule.spec.tsx | 14 +++ .../alfa-rules/test/sia-r74/rule.spec.tsx | 6 ++ packages/alfa-rules/tsconfig.json | 1 - packages/alfa-string/src/string.ts | 16 +++- 18 files changed, 124 insertions(+), 96 deletions(-) create mode 100644 .changeset/sharp-chefs-glow.md delete mode 100644 packages/alfa-rules/src/common/predicate/is-whitespace.ts diff --git a/.changeset/sharp-chefs-glow.md b/.changeset/sharp-chefs-glow.md new file mode 100644 index 0000000000..bd0d469744 --- /dev/null +++ b/.changeset/sharp-chefs-glow.md @@ -0,0 +1,5 @@ +--- +"@siteimprove/alfa-rules": patch +--- + +**Fixed:** A regression on R1 for `` with whitespace but not empty content has been fixed (was introduced in v0.73.0). diff --git a/docs/review/api/alfa-string.api.md b/docs/review/api/alfa-string.api.md index 6a0e179a1f..df00ef2498 100644 --- a/docs/review/api/alfa-string.api.md +++ b/docs/review/api/alfa-string.api.md @@ -9,7 +9,9 @@ type String_2 = globalThis.String; // @public (undocumented) namespace String_2 { + function flatten(input: string): string; function indent(input: string): string; + function isWhitespace(input: string, allowEmpty?: boolean): boolean; function normalize(input: string, toLowerCase?: boolean): string; } export { String_2 as String } diff --git a/packages/alfa-dom/src/node.ts b/packages/alfa-dom/src/node.ts index 9c85730e97..91986abd9f 100644 --- a/packages/alfa-dom/src/node.ts +++ b/packages/alfa-dom/src/node.ts @@ -29,6 +29,7 @@ import { import * as predicate from "./node/predicate"; import * as traversal from "./node/traversal"; +import { String } from "@siteimprove/alfa-string"; /** * @public @@ -36,9 +37,9 @@ import * as traversal from "./node/traversal"; export abstract class Node<T extends string = string> extends tree.Node<Node.Traversal.Flag, T> implements - earl.Serializable<Node.EARL>, - json.Serializable<tree.Node.JSON<T>, Node.SerializationOptions>, - sarif.Serializable<sarif.Location> + earl.Serializable<Node.EARL>, + json.Serializable<tree.Node.JSON<T>, Node.SerializationOptions>, + sarif.Serializable<sarif.Location> { protected constructor( children: Array<Node>, @@ -53,7 +54,7 @@ export abstract class Node<T extends string = string> * {@link https://dom.spec.whatwg.org/#concept-descendant-text-content} */ public textContent(options: Node.Traversal = Node.Traversal.empty): string { - return this.descendants(options).filter(Text.isText).join(""); + return String.flatten(this.descendants(options).filter(Text.isText).join("")); } /** @@ -277,7 +278,7 @@ export interface Node { * @public */ export namespace Node { - export interface JSON<T extends string = string> extends tree.Node.JSON<T> {} + export interface JSON<T extends string = string> extends tree.Node.JSON<T> { } export interface SerializationOptions { device: Device; diff --git a/packages/alfa-rules/src/common/predicate.ts b/packages/alfa-rules/src/common/predicate.ts index 0819679413..0832eaafa7 100644 --- a/packages/alfa-rules/src/common/predicate.ts +++ b/packages/alfa-rules/src/common/predicate.ts @@ -1,4 +1,3 @@ export * from "./predicate/is-large-text"; export * from "./predicate/is-at-the-start"; -export * from "./predicate/is-whitespace"; export * from "./predicate/reference-same-resource"; diff --git a/packages/alfa-rules/src/common/predicate/is-whitespace.ts b/packages/alfa-rules/src/common/predicate/is-whitespace.ts deleted file mode 100644 index c0da7df092..0000000000 --- a/packages/alfa-rules/src/common/predicate/is-whitespace.ts +++ /dev/null @@ -1,4 +0,0 @@ -import { Predicate } from "@siteimprove/alfa-predicate"; - -export const isWhitespace: Predicate<string> = (string) => - string.length > 0 && string.trim().length === 0; diff --git a/packages/alfa-rules/src/sia-r1/rule.ts b/packages/alfa-rules/src/sia-r1/rule.ts index 07238c76ca..ea194fe3c1 100644 --- a/packages/alfa-rules/src/sia-r1/rule.ts +++ b/packages/alfa-rules/src/sia-r1/rule.ts @@ -8,6 +8,7 @@ import { } from "@siteimprove/alfa-dom"; import { Predicate } from "@siteimprove/alfa-predicate"; import { Err, Ok } from "@siteimprove/alfa-result"; +import { String } from "@siteimprove/alfa-string"; import { Criterion, Technique } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; @@ -18,7 +19,7 @@ import { Scope, Stability } from "../tags"; const { hasName, hasNamespace } = Element; const { hasTextContent } = Node; -const { and } = Predicate; +const { and, not } = Predicate; const { getElementDescendants } = Query; export default Rule.Atomic.of<Page, Document>({ @@ -48,7 +49,7 @@ export default Rule.Atomic.of<Page, Document>({ ), 2: expectation( - title.some(hasTextContent()), + title.some(hasTextContent(not(String.isWhitespace))), () => Outcomes.HasNonEmptyTitle, () => Outcomes.HasEmptyTitle, ), diff --git a/packages/alfa-rules/src/sia-r14/rule.ts b/packages/alfa-rules/src/sia-r14/rule.ts index 16a4c9219d..8b27589563 100644 --- a/packages/alfa-rules/src/sia-r14/rule.ts +++ b/packages/alfa-rules/src/sia-r14/rule.ts @@ -12,7 +12,6 @@ import { Page } from "@siteimprove/alfa-web"; import { expectation } from "../common/act/expectation"; -import { isWhitespace } from "../common/predicate"; import { Scope, Stability } from "../tags"; const { hasAccessibleName, hasRole, isPerceivableForAll } = DOM; @@ -96,7 +95,7 @@ function getPerceivableInnerTextFromTextNode( if ( and(not(isPerceivableForAll(device)), isRendered(device))(text) && - isWhitespace(text.data) + String.isWhitespace(text.data, false) ) { return " "; } diff --git a/packages/alfa-rules/src/sia-r4/rule.ts b/packages/alfa-rules/src/sia-r4/rule.ts index c6995329b7..75959dd2d2 100644 --- a/packages/alfa-rules/src/sia-r4/rule.ts +++ b/packages/alfa-rules/src/sia-r4/rule.ts @@ -1,19 +1,17 @@ import { Diagnostic, Rule } from "@siteimprove/alfa-act"; import { Element } from "@siteimprove/alfa-dom"; -import { Iterable } from "@siteimprove/alfa-iterable"; import { Predicate } from "@siteimprove/alfa-predicate"; import { Err, Ok } from "@siteimprove/alfa-result"; +import { String } from "@siteimprove/alfa-string"; import { Criterion, Technique } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; import { expectation } from "../common/act/expectation"; -import { isWhitespace } from "../common/predicate"; import { Scope, Stability } from "../tags"; const { hasAttribute, isDocumentElement } = Element; -const { isEmpty } = Iterable; -const { nor } = Predicate; +const { not } = Predicate; export default Rule.Atomic.of<Page, Element>({ uri: "https://alfa.siteimprove.com/rules/sia-r4", @@ -28,7 +26,7 @@ export default Rule.Atomic.of<Page, Element>({ expectations(target) { return { 1: expectation( - hasAttribute("lang", nor(isEmpty, isWhitespace))(target), + hasAttribute("lang", not(String.isWhitespace))(target), () => Outcomes.HasLanguage, () => Outcomes.HasNoLanguage, ), diff --git a/packages/alfa-rules/src/sia-r5/rule.ts b/packages/alfa-rules/src/sia-r5/rule.ts index 61ee32355e..a434d5184d 100644 --- a/packages/alfa-rules/src/sia-r5/rule.ts +++ b/packages/alfa-rules/src/sia-r5/rule.ts @@ -1,20 +1,18 @@ import { Diagnostic, Rule } from "@siteimprove/alfa-act"; import { Attribute, Element } from "@siteimprove/alfa-dom"; import { Language } from "@siteimprove/alfa-iana"; -import { Iterable } from "@siteimprove/alfa-iterable"; import { Predicate } from "@siteimprove/alfa-predicate"; import { Err, Ok } from "@siteimprove/alfa-result"; +import { String } from "@siteimprove/alfa-string"; import { Criterion, Technique } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; import { expectation } from "../common/act/expectation"; -import { isWhitespace } from "../common/predicate"; import { Scope, Stability } from "../tags"; const { hasAttribute, isDocumentElement } = Element; -const { isEmpty } = Iterable; -const { nor } = Predicate; +const { not } = Predicate; export default Rule.Atomic.of<Page, Attribute>({ uri: "https://alfa.siteimprove.com/rules/sia-r5", @@ -27,7 +25,7 @@ export default Rule.Atomic.of<Page, Attribute>({ document .children() .filter(isDocumentElement) - .filter(hasAttribute("lang", nor(isEmpty, isWhitespace))) + .filter(hasAttribute("lang", not(String.isWhitespace))) // The previous filter ensures that lang exists .map((element) => element.attribute("lang").getUnsafe()) ); diff --git a/packages/alfa-rules/src/sia-r57/rule.ts b/packages/alfa-rules/src/sia-r57/rule.ts index ebcd9c2ad7..c6d81aa235 100644 --- a/packages/alfa-rules/src/sia-r57/rule.ts +++ b/packages/alfa-rules/src/sia-r57/rule.ts @@ -1,10 +1,10 @@ import { Diagnostic, Rule } from "@siteimprove/alfa-act"; import { DOM, Node } from "@siteimprove/alfa-aria"; import { Element, Query, Text } from "@siteimprove/alfa-dom"; -import { Iterable } from "@siteimprove/alfa-iterable"; import { Predicate } from "@siteimprove/alfa-predicate"; import { Refinement } from "@siteimprove/alfa-refinement"; import { Err, Ok } from "@siteimprove/alfa-result"; +import { String } from "@siteimprove/alfa-string"; import { Style } from "@siteimprove/alfa-style"; import { Page } from "@siteimprove/alfa-web"; @@ -12,7 +12,6 @@ import * as dom from "@siteimprove/alfa-dom"; import { expectation } from "../common/act/expectation"; -import { isWhitespace } from "../common/predicate"; import { Scope, Stability } from "../tags"; const { @@ -21,8 +20,7 @@ const { isIncludedInTheAccessibilityTree, } = DOM; const { isElement } = Element; -const { isEmpty } = Iterable; -const { nor, not, or, property, test } = Predicate; +const { not, or, property, test } = Predicate; const { and } = Refinement; const { isTabbable } = Style; const { isText } = Text; @@ -53,7 +51,7 @@ export default Rule.Atomic.of<Page, Text>({ .filter(isText) .filter( and( - property("data", nor(isEmpty, isWhitespace)), + property("data", not(String.isWhitespace)), isIncludedInTheAccessibilityTree(device), ), ); diff --git a/packages/alfa-rules/src/sia-r62/rule.ts b/packages/alfa-rules/src/sia-r62/rule.ts index 3f61696a7a..944db6d4f5 100644 --- a/packages/alfa-rules/src/sia-r62/rule.ts +++ b/packages/alfa-rules/src/sia-r62/rule.ts @@ -15,6 +15,7 @@ import { Err, Ok, Result } from "@siteimprove/alfa-result"; import { Context } from "@siteimprove/alfa-selector"; import { Sequence } from "@siteimprove/alfa-sequence"; import { Set } from "@siteimprove/alfa-set"; +import { String } from "@siteimprove/alfa-string"; import { Style } from "@siteimprove/alfa-style"; import { Criterion } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; @@ -25,7 +26,6 @@ import { Contrast } from "../common/diagnostic/contrast"; import { contrast } from "../common/expectation/contrast"; import { getForeground } from "../common/dom/get-colors"; -import { isWhitespace } from "../common/predicate"; import { Scope, Stability, Version } from "../tags"; @@ -230,25 +230,25 @@ export default Rule.Atomic.of<Page, Element>({ return hasDistinguishableStyle ? Ok.of( - ElementDistinguishable.from( - link, - device, - target, - context, - properties, - distinguishableContrast, - ), - ) + ElementDistinguishable.from( + link, + device, + target, + context, + properties, + distinguishableContrast, + ), + ) : Err.of( - ElementDistinguishable.from( - link, - device, - target, - context, - properties, - distinguishableContrast, - ), - ); + ElementDistinguishable.from( + link, + device, + target, + context, + properties, + distinguishableContrast, + ), + ); }), ) .toArray() @@ -272,8 +272,8 @@ export default Rule.Atomic.of<Page, Element>({ // If at least one link element is good, this is enough. The sorting // guarantees it is first in the array. isDefaultDistinguishable[0].isOk() && - isHoverDistinguishable[0].isOk() && - isFocusDistinguishable[0].isOk(), + isHoverDistinguishable[0].isOk() && + isFocusDistinguishable[0].isOk(), () => Outcomes.IsDistinguishable( isDefaultDistinguishable, @@ -354,7 +354,7 @@ function hasNonLinkText(device: Device): Predicate<Element> { children.some( and( isText, - and<Text>(isVisible(device), (text) => !isWhitespace(text.data)), + and<Text>(isVisible(device), (text) => !String.isWhitespace(text.data, false)), ), ) ) { @@ -387,32 +387,32 @@ namespace Distinguishable { let predicates: Array< readonly [ElementDistinguishable.Property, Predicate<Element>] > = [ - // Things like text decoration and backgrounds risk blending with the - // container element. We therefore need to check if these can be distinguished - // from what the container element might itself set. - ["background", hasDistinguishableBackground(container, device, context)], - ["contrast", hasDistinguishableContrast(container, device, context)], - ["font", hasDistinguishableFont(container, device, context)], - [ - "text-decoration", - hasDistinguishableTextDecoration(container, device, context), - ], - [ - "vertical-align", - hasDistinguishableVerticalAlign(container, device, context), - ], - // We consider the mere presence of borders, box-shadows or outlines on the element as - // distinguishable features. There's of course a risk of these blending with - // other features of the container element, such as its background, but this - // should hopefully not happen (too often) in practice. When it does, we - // risk false negatives. - ["border", hasBorder(device, context)], - [ - "box-shadow", - hasBoxShadow(device, context), //Checks for color != transparent and spread => 0 - ], - ["outline", hasOutline(device, context)], - ]; + // Things like text decoration and backgrounds risk blending with the + // container element. We therefore need to check if these can be distinguished + // from what the container element might itself set. + ["background", hasDistinguishableBackground(container, device, context)], + ["contrast", hasDistinguishableContrast(container, device, context)], + ["font", hasDistinguishableFont(container, device, context)], + [ + "text-decoration", + hasDistinguishableTextDecoration(container, device, context), + ], + [ + "vertical-align", + hasDistinguishableVerticalAlign(container, device, context), + ], + // We consider the mere presence of borders, box-shadows or outlines on the element as + // distinguishable features. There's of course a risk of these blending with + // other features of the container element, such as its background, but this + // should hopefully not happen (too often) in practice. When it does, we + // risk false negatives. + ["border", hasBorder(device, context)], + [ + "box-shadow", + hasBoxShadow(device, context), //Checks for color != transparent and spread => 0 + ], + ["outline", hasOutline(device, context)], + ]; if (context.isHovered(target)) { predicates = [ diff --git a/packages/alfa-rules/src/sia-r7/rule.ts b/packages/alfa-rules/src/sia-r7/rule.ts index 712d43de6d..62fadd53e2 100644 --- a/packages/alfa-rules/src/sia-r7/rule.ts +++ b/packages/alfa-rules/src/sia-r7/rule.ts @@ -15,16 +15,16 @@ import { Predicate } from "@siteimprove/alfa-predicate"; import { Refinement } from "@siteimprove/alfa-refinement"; import { Err, Ok } from "@siteimprove/alfa-result"; import { Sequence } from "@siteimprove/alfa-sequence"; +import { String } from "@siteimprove/alfa-string"; import { Style } from "@siteimprove/alfa-style"; import { Criterion, Technique } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; import { expectation } from "../common/act/expectation"; -import { isWhitespace } from "../common/predicate"; import { Scope, Stability } from "../tags"; -const { hasAccessibleName, isIncludedInTheAccessibilityTree } = DOM; +const { hasNonEmptyAccessibleName, isIncludedInTheAccessibilityTree } = DOM; const { hasAttribute, hasName, hasNamespace, isElement } = Element; const { isEmpty } = Iterable; const { not, or } = Predicate; @@ -52,16 +52,13 @@ export default Rule.Atomic.of<Page, Attribute>({ isText, and( or(isVisible(device), isIncludedInTheAccessibilityTree(device)), - (text: Text) => !isWhitespace(text.data), + (text: Text) => !String.isWhitespace(text.data, false), ), ); const isElementWithAccessibleName = and( isElement, - hasAccessibleName( - device, - (accessibleName) => !isWhitespace(accessibleName.value), - ), + hasNonEmptyAccessibleName(device), ); if (test(or(isVisibleText, isElementWithAccessibleName), node)) { diff --git a/packages/alfa-rules/src/sia-r74/rule.ts b/packages/alfa-rules/src/sia-r74/rule.ts index 82d97ba3f2..951e258147 100644 --- a/packages/alfa-rules/src/sia-r74/rule.ts +++ b/packages/alfa-rules/src/sia-r74/rule.ts @@ -4,6 +4,7 @@ import { Length } from "@siteimprove/alfa-css"; import { Element, Node, Query } from "@siteimprove/alfa-dom"; import { Predicate } from "@siteimprove/alfa-predicate"; import { Err, Ok } from "@siteimprove/alfa-result"; +import { String } from "@siteimprove/alfa-string"; import { Style } from "@siteimprove/alfa-style"; import { Criterion } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; @@ -13,7 +14,7 @@ import { expectation } from "../common/act/expectation"; import { Scope, Stability } from "../tags"; const { hasRole } = DOM; -const { and } = Predicate; +const { and, not } = Predicate; const { isVisible, hasCascadedStyle } = Style; const { getElementDescendants } = Query; @@ -27,7 +28,7 @@ export default Rule.Atomic.of<Page, Element>({ return getElementDescendants(document, Node.fullTree).filter( and( hasRole(device, "paragraph"), - Node.hasTextContent(), + Node.hasTextContent(not(String.isWhitespace)), isVisible(device), // If the font-size ultimately computes to size 0, the element is not // visible. @@ -46,10 +47,10 @@ export default Rule.Atomic.of<Page, Element>({ 1: expectation( // Keyword, percentage, number !Length.isLength(fontSize) || - // Calculated length - fontSize.hasCalculation() || - // Fixed length in relative units - fontSize.isRelative(), + // Calculated length + fontSize.hasCalculation() || + // Fixed length in relative units + fontSize.isRelative(), () => Outcomes.HasRelativeUnit, () => Outcomes.HasAbsoluteUnit, ), diff --git a/packages/alfa-rules/src/sia-r80/rule.ts b/packages/alfa-rules/src/sia-r80/rule.ts index ab86bb24c0..af29fb9b52 100644 --- a/packages/alfa-rules/src/sia-r80/rule.ts +++ b/packages/alfa-rules/src/sia-r80/rule.ts @@ -5,6 +5,7 @@ import { Device } from "@siteimprove/alfa-device"; import { Element, Node, Query } from "@siteimprove/alfa-dom"; import { Predicate } from "@siteimprove/alfa-predicate"; import { Err, Ok } from "@siteimprove/alfa-result"; +import { String } from "@siteimprove/alfa-string"; import { Style } from "@siteimprove/alfa-style"; import { Criterion } from "@siteimprove/alfa-wcag"; import { Page } from "@siteimprove/alfa-web"; @@ -14,7 +15,7 @@ import { expectation } from "../common/act/expectation"; import { Scope, Stability } from "../tags"; const { hasRole } = DOM; -const { and, test } = Predicate; +const { and, not, test } = Predicate; const { isVisible, hasCascadedStyle } = Style; const { getElementDescendants } = Query; @@ -28,10 +29,9 @@ export default Rule.Atomic.of<Page, Element>({ return getElementDescendants(document, Node.fullTree).filter( and( hasRole(device, "paragraph"), - (element) => - Style.from(element, device).cascaded("line-height").isSome(), - Node.hasTextContent(), + Node.hasTextContent(not(String.isWhitespace)), isVisible(device), + hasCascadedStyle("line-height", () => true, device), ), ); }, diff --git a/packages/alfa-rules/test/sia-r1/rule.spec.tsx b/packages/alfa-rules/test/sia-r1/rule.spec.tsx index 94209cca20..625b964faf 100644 --- a/packages/alfa-rules/test/sia-r1/rule.spec.tsx +++ b/packages/alfa-rules/test/sia-r1/rule.spec.tsx @@ -45,6 +45,20 @@ test("evaluate() fails a document that has an empty <title> element", async (t) ]); }); +test("evaluate() fails a document that has a whitespace only <title> element", async (t) => { + const document = h.document([ + <html> + <head> + <title> + + , + ]); + + t.deepEqual(await evaluate(R1, { document }), [ + failed(R1, document, { 1: Outcomes.HasTitle, 2: Outcomes.HasEmptyTitle }), + ]); +}); + test("evaluate() is inapplicable to a document that is not an HTML document", async (t) => { const document = h.document([]); diff --git a/packages/alfa-rules/test/sia-r74/rule.spec.tsx b/packages/alfa-rules/test/sia-r74/rule.spec.tsx index 7db95c8705..a74de3a28d 100644 --- a/packages/alfa-rules/test/sia-r74/rule.spec.tsx +++ b/packages/alfa-rules/test/sia-r74/rule.spec.tsx @@ -56,6 +56,12 @@ test("evaluate() is inapplicable to a paragraph that has no text", async (t) => t.deepEqual(await evaluate(R74, { document }), [inapplicable(R74)]); }); +test("evaluate() is inapplicable to a paragraph that has only whitespace text", async (t) => { + const document = h.document([

]); + + t.deepEqual(await evaluate(R74, { document }), [inapplicable(R74)]); +}); + test("evaluate() is inapplicable to a paragraph that isn't visible", async (t) => { const document = h.document([