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

Improve handling of cyclic aria-owns references #442

Merged
merged 1 commit into from
Oct 7, 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
1 change: 1 addition & 0 deletions packages/alfa-aria/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"@siteimprove/alfa-device": "^0.5.0",
"@siteimprove/alfa-dom": "^0.5.0",
"@siteimprove/alfa-equatable": "^0.5.0",
"@siteimprove/alfa-graph": "^0.5.0",
"@siteimprove/alfa-hash": "^0.5.0",
"@siteimprove/alfa-iterable": "^0.5.0",
"@siteimprove/alfa-json": "^0.5.0",
Expand Down
38 changes: 26 additions & 12 deletions packages/alfa-aria/src/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Branched } from "@siteimprove/alfa-branched";
import { Cache } from "@siteimprove/alfa-cache";
import { Browser } from "@siteimprove/alfa-compatibility";
import { Device } from "@siteimprove/alfa-device";
import { Graph } from "@siteimprove/alfa-graph";
import { Serializable } from "@siteimprove/alfa-json";
import { Lazy } from "@siteimprove/alfa-lazy";
import { Map } from "@siteimprove/alfa-map";
Expand Down Expand Up @@ -285,30 +286,43 @@ export namespace Node {
// Refine the collected `aria-owns` references, constructing a set of
// claimed elements and resolving conflicting claims as needed.
const [claimed, owned] = references.reduce(
([claimed, owned], [element, references]) => {
// Reject all element references that have already been claimed. While
// authors are not allowed to specify a given ID in more than one
// `aria-owns` attribute, it will inevitably happen that multiple
// `aria-owns` attributes reference the same ID. We deal with this on a
// first come, first serve basis and deny anything but the first claim
// to a given ID.
references = references.reject((element) => claimed.has(element));
([claimed, owned, graph], [element, references]) => {
// Reject all element references that have either already been claimed
// or would introduce a cyclic reference. While authors are not allowed
// to specify a given ID in more than one `aria-owns` attribute, it will
// inevitably happen that multiple `aria-owns` attributes reference the
// same ID. We deal with this on a first come, first serve basis and
// deny anything but the first claim to a given ID.
references = references.reject(
(reference) =>
claimed.has(reference) || graph.hasPath(reference, element)
);

// If there are no references left, this element has no explicit
// ownership.
if (references.isEmpty()) {
return [claimed, owned];
return [claimed, owned, graph];
}

// Claim the remaining references.
claimed = references.reduce(
(claimed, element) => claimed.add(element),
(claimed, reference) => claimed.add(reference),
claimed
);

return [claimed, owned.set(element, references)];
// Connect the element to each of its references to track cycles.
graph = references.reduce(
(graph, reference) => graph.connect(element, reference),
graph
);

return [claimed, owned.set(element, references), graph];
},
[Set.empty<dom.Node>(), Map.empty<dom.Element, Sequence<dom.Node>>()]
[
Set.empty<dom.Element>(),
Map.empty<dom.Element, Sequence<dom.Element>>(),
Graph.empty<dom.Element>(),
]
);

build(root, device, claimed, owned);
Expand Down
24 changes: 18 additions & 6 deletions packages/alfa-aria/test/node.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,25 @@ test(`.from() correctly handles circular aria-owns references between siblings`,
{bar}
</div>;

// We should use the `Graph` class for building up a graph of references and
// reject any reference that would cause a cycle. Until then, cyclic references
// will cause all nodes participating in the cycle to become inert.

t.deepEqual(Node.from(foo, device).toJSON(), [[Inert.of(foo).toJSON(), []]]);
t.deepEqual(Node.from(foo, device).toJSON(), [
[
Element.of(
foo,
None,
None,
[Attribute.of("aria-owns", "bar")],
[Element.of(bar, None, None, [Attribute.of("aria-owns", "foo")])]
).toJSON(),
[],
],
]);

t.deepEqual(Node.from(bar, device).toJSON(), [[Inert.of(bar).toJSON(), []]]);
t.deepEqual(Node.from(bar, device).toJSON(), [
[
Element.of(bar, None, None, [Attribute.of("aria-owns", "foo")]).toJSON(),
[],
],
]);
});

test(`.from() correctly handles circular aria-owns references between ancestors
Expand Down
3 changes: 3 additions & 0 deletions packages/alfa-aria/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
{
"path": "../alfa-equatable"
},
{
"path": "../alfa-graph"
},
{
"path": "../alfa-hash"
},
Expand Down