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): Add WAI-ARIA 1.2 roles #2544

Merged

Conversation

timmybytes
Copy link
Contributor

@timmybytes timmybytes commented Oct 7, 2020

Adds new roles from WAI-ARIA 1.2 to lib/standards/aria-roles.js

Closes issue: #2107

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security by @WilcoFiers

Adds new roles from WAI-ARIA 1.2 spec

Closes issue dequelabs#2107
@timmybytes timmybytes requested a review from a team as a code owner October 7, 2020 15:48
@timmybytes
Copy link
Contributor Author

Hey there. This is my first time working with a codebase of this size and complexity. I followed the contributing guidelines correctly (I think), only altered the single aria-roles.js file, but run into the following error:

  1 failing

  1) standards.getAriaRolesByType
       should return a list of role names by type:

      AssertionError: expected [ Array(37) ] to deeply equal [ Array(25) ]
      + expected - actual

       [
         "article"
      -  "blockquote"
      -  "caption"
         "cell"
      -  "code"
         "columnheader"
         "definition"
      -  "deletion"
         "directory"
         "document"
      -  "emphasis"
         "feed"
         "figure"
         "group"
         "heading"
         "img"
      -  "insertion"
         "list"
         "listitem"
         "math"
      -  "meter"
         "none"
         "note"
      -  "paragraph"
         "presentation"
         "row"
         "rowgroup"
         "rowheader"
         "separator"
      -  "strong"
      -  "subscript"
      -  "superscript"
         "table"
         "term"
      -  "time"
         "toolbar"
         "tooltip"
       ]
      
      at Context.<anonymous> (standards/get-aria-roles-by-type.js:15:10)

I've gone through the get-aria-roles-by-type.js file, which imports from lib/standards/index.js, but I'm at a bit of a loss as how to proceed. It seems like these new roles aren't being added in somewhere else for comparison checks, but like I said I'm a bit green with a project this complex. Could you point me in the right direction to get this fixed for you all?

@straker
Copy link
Contributor

straker commented Oct 7, 2020

Sure. The test that is failing takes the roles directly from the standards object. So the fix should just be to add all the roles (looks like you added them all as structure roles) to the list from that test and it should work.

Adds new roles from WAI-ARIA 1.2 spec to test/commons/standards/get-aria-roles-by-type.js

Closes issue dequelabs#2107
straker
straker previously requested changes Oct 7, 2020
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.

Awesome work. Just need to add 1 attribute and should be good to go after the tests are fixed.

lib/standards/aria-roles.js Show resolved Hide resolved
Adds the allowedAttrs: ['aria-valuetext'] to the meter role

Closes issue dequelabs#2107
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. But I want @WilcoFiers to look it over really quick make sure this is what he intended.

@timmybytes
Copy link
Contributor Author

Absolutely, no worries. Thanks so much for the help!

button: {
type: 'widget',
allowedAttrs: ['aria-expanded', 'aria-pressed'],
nameFromContent: true
},
caption: {
type: 'structure',
requiredContext: ['figure', 'table', 'grid']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
requiredContext: ['figure', 'table', 'grid']
requiredContext: ['figure', 'table', 'grid', 'treegrid']

Adds treegrid to caption role's requiredContext array.

Closes issue: dequelabs#2107
@straker straker added the hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/ label Oct 9, 2020
@WilcoFiers
Copy link
Contributor

@timmybytes Thank you for the pull request! Much appreciated.

@WilcoFiers WilcoFiers merged commit 635b084 into dequelabs:develop Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants