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

gaiety-deque · 2024-04-22T16:11:35Z

Too many aria roles were set to widget or otherwise were incorrect, added a comment source for where I got my information for which abstract roles were the type of other aria roles

Fix: #4371

Too many aria roles were set to widget or otherwise were incorrect, added a comment source for where I got my information for which abstract roles were the type of which roles Refs: #4371

Previously there was a test to confirm a table with role of application was a data table, but the spec indicates that has an anstract role of structure meaning this should be a structure table instead This commit is just a test correction Refs: #4421

widget not inline rule corrected isWidgetType check to consider composite widget types

gaiety-deque · 2024-04-22T19:23:14Z

@WilcoFiers unsure how to test this PR's changes against the comment you left in the referenced issue, I don't see a cookie notice on the horse insurance website. Any guidance is appreciated.

@straker I feel reasonably good about this change but I'm a bit unsure if it fully addresses the original issue as it seemed fairly open ended

straker

Thanks for taking this on. Addressing the change should mostly be covered by updating the roles, specifically of tabpanel.

https://www.w3.org/WAI/ARIA/1.2/class-diagram/rdf_model.svg is an interesting resource. I've not seen it before. It's also hard to follow sometimes what line belongs to what, so I hope I parsed it correctly.

lib/standards/aria-roles.js

straker · 2024-04-22T21:47:43Z

lib/standards/aria-roles.js

