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: better unsupported attribute support for aria-roledescription #1382

Merged
merged 11 commits into from
Feb 28, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Feb 19, 2019

Expand the unsupported property of an attribute by allowing certain elements to use the attribute.

Closes: #1216

Reviewer checks

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

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @WilcoFiers

doc/aria-supported.md Show resolved Hide resolved
lib/checks/aria/unsupportedattr.js Outdated Show resolved Hide resolved
@straker straker marked this pull request as ready for review February 21, 2019 17:32
@straker straker requested a review from a team as a code owner February 21, 2019 17:32
jeeyyy
jeeyyy previously requested changes Feb 22, 2019
build/tasks/aria-supported.js Outdated Show resolved Hide resolved
build/tasks/aria-supported.js Outdated Show resolved Hide resolved
build/tasks/aria-supported.js Outdated Show resolved Hide resolved
lib/commons/aria/attributes.js Outdated Show resolved Hide resolved
jeeyyy
jeeyyy previously requested changes Feb 25, 2019
build/tasks/aria-supported.js Outdated Show resolved Hide resolved
build/tasks/aria-supported.js Outdated Show resolved Hide resolved
build/tasks/aria-supported.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy self-assigned this Feb 25, 2019
@jeeyyy jeeyyy dismissed their stale review February 25, 2019 19:14

Updated.

@jeeyyy jeeyyy requested a review from WilcoFiers February 25, 2019 19:14
@jeeyyy
Copy link
Contributor

jeeyyy commented Feb 25, 2019

@WilcoFiers
I have updated this, so on your plate to review.

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.

Please add integration tests for each of the exceptions, and a few for elements that are not an exception, such as input[type=text] and textarea.

const unsupported = attribute.unsupported;

if (typeof unsupported === 'boolean' || !unsupported.exceptions) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, rather than checking if unsupported is falsey on line 8, I think this code would be a little easier to read if you check on line 14 if unsupported is not an object, and on line 14 do return !!unsupported.

}

// validate attributes and conditions (if any) from allowedElement to given node
let out = axe.commons.matches(node, unsupported.exceptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

let out = -> const isException =

@WilcoFiers WilcoFiers merged commit 93f721e into develop Feb 28, 2019
@WilcoFiers WilcoFiers deleted the ariaRoleDescription branch February 28, 2019 08:44
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.

3 participants