Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update SIA R67 #272

Merged
merged 19 commits into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/alfa-aria/src/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ export namespace Role {
// ...and it's not a presentational role in a forbidden context...
!(
role.some(Role.isPresentational) &&
!isAllowedPresentational
!allowedPresentational
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

)
) {
// ...then we got ourselves a valid explicit role...
Expand Down
40 changes: 38 additions & 2 deletions packages/alfa-dom/src/node/attribute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,26 @@ export class Attribute extends Node {
return this._owner;
}

public hasName(name: string): boolean {
return this._name === foldCase(name, this._owner);
public hasName(predicate: Predicate<string>): boolean;

public hasName(name: string, ...rest: Array<string>): boolean;

public hasName(
nameOrPredicate: string | Predicate<string>,
...names: Array<string>
): boolean {
let predicate: Predicate<string>;

if (typeof nameOrPredicate === "function") {
predicate = nameOrPredicate;
} else {
const namesWithCases = [nameOrPredicate, ...names].map((name) =>
foldCase(name, this._owner)
);
predicate = equals(...namesWithCases);
}

return predicate(this._name);
}

/**
Expand Down Expand Up @@ -165,6 +183,24 @@ export namespace Attribute {
owner
);
}

export function hasName(predicate: Predicate<string>): Predicate<Attribute>;

export function hasName(
name: string,
...rest: Array<string>
): Predicate<Attribute>;

export function hasName(
nameOrPredicate: string | Predicate<string>,
...names: Array<string>
): Predicate<Attribute> {
if (typeof nameOrPredicate === "function") {
return attribute => attribute.hasName(nameOrPredicate);
} else {
return (attribute) => attribute.hasName(nameOrPredicate, ...names);
}
}
}

/**
Expand Down
45 changes: 35 additions & 10 deletions packages/alfa-rules/src/sia-r67/rule.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Rule, Diagnostic } from "@siteimprove/alfa-act";
import { Node, Role } from "@siteimprove/alfa-aria";
import { Element, Namespace } from "@siteimprove/alfa-dom";
import { Predicate } from "@siteimprove/alfa-predicate";
import { Err, Ok } from "@siteimprove/alfa-result";
Expand All @@ -7,10 +8,9 @@ import { Page } from "@siteimprove/alfa-web";
import { expectation } from "../common/expectation";

import { hasAccessibleName } from "../common/predicate/has-accessible-name";
import { isDecorative } from "../common/predicate/is-decorative";

const { isElement, hasName, hasNamespace } = Element;
const { and } = Predicate;
const { and, or } = Predicate;

export default Rule.Atomic.of<Page, Element>({
uri: "https://siteimprove.github.io/sanshikan/rules/sia-r67.html",
Expand All @@ -22,17 +22,30 @@ export default Rule.Atomic.of<Page, Element>({
.filter(
and(
isElement,
and(hasNamespace(Namespace.HTML), hasName("img"), isDecorative)
and(
or(
and(hasNamespace(Namespace.HTML), hasName("img")),
and(hasNamespace(Namespace.SVG), hasName("svg"))
),
isMarkedAsDecorative
)
)
);
},

expectations(target) {
return {
1: expectation(
hasAccessibleName(device)(target),
() => Outcomes.HasName,
() => Outcomes.HasNoName
Node.from(target, device).every((accNode) => {
const role = accNode.role();

return (
role.isNone() ||
Role.hasName("none", "presentation")(role.get())
);
}),
() => Outcomes.IsNotExposed,
() => Outcomes.IsExposed
),
};
},
Expand All @@ -41,15 +54,27 @@ export default Rule.Atomic.of<Page, Element>({
});

export namespace Outcomes {
export const HasNoName = Ok.of(
export const IsNotExposed = Ok.of(
Diagnostic.of(
`The image is marked as decorative and does not have an accessible name`
`The element is marked as decorative and is not exposed`
)
);

export const HasName = Err.of(
export const IsExposed = Err.of(
Diagnostic.of(
`The image is marked as decorative but has an accessible name`
`The element is marked as decorative but is exposed`
)
);
}

/**
* Check if an element is marked as decorative by looking at its role but without conflict resolution.
* If the result is "none" or "presentation", then the element is marked as decorative.
*/
function isMarkedAsDecorative(element: Element): boolean {
return (
Role.from(element, { allowPresentational: true })
// Element is marked as decorative if at least one browser thinks so.
.some((r) => r.some(Role.hasName("none", "presentation")))
);
}
120 changes: 120 additions & 0 deletions packages/alfa-rules/test/sia-r67/rule.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import {Node} from "@siteimprove/alfa-aria";
import {getName} from "@siteimprove/alfa-aria/src/get-name";
import { Device } from "@siteimprove/alfa-device";
import { jsx } from "@siteimprove/alfa-dom/jsx";
import { Option } from "@siteimprove/alfa-option";
import { Predicate } from "@siteimprove/alfa-predicate";
import { test } from "@siteimprove/alfa-test";

import { Document, Element } from "@siteimprove/alfa-dom";

import R67, { Outcomes } from "../../src/sia-r67/rule";

import { evaluate } from "../common/evaluate";
import { passed, failed, inapplicable } from "../common/outcome";

const { and } = Predicate;
const { hasId } = Element;

const device = Device.standard();

function getElementById(document: Document): (id: string) => Element {
return (id) =>
document
.descendants()
.find(and(Element.isElement, hasId(id)))
.get();
}

test("evaluate() passes on elements marked as decorative and not exposed", async (t) => {
const document = Document.of((self) => [
Element.fromElement(
<html>
<img id="empty-alt" src="foo.jpg" alt="" />
<img id="role-none" src="foo.jpg" role="none" />
<img id="role-presentation" src="foo.jpg" role="presentation" />
<svg id="svg" role="none">
<circle cx="50" cy="50" r="40" fill="yellow"></circle>
</svg>
<img id="aria-hidden" src="foo.jpg" role="none" aria-hidden="true" />
<div aria-hidden="true"><img id="aria-hidden-inherit" src="foo.jpg" role="none" /></div>
</html>,
Option.of(self)
),
]);
const getById = getElementById(document);
const emptyAlt = getById("empty-alt");
const roleNone = getById("role-none");
const rolePresentation = getById("role-presentation");
const svg = getById("svg");
Comment on lines +46 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done using destructuring and without additional helpers and need for IDs:

const [img, video, object, svg] = document
  .first()
  .get()
  .children()
  .filter(Element.isElement);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but this is less resilient to changes in the HTML.
Additionally, ids are commenting (in the HTML) what this target is testing, and can be handy for debugging messages from inside the rule itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's reasonable 👍 What we need then is a unified approach to this rather than ad-hoc helpers scattered throughout rule spec files. Test code is most annoying code to maintain so it needs to be super maintainable 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I totally agree. If we decide that IDs + getter is the way we wanna go, we should of course make the helper common and update existing tests to use it…
I've also been considering writing a wrapper for Document.of+Element.fromElement that we seem to use pretty often in tests…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#281 should hopefully help on the last point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it gets a bit better. Still need to

Document.fromDocument(
  h.document([
    <html>
      <head>
        <title>Hello world</title>
      </head>
    </html>
  ])
);

instead of

documentFromHtml(<html>
      <head>
        <title>Hello world</title>
      </head>
    </html>);

const ariaHidden = getById("aria-hidden");
const ariaHiddenInherit = getById("aria-hidden-inherit");

t.deepEqual(await evaluate(R67, { device, document }), [
passed(R67, emptyAlt, { 1: Outcomes.IsNotExposed }),
passed(R67, roleNone, { 1: Outcomes.IsNotExposed }),
passed(R67, rolePresentation, { 1: Outcomes.IsNotExposed }),
passed(R67, svg, { 1: Outcomes.IsNotExposed }),
passed(R67, ariaHidden, { 1: Outcomes.IsNotExposed }),
passed(R67, ariaHiddenInherit, { 1: Outcomes.IsNotExposed }),
]);
});

test("evaluate() fails on elements marked as decorative but exposed", async (t) => {
const document = Document.of((self) => [
Element.fromElement(
<html>
<span id="label">Foo</span>
<img id="empty-alt-aria-label" src="foo.jpg" alt="" aria-label="Foo" />
<img
id="role-none-aria-labelledby"
src="foo.jpg"
role="none"
aria-labelledby="label"
/>
</html>,
Option.of(self)
),
]);
const getById = getElementById(document);
const emptyAltAriaLabel = getById("empty-alt-aria-label");
const roleNoneAriaLabelledby = getById("role-none-aria-labelledby");

t.deepEqual(await evaluate(R67, { device, document }), [
failed(R67, emptyAltAriaLabel, { 1: Outcomes.IsExposed }),
failed(R67, roleNoneAriaLabelledby, { 1: Outcomes.IsExposed }),
]);
});

test("evaluate() is inapplicable on non-img/svg elements", async (t) => {
const document = Document.of((self) => [
Element.fromElement(
<html>
<math role="none"></math>
<span role="none"></span>
<iframe role="presentation"></iframe>
</html>,
Option.of(self)
),
]);

t.deepEqual(await evaluate(R67, { device, document }), [inapplicable(R67)]);
});

test("evaluate() is inapplicabale on elements which are not marked as decorative", async (t) => {
const document = Document.of((self) => [
Element.fromElement(
<html>
<img src="foo.jpg" alt="foo" />
<img src="foo.jpg" />
<img src="foo.jpg" />
<svg>
<circle cx="50" cy="50" r="40" fill="yellow"></circle>
</svg>
</html>,
Option.of(self)
),
]);

t.deepEqual(await evaluate(R67, { device, document }), [inapplicable(R67)]);
});
1 change: 1 addition & 0 deletions packages/alfa-rules/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@
"test/sia-r57/rule.spec.tsx",
"test/sia-r64/rule.spec.tsx",
"test/sia-r61/rule.spec.tsx",
"test/sia-r67/rule.spec.tsx",
"test/sia-r68/rule.spec.tsx",
"test/sia-r69/rule.spec.tsx",
"test/sia-r71/rule.spec.tsx",
Expand Down