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-valid-attr-value): report empty values as incomplete #3635

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

WilcoFiers
Copy link
Contributor

There is no real difference between an empty ARIA attribute, a null attribute, and not having the attribute at all. Failing empty ARIA attributes therefor seems a little strict. They shouldn't be passed either though, since it is pretty easy to mistakenly assume <input aria-invalid> means the input is invalid. Reporting incomplete seems more appropriate. This PR makes that change, along with:

  • String type attributes, like aria-valuenow are still not allowed empty
  • aria-invalid is now reported as incomplete when empty, rather than passed
  • Extra tests were added to aria-required-attr to ensure empty attributes when required are failed there

Closes issue: #3489

@WilcoFiers WilcoFiers requested a review from a team as a code owner September 5, 2022 12:12
@@ -106,30 +107,35 @@ function ariaValidAttrValueEvaluate(node, options, virtualNode) {
} catch (e) {
needsReview = `${attrName}="${attrValue}"`;
messageKey = 'idrefs';
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoid getting into that if statement below, as we don't want both needsReview and invalid set.

}
});

if (needsReview) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Report invalid before needsReview.

@@ -1,48 +1,72 @@
describe('aria-required-attr', function() {
describe('aria-required-attr', function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check didn't change, just want to better prove empty values are not allowed under this check.

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.

2 participants