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-required-attr): allow aria-valuenow to pass on elements with value #1579

Merged
merged 6 commits into from
Jun 3, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented May 20, 2019

Because <input type="number" role="spinbutton"> should pass even though it has an empty string value '', I had to modify the formControlValue 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 the text.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 main formControlValue 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)

  • 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

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.

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.

@straker straker self-assigned this May 24, 2019
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.

Minor point.

lib/commons/forms/is-aria-range.js Outdated Show resolved Hide resolved
@straker straker merged commit 3893e04 into develop Jun 3, 2019
@straker straker deleted the requiredValuenow branch June 3, 2019 17:54
@jeankaplansky
Copy link
Contributor

jeankaplansky commented Jun 4, 2019

No additional rule help required.

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.

aria-valuenow should not fail aria-required-attr when there is a fallback value
3 participants