@@ -169,7 +170,7 @@ const ariaRoles = {
    prohibitedAttrs: ['aria-label', 'aria-labelledby']
  },
  dialog: {
-    type: 'widget',
+    type: 'window',


Probably structure again as not a landmark

Suggested change

type: 'window',

type: 'structure',

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

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

lib/standards/aria-roles.js

straker · 2024-04-23T16:26:10Z

lib/rules/widget-not-inline-matches.js

@@ -18,7 +18,8 @@ const matchesFns = [
 ];

 function isWidgetType(vNode) {
-  return getRoleType(vNode) === 'widget';
+  const roleType = getRoleType(vNode);
+  return roleType === 'widget' || roleType === 'composite';


Undo this change since we'll leave listbox and combobox as widgets

Undid this change 2d185f6

lib/standards/aria-roles.js

straker · 2024-04-23T16:28:17Z

Will also need to add exception to focus-order-semantics to allow alert and alertdialog roles (add unit & integration tests to match)

- learned sometimes we intentionally spec change - added `window` as a supported type, and `composite` which was missing from the readme - undid my change to `isWidgetType` Refs: #4371

test change was only for data tables, but we didn't want to keep that type change Refs: #4371

add `window` to has-widget-role, renamed Refs: #4371

better pass/fail english message, removed outdated translations Refs: #4371

gaiety-deque · 2024-04-25T19:42:45Z

Will also need to add exception to focus-order-semantics to allow alert and alertdialog roles (add unit & integration tests to match)

@straker added an exception to focus-order-semantics, renamed check and added a window test describe for both allowed role types. Refactored the test check as well, I'm all about verbose test files with repetition but 500+ lines had me scrolling all over the place losing track so I simplified down the checks to a static easier to comprehend list facd65f (and bfa95ae)

WilcoFiers

Can you add integration tests for:

Show that target-size no longer applies to some of the things that are no longer widgets. Most importantly alertdialog
Show that target-size no longer treats elements with those roles as neighbor targets
Show which roles are still allowed to be focusable under valid-scrollable-semantics, and which are not

WilcoFiers · 2024-04-29T09:35:11Z

lib/checks/aria/has-widget-role.json

@@ -1,12 +0,0 @@
-{
-  "id": "has-widget-role",
-  "evaluate": "has-widget-role-evaluate",


Removing a check is a breaking change. Apologies, I should have mentioned that when we talked about it. I don't think we should be updating this check at all. It would be better to add the roles that are no longer allowed under has-widget-role as an allowed role in valid-scrollable-semantics.

WilcoFiers · 2024-04-29T09:37:27Z

lib/checks/aria/has-widget-or-window-role-evaluate.js

+ * @memberof checks
+ * @return {Boolean} True if the element uses a `widget`, `composite`, or `window` abstract role (type). False otherwise.
+ */
+// # TODO: change to abstract role for widget and window


I'm not sure what you mean by this. Is this supposed to be here?

Not intentional, reverted a07c488

WilcoFiers · 2024-04-29T09:49:33Z

lib/standards/aria-roles.js

@@ -169,7 +170,7 @@ const ariaRoles = {
    prohibitedAttrs: ['aria-label', 'aria-labelledby']
  },
  dialog: {
-    type: 'widget',
+    type: 'window',


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

WilcoFiers · 2024-04-29T10:20:12Z

lib/standards/aria-roles.js

@@ -17,13 +18,13 @@
 */
 const ariaRoles = {
  alert: {
-    type: 'widget',
+    type: 'structure',


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.

WilcoFiers · 2024-04-29T10:27:15Z

lib/standards/aria-roles.js

@@ -816,7 +819,7 @@ const ariaRoles = {
    superclassRole: ['section']
  },
  timer: {
-    type: 'widget',
+    type: 'structure',


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.

WilcoFiers · 2024-04-29T10:27:32Z

lib/standards/aria-roles.js

@@ -340,7 +343,7 @@ const ariaRoles = {
    superclassRole: ['landmark']
  },
  marquee: {
-    type: 'widget',
+    type: 'structure',


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.

WilcoFiers · 2024-04-29T10:30:19Z

test/checks/aria/has-widget-or-window-role.js

+    axe.testUtils
+      .getCheckEvaluate('has-widget-or-window-role')
+      .call(checkContext, currentNode);
+  const roles = {


This feels a little brittle. Generally what we do is we have the unit tests just check the code works. One or two examples of each role type is enough. And then we have fairly extensive integration tests. Those are a little more telling.

I'll be reverting this change since the desire is to not change this check. EDIT: Reverted a07c488

Though I believe this change was not brittle. This was a refactor from ~500 lines of tests to ~100 that checked the same things, but it was difficult to parse by a person before. I found myself getting quite lost in the code previously. The underlying test code was the same with this change.

Addresses some feedback from Wilco in the PR that this isn't the change we want This reverts commit facd65f.

reflected in focus-order-semantics

…order-semantics addressing more pr feedback

This reverts commit bfa95ae.

extends prior work by accounting for all standards aria roles changes

last feedback yay

gaiety-deque · 2024-04-29T17:51:04Z

@WilcoFiers all feedback addressed :) let me know if there's anything else amiss

responded

@WilcoFiers

### [4.9.1](v4.9.0...v4.9.1) (2024-05-06) ### Bug Fixes - Prevent errors when loading axe in a page with prototype.js - **aria-allowed-attr:** allow meter role allowed aria-\* attributes on meter element ([#4435](#4435)) ([7ac6392](7ac6392)) - **aria-allowed-role:** add gridcell, separator, slider and treeitem to allowed roles of button element ([#4398](#4398)) ([4788bf8](4788bf8)) - **aria-roles:** correct abstract roles (types) for aria-roles([#4421](#4421)) - **aria-valid-attr-value:** aria-controls & aria-haspopup incomplete ([#4418](#4418)) - fix building axe-core translation files with region locales ([#4396](#4396)) ([5c318f3](5c318f3)), closes [#4388](#4388) - **invalidrole:** allow upper and mixed case role names ([#4358](#4358)) ([105016c](105016c)), closes [#2695](#2695) - **isVisibleOnScreen:** account for position: absolute elements inside overflow container ([#4405](#4405)) ([2940f6e](2940f6e)), closes [#4016](#4016) - **label-content-name-mismatch:** better dismiss and wysiwyg symbolic text characters ([#4402](#4402)) - **region:** Decorative images ignored by region rule ([#4412](#4412)) - **target-size:** ignore descendant elements in shadow dom ([#4410](#4410)) ([6091367](6091367)) - **target-size:** pass for element that has nearby elements that are obscured ([#4422](#4422)) ([3a90bb7](3a90bb7)), closes [#4387](#4387) This PR was opened by a robot 🤖 🎉 (And updated by @WilcoFiers )

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

69f778b

Too many aria roles were set to widget or otherwise were incorrect, added a comment source for where I got my information for which abstract roles were the type of which roles Refs: #4371

gaiety-deque changed the title ~~WIP fix(aria-roles): correct abstract roles (types) for aria-roles~~ fix(aria-roles): correct abstract roles (types) for aria-roles Apr 22, 2024

gaiety-deque added 2 commits April 22, 2024 12:48

fix(aria-roles): inline widgets consider composite type

aa1aa20

widget not inline rule corrected isWidgetType check to consider composite widget types

straker previously requested changes Apr 22, 2024

View reviewed changes

straker reviewed Apr 23, 2024

View reviewed changes

gaiety-deque added 5 commits April 23, 2024 16:58

Merge branch 'develop' into aria-role-abstract-corrections

fd59e8d

fix(aria-roles): address pr comments

2d185f6

- learned sometimes we intentionally spec change - added `window` as a supported type, and `composite` which was missing from the readme - undid my change to `isWidgetType` Refs: #4371

fix(aria-roles): reverted test change as well

417e6b1

test change was only for data tables, but we didn't want to keep that type change Refs: #4371

fix(aria-roles): exception for focus-order-semantics

facd65f

add `window` to has-widget-role, renamed Refs: #4371

fix(aria-roles): updated locales

bfa95ae

better pass/fail english message, removed outdated translations Refs: #4371

gaiety-deque marked this pull request as ready for review April 25, 2024 19:42

gaiety-deque requested a review from a team as a code owner April 25, 2024 19:42

gaiety-deque requested a review from straker April 25, 2024 19:43

WilcoFiers requested changes Apr 29, 2024

View reviewed changes

gaiety-deque added 6 commits April 29, 2024 08:25

Revert "fix(aria-roles): exception for focus-order-semantics"

a07c488

Addresses some feedback from Wilco in the PR that this isn't the change we want This reverts commit facd65f.

fix(aria-roles): support window role in valid-scrollable-semantics

f11c03f

reflected in focus-order-semantics

fix(aria-roles): window roles and marque/timer expectations in focus-…

d856316

…order-semantics addressing more pr feedback

Revert "fix(aria-roles): updated locales"

dc966d3

This reverts commit bfa95ae.

fix(aria-roles): log and status allowed in focus-order-semantics

9b15ae0

extends prior work by accounting for all standards aria roles changes

fix(aria-roles): target-size tests consider changed aria roles

5594d8f

last feedback yay

gaiety-deque requested a review from WilcoFiers April 29, 2024 17:50

WilcoFiers approved these changes May 1, 2024

View reviewed changes

gaiety-deque merged commit 19bde94 into develop May 2, 2024
21 checks passed

gaiety-deque deleted the aria-role-abstract-corrections branch May 2, 2024 13:06

attest-team-ci mentioned this pull request May 6, 2024

chore(release): v4.9.1 #4446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gaiety-deque commented Apr 22, 2024 •

edited

gaiety-deque commented Apr 22, 2024

straker left a comment •

edited

straker Apr 22, 2024

gaiety-deque Apr 23, 2024

WilcoFiers Apr 29, 2024

straker Apr 23, 2024

gaiety-deque Apr 25, 2024

straker commented Apr 23, 2024

gaiety-deque commented Apr 25, 2024

WilcoFiers left a comment •

edited

WilcoFiers Apr 29, 2024

WilcoFiers Apr 29, 2024

gaiety-deque Apr 29, 2024

WilcoFiers Apr 29, 2024

WilcoFiers Apr 29, 2024

WilcoFiers Apr 29, 2024

WilcoFiers Apr 29, 2024

WilcoFiers Apr 29, 2024

gaiety-deque Apr 29, 2024 •

edited

gaiety-deque commented Apr 29, 2024

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

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

Conversation

gaiety-deque commented Apr 22, 2024 • edited

gaiety-deque commented Apr 22, 2024

straker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straker commented Apr 23, 2024

gaiety-deque commented Apr 25, 2024

WilcoFiers left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaiety-deque Apr 29, 2024 • edited

Choose a reason for hiding this comment

gaiety-deque commented Apr 29, 2024

gaiety-deque commented Apr 22, 2024 •

edited

straker left a comment •

edited

WilcoFiers left a comment •

edited

gaiety-deque Apr 29, 2024 •

edited