feat: Add allowEmpty option for aria-valid-attr-value #1154
Conversation
WilcoFiers
changed the title
| }, | ||
| 'aria-label': { | ||
| type: 'string' | ||
| type: 'string', | ||
| allowEmpty: true |
There was a problem hiding this comment.
Do we have a test to make sure that an empty aria-label will not pass an input with aria-label=""?
There was a problem hiding this comment.
aria-label has always been allowed empty. See: https://github.com/dequelabs/axe-core/pull/1154/files#diff-0dbb83fa6e11da52031f948e3f85a3ecL105
Rules that require elements to have an accessible name don't allow empty or whitespace values to pass.
| @@ -55,21 +59,24 @@ lookupTable.attributes = { | |||
| values: ['copy', 'move', 'reference', 'execute', 'popup', 'none'] | |||
| }, | |||
| 'aria-errormessage': { | |||
| type: 'idref' | |||
| type: 'idref', | |||
| allowEmpty: true | |||
There was a problem hiding this comment.
Is this new? If so, where are the tests for it?
There was a problem hiding this comment.
In order to make this configurable per attribute I added this. This property is used in aria.validateAttrValue, which is where the tests for it are. All changes (see PR description) are also tested in integration tests.
There was a problem hiding this comment.
There is no current test to test that this passes when empty
| }, | ||
| 'aria-labelledby': { | ||
| type: 'idrefs' | ||
| type: 'idrefs', | ||
| allowEmpty: true |
There was a problem hiding this comment.
There is no current test to test the empty case for aria-labelledby - we should add one
| @@ -113,10 +124,12 @@ lookupTable.attributes = { | |||
| values: ['horizontal', 'vertical'] | |||
| }, | |||
| 'aria-owns': { | |||
| type: 'idrefs' | |||
| type: 'idrefs', | |||
| allowEmpty: true | |||
There was a problem hiding this comment.
There is no current test for aria-owns to test that it is allowed to be empty
| }, | ||
| 'aria-placeholder': { | ||
| type: 'string' | ||
| type: 'string', | ||
| allowEmpty: true |
There was a problem hiding this comment.
There are no tests for aria-placeholder
| values: ['true', 'false', 'spelling', 'grammar'] | ||
| }, | ||
| 'aria-keyshortcuts': { | ||
| type: 'string' | ||
| type: 'string', | ||
| allowEmpty: true |
There was a problem hiding this comment.
No tests for aria-keyshortcuts
| }, | ||
| 'aria-expanded': { | ||
| type: 'nmtoken', | ||
| values: ['true', 'false', 'undefined'] | ||
| }, | ||
| 'aria-flowto': { | ||
| type: 'idrefs' | ||
| type: 'idrefs', | ||
| allowEmpty: true |
There was a problem hiding this comment.
No current test for aria-flowto to test that it passes when empty
| @@ -55,21 +59,24 @@ lookupTable.attributes = { | |||
| values: ['copy', 'move', 'reference', 'execute', 'popup', 'none'] | |||
| }, | |||
| 'aria-errormessage': { | |||
| type: 'idref' | |||
| type: 'idref', | |||
| allowEmpty: true | |||
There was a problem hiding this comment.
There is no current test to test that this passes when empty
| values: ['page', 'step', 'location', 'date', 'time', 'true', 'false'] | ||
| }, | ||
| 'aria-describedby': { | ||
| type: 'idrefs' | ||
| type: 'idrefs', | ||
| allowEmpty: true |
There was a problem hiding this comment.
No current test fo aria-describedby to show that it passes when empty
| @@ -37,14 +38,17 @@ lookupTable.attributes = { | |||
| type: 'int' | |||
| }, | |||
| 'aria-controls': { | |||
| type: 'idrefs' | |||
| type: 'idrefs', | |||
| allowEmpty: true | |||
There was a problem hiding this comment.
No current test to show that this passes when empty
|
@dylanb you were totally right. Actually found a bug with aria-errormessage. This should solve it. |
| @@ -45,70 +45,3 @@ aria.validateAttr = function(att) { | |||
| 'use strict'; | |||
| return !!aria.lookupTable.attributes[att]; | |||
| }; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I wish this file was moved in a separate commit so I could see if there were any changes to it side by side.... Lots of style changes mixed in here.
There was a problem hiding this comment.
I'm not sure how to do this.
There was a problem hiding this comment.
I thought maybe you could make the changes in the existing file in one commit and move/rename it in a separate commit. I don't think it's worth holding up this PR, just something to keep in mind moving forward.
|
|
||
| afterEach(function() { | ||
| axe.commons.aria.lookupTable.attributes[ |
There was a problem hiding this comment.
Do we do this object assignment anywhere else? Maybe it makes sense to abstract into a utility that we can reuse?
There was a problem hiding this comment.
Not that I know of.
|
@WilcoFiers are you going to add any more unit tests to this like Dylan mentioned, or is it covered enough by the integration tests? |
|
|
||
| function validateAttrValue() { | ||
| var idref = attr && doc.getElementById(attr); | ||
| function validateAttrValue(attr) { |
There was a problem hiding this comment.
Why is there a method here called validateAttrValue when there is also one in commons.aria? Should they be the same method, or at least have different names?
There was a problem hiding this comment.
I don't know. It was something you approved. Is it relevant to this PR?
c96f58c#diff-bbc5b3826882793926c0ad8f410725fe
There was a problem hiding this comment.
That PR doesn't reference the commons method, only the inline function. This PR has both.
Allow specifying per ARIA attribute which ones are allowed to be empty. Changes include:
Closes #994
Reviewer checks
Required fields, to be filled out by PR reviewer(s)