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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
69f778b
fix(aria-roles): correct abstract roles (types) for aria-roles
gaiety-deque Apr 22, 2024
ffe37f6
fix(aria-roles): table with role application is structure
gaiety-deque Apr 22, 2024
aa1aa20
fix(aria-roles): inline widgets consider composite type
gaiety-deque Apr 22, 2024
fd59e8d
Merge branch 'develop' into aria-role-abstract-corrections
gaiety-deque Apr 23, 2024
2d185f6
fix(aria-roles): address pr comments
gaiety-deque Apr 25, 2024
417e6b1
fix(aria-roles): reverted test change as well
gaiety-deque Apr 25, 2024
facd65f
fix(aria-roles): exception for focus-order-semantics
gaiety-deque Apr 25, 2024
bfa95ae
fix(aria-roles): updated locales
gaiety-deque Apr 25, 2024
a07c488
Revert "fix(aria-roles): exception for focus-order-semantics"
gaiety-deque Apr 29, 2024
f11c03f
fix(aria-roles): support window role in valid-scrollable-semantics
gaiety-deque Apr 29, 2024
d856316
fix(aria-roles): window roles and marque/timer expectations in focus-…
gaiety-deque Apr 29, 2024
dc966d3
Revert "fix(aria-roles): updated locales"
gaiety-deque Apr 29, 2024
9b15ae0
fix(aria-roles): log and status allowed in focus-order-semantics
gaiety-deque Apr 29, 2024
5594d8f
fix(aria-roles): target-size tests consider changed aria roles
gaiety-deque Apr 29, 2024
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
6 changes: 4 additions & 2 deletions doc/standards-object.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
10 changes: 10 additions & 0 deletions test/integration/rules/focus-order-semantics/focus-order-semantics.html
View file
Open in desktop
Original file line number Diff line number Diff line change
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
11 changes: 9 additions & 2 deletions test/integration/rules/focus-order-semantics/focus-order-semantics.json
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@
["#pass6"],
["#pass7"],
["#pass8"],
["#pass9"],
["#pass10"],
["#pass11"],
["#pass12"],
["#pass13"],
["#pass14"],
["#pass15"],
["#pass-adjacent"],
["#pass-fixed"]
],
Expand Down
Loading