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

Conversation

gaiety-deque
Copy link
Contributor

@gaiety-deque gaiety-deque commented Apr 22, 2024

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
@gaiety-deque 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
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
Copy link
Contributor Author

@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

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
@@ -169,7 +170,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.

lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
@@ -18,7 +18,8 @@ const matchesFns = [
];

function isWidgetType(vNode) {
return getRoleType(vNode) === 'widget';
const roleType = getRoleType(vNode);
return roleType === 'widget' || roleType === 'composite';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undid this change 2d185f6

lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
@straker
Copy link
Contributor

straker commented Apr 23, 2024

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 gaiety-deque marked this pull request as ready for review April 25, 2024 19:42
@gaiety-deque gaiety-deque requested a review from a team as a code owner April 25, 2024 19:42
@gaiety-deque
Copy link
Contributor Author

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)

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Can you add integration tests for:

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

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

Choose a reason for hiding this comment

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

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.

* @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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not intentional, reverted a07c488

@@ -169,7 +170,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.

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

@@ -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.

@@ -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.

@@ -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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gaiety-deque gaiety-deque Apr 29, 2024

Choose a reason for hiding this comment

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

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.
extends prior work by accounting for all standards aria roles changes
@gaiety-deque
Copy link
Contributor Author

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

@gaiety-deque gaiety-deque merged commit 19bde94 into develop May 2, 2024
21 checks passed
@gaiety-deque gaiety-deque deleted the aria-role-abstract-corrections branch May 2, 2024 13:06
WilcoFiers added a commit that referenced this pull request May 6, 2024
###
[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
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aria-roles standards object contains many roles that are classified as widgets but are not
3 participants