Skip to content

feat: add new ARIA roles#3138

Merged
Zidious merged 6 commits intodevelopfrom
addariaroles
Sep 7, 2021
Merged

feat: add new ARIA roles#3138
Zidious merged 6 commits intodevelopfrom
addariaroles

Conversation

@Zidious
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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

@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
Copy Markdown
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
Copy Markdown
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

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

@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