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

fix(aria-roles): correct abstract roles (types) for aria-roles #4421

Merged
merged 14 commits into from May 2, 2024
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions doc/standards-object.md
Expand Up @@ -67,9 +67,11 @@ The [`ariaRoles`](../lib/standards/aria-roles.js) object defines valid ARIA role

- `type` - string(required). [The role type](https://www.w3.org/TR/wai-aria-1.1/#roles_categorization). Valid types are:
- `abstract`
- `widget`
- `structure`
- `composite`
- `landmark`
- `structure`
- `widget`
- `window`
- `requiredContext` - array(optional). List of required parent roles.
- `requiredOwned` - array(optional). List of required owned roles.
- `requiredAttrs` - array(optional). List of required attributes.
Expand Down
7 changes: 6 additions & 1 deletion lib/checks/aria/valid-scrollable-semantics-evaluate.js
Expand Up @@ -16,16 +16,21 @@ const VALID_TAG_NAMES_FOR_SCROLLABLE_REGIONS = {
* appropriate for scrollable elements found in the focus order.
*/
const VALID_ROLES_FOR_SCROLLABLE_REGIONS = {
alert: true,
alertdialog: true,
application: true,
article: true,
banner: false,
complementary: true,
contentinfo: true,
dialog: true,
form: true,
log: true,
main: true,
navigation: true,
region: true,
search: false
search: false,
status: true
};

/**
Expand Down
17 changes: 10 additions & 7 deletions lib/standards/aria-roles.js
@@ -1,4 +1,5 @@
// Source: https://www.w3.org/TR/wai-aria-1.1/#roles
// Source for abstract roles (types): https://www.w3.org/TR/wai-aria/#abstract_roles

/* easiest way to see allowed roles is to filter out the global ones
from the list of inherited states and properties. The dpub spec
Expand All @@ -17,13 +18,13 @@
*/
const ariaRoles = {
alert: {
type: 'widget',
type: 'structure',
Copy link
Contributor

Choose a reason for hiding this comment

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

This creates a problem for focus-order-semantics. With this change, the following is going to start failing. I don't think we should want that.

<div role="alert" tabindex="0"> Hello world </div>

This is what we talked about on Thursday. We'll need some way to exempt these roles that are no longer considered as widgets in the focus-order-semantics rule. I think the best way to do that is just to add them to the VALID_ROLES_FOR_SCROLLABLE_REGIONS object in valid-scrollable-semantics-evaluate.

// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded'],
superclassRole: ['section']
},
alertdialog: {
type: 'widget',
type: 'window',
straker marked this conversation as resolved.
Show resolved Hide resolved
// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded', 'aria-modal'],
superclassRole: ['alert', 'dialog'],
Expand Down Expand Up @@ -119,6 +120,7 @@ const ariaRoles = {
nameFromContent: true
},
combobox: {
// Note: spec difference
type: 'widget',
requiredAttrs: ['aria-expanded', 'aria-controls'],
allowedAttrs: [
Expand Down Expand Up @@ -169,7 +171,7 @@ const ariaRoles = {
prohibitedAttrs: ['aria-label', 'aria-labelledby']
},
dialog: {
type: 'widget',
type: 'window',
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably structure again as not a landmark

Suggested change
type: 'window',
type: 'structure',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we said window is fine for alertdialog I assume it's fine here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Ava. Since this is window in the spec, lets have it consistent.

// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded', 'aria-modal'],
superclassRole: ['window'],
Expand Down Expand Up @@ -301,6 +303,7 @@ const ariaRoles = {
superclassRole: ['section']
},
listbox: {
// Note: spec difference
type: 'widget',
requiredOwned: ['group', 'option'],
allowedAttrs: [
Expand Down Expand Up @@ -328,7 +331,7 @@ const ariaRoles = {
nameFromContent: true
},
log: {
type: 'widget',
type: 'structure',
// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded'],
superclassRole: ['section']
Expand All @@ -340,7 +343,7 @@ const ariaRoles = {
superclassRole: ['landmark']
},
marquee: {
type: 'widget',
type: 'structure',
Copy link
Contributor

Choose a reason for hiding this comment

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

When you're adding roles to valid-scrollable-semantics, I think you can leave this one out. role=marquee doesn't seem like it should be focusable.

// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded'],
superclassRole: ['section']
Expand Down Expand Up @@ -696,7 +699,7 @@ const ariaRoles = {
accessibleNameRequired: true
},
status: {
type: 'widget',
type: 'structure',
// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded'],
superclassRole: ['section']
Expand Down Expand Up @@ -816,7 +819,7 @@ const ariaRoles = {
superclassRole: ['section']
},
timer: {
type: 'widget',
type: 'structure',
Copy link
Contributor

Choose a reason for hiding this comment

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

When you're adding roles to valid-scrollable-semantics, I think you can leave this one out. role=timer doesn't seem like it should be focusable.

// Spec difference: Aria-expanded removed in 1.2
allowedAttrs: ['aria-expanded'],
superclassRole: ['status']
Expand Down
24 changes: 24 additions & 0 deletions test/checks/aria/valid-scrollable-semantics.js
Expand Up @@ -106,6 +106,18 @@ describe('valid-scrollable-semantics', function () {
);
});

it('should return true for role=alertdialog', function () {
var node = document.createElement('div');
node.setAttribute('role', 'alertdialog');
fixture.appendChild(node);
flatTreeSetup(fixture);
assert.isTrue(
axe.testUtils
.getCheckEvaluate('valid-scrollable-semantics')
.call(checkContext, node)
);
});

it('should return true for role=article', function () {
var node = document.createElement('div');
node.setAttribute('role', 'article');
Expand All @@ -118,6 +130,18 @@ describe('valid-scrollable-semantics', function () {
);
});

it('should return true for role=dialog', function () {
var node = document.createElement('div');
node.setAttribute('role', 'dialog');
fixture.appendChild(node);
flatTreeSetup(fixture);
assert.isTrue(
axe.testUtils
.getCheckEvaluate('valid-scrollable-semantics')
.call(checkContext, node)
);
});

it('should return true for nav elements', function () {
var node = document.createElement('nav');
fixture.appendChild(node);
Expand Down
Expand Up @@ -19,6 +19,8 @@ <h4>Invalid landmark roles for scrollable containers</h4>
<div>
<div id="violation3" role="banner" tabindex="0"></div>
<div id="violation4" role="search" tabindex="0"></div>
<div id="violation5" role="marquee" tabindex="0"></div>
<div id="violation6" role="timer" tabindex="0"></div>
</div>
<h4>Valid landmark roles for scrollable containers</h4>
<div>
Expand All @@ -31,6 +33,14 @@ <h4>Valid landmark roles for scrollable containers</h4>
<div id="pass8" role="application" tabindex="0"></div>
<div id="pass9" role="tooltip" tabindex="0"></div>
<div id="pass10" role="article" tabindex="0"></div>
<div id="pass11" role="alert" tabindex="0"></div>
<div id="pass12" role="log" tabindex="0"></div>
<div id="pass13" role="status" tabindex="0"></div>
</div>
<h4>Valid window roles for scrollable containers</h4>
<div>
<div id="pass14" role="alertdialog" tabindex="0"></div>
<div id="pass15" role="dialog" tabindex="0"></div>
</div>
<h4>
Valid scrollable HTML tags for scrollable regions, not selected by this
Expand Down
Expand Up @@ -11,12 +11,19 @@
["#pass7"],
["#pass8"],
["#pass9"],
["#pass10"]
["#pass10"],
["#pass11"],
["#pass12"],
["#pass13"],
["#pass14"],
["#pass15"]
],
"violations": [
["#violation1"],
["#violation2"],
["#violation3"],
["#violation4"]
["#violation4"],
["#violation5"],
["#violation6"]
]
}
35 changes: 35 additions & 0 deletions test/integration/rules/target-size/target-size.html
Expand Up @@ -28,6 +28,41 @@
</textarea>
</p>

<div>
<button id="pass9">x</button>
<div role="alertdialog" tabindex="0" id="ignore1">Cookies</div>
</div>

<div>
<button id="pass10">x</button>
<div role="dialog" tabindex="0" id="ignore2">Cookies</div>
</div>

<div>
<button id="pass11">x</button>
<div role="alert" tabindex="0" id="ignore3">Cookies</div>
</div>

<div>
<button id="pass12">x</button>
<div role="log" tabindex="0" id="ignore4">Log</div>
</div>

<div>
<button id="pass13">x</button>
<div role="marquee" tabindex="0" id="ignore4">Marquee</div>
</div>

<div>
<button id="pass14">x</button>
<div role="status" tabindex="0" id="ignore5">Status</div>
</div>

<div>
<button id="pass15">x</button>
<div role="timer" tabindex="0" id="ignore6">Timer</div>
</div>

<!-- Nested controls-->
<p>
<a
Expand Down
7 changes: 7 additions & 0 deletions test/integration/rules/target-size/target-size.json
Expand Up @@ -20,6 +20,13 @@
["#pass6"],
["#pass7"],
["#pass8"],
["#pass9"],
["#pass10"],
["#pass11"],
["#pass12"],
["#pass13"],
["#pass14"],
["#pass15"],
["#pass-adjacent"],
["#pass-fixed"]
],
Expand Down