-
Notifications
You must be signed in to change notification settings - Fork 795
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-required-attr): allow aria-valuenow to pass on elements with value #1579
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think changing formControlValue
so that it can return null
is a good idea. AccName is one of the most complex parts of axe-core. There are significant downstream costs to adding features to it, and we can only add so many of them before it becomes brittle again.
AccName and its parts should have one clearly defined purpose, and not try to be the answer to every accessible name question in axe-core. That's how the original accName implementation got as buggy as it was. Nobody could maintain it, because we didn't know if the code that was in their was to serve some edge case in a rule, if it was build to work around some browser edge case, or if something was done because of the spec.
I think there's a much simpler way to solve this, add an isNativeTextbox
method somewhere, that you can use in the check, as well as in nativeTextboxValue
. Either that or simply copy the code that tests that over into the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor point.
No additional rule help required. |
Because
<input type="number" role="spinbutton">
should pass even though it has an empty string value''
, I had to modify theformControlValue
function to differentiate between a form control with a value (even if empty string) and one that didn't.After trying a few things, this solution was the least worst option. I wanted to create a method that returned the true value property (with no empty string default). But since each of the form control value methods are public, I would have had to create a similar method for each that just returned the true value. That resulted in tons of duplicate tests to test the default value and the text method that returned an empty string.
For axe 4.0, I would like to remove the
text.formControlValueMethods
from the public api and only test thetext.formControlValue
function directly. That would allow us to clean this and the unit tests up to make a cleaner api. As it stands, the unit tests mostly test the individual methods but not the mainformControlValue
method that does all the logic. So if the main method had an error but the individual functions were correct, we would never know.Closes: #1501
Reviewer checks
Required fields, to be filled out by PR reviewer(s)