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

feat: add new ARIA roles #3138

Merged
merged 6 commits into from
Sep 7, 2021
Merged

feat: add new ARIA roles #3138

merged 6 commits into from
Sep 7, 2021

Conversation

Zidious
Copy link
Contributor

@Zidious Zidious commented Aug 27, 2021

ARIA 1.3 Annotations Added new roles: mark, suggestion, and comment.

Closes issue: #3122

@Zidious Zidious requested a review from a team as a code owner August 27, 2021 13:44
@CLAassistant
Copy link

CLAassistant commented Aug 27, 2021

CLA assistant check
All committers have signed the CLA.

@Zidious Zidious changed the title feat(addariaroles) - Add new ARIA roles feat - Add new ARIA roles Aug 27, 2021
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.

Great start! Since some of these roles have attributes, owned elements, or prohibited attributes, we'll need to add a few more tests to make sure they are all covered

lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
@Zidious Zidious requested a review from straker August 30, 2021 18:27
@Zidious Zidious dismissed straker’s stale review August 31, 2021 12:29

changed added

<span role="insertion">option</span>
</div>

<div role="suggestion" id="fail8">
Copy link
Contributor

Choose a reason for hiding this comment

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

Good start. So with our integration tests we want to be really thorough and test all possible states, so we'll want 3 more tests added. One for suggestion without any children, and one for suggestion with just an insertion child, and one for suggestion with just a deletion child.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to test for one child due to requiredOwned (as discussed). However ARIA Suggestion Docs say that both need to be present, playground throws no errors if using one child

test/integration/rules/aria-allowed-attr/passes.html Outdated Show resolved Hide resolved
test/integration/rules/aria-allowed-attr/passes.json Outdated Show resolved Hide resolved
@Zidious Zidious changed the title feat - Add new ARIA roles feat: add new ARIA roles Sep 1, 2021
@Zidious Zidious requested a review from straker September 1, 2021 15:29
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.

LGTM. Will wait for @WilcoFiers final approve.

@WilcoFiers the main question we have is how to handle the role=suggestion element requiring that it owns both a role=insertion and role=deletion element. Currently aria-required-children will pass if at least one role is owned, but not enforce all roles.

@WilcoFiers
Copy link
Contributor

@straker Suggestion doesn't require both roles. MDN is wrong about that:

Authors MUST ensure that a suggestion contains either one insertion child or one deletion child or ensure that it contains two children where one is an insertion and the other is a deletion. Authors MUST ensure a suggestion does not contain any other children.
https://w3c.github.io/aria/#suggestion

Either way, it's so early days on this spec, I don't think we should be too harsh on testing just yet.

lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
lib/standards/aria-roles.js Outdated Show resolved Hide resolved
@Zidious Zidious merged commit 61be7e5 into develop Sep 7, 2021
@Zidious Zidious deleted the addariaroles branch September 7, 2021 18:54
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.

4 participants