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-allowed-attr): no inconsistent aria-checked on HTML checkboxes #3895

Merged
merged 9 commits into from
May 2, 2023

Conversation

WilcoFiers
Copy link
Contributor

Because I want to add explicit messages without confusing the missing messages of the aria-allowed-attr check I decided to create a new aria-conditional-attr check. I moved existing code that checks that row attributes that are allowed on treegrid but not on table or grid into that new check too.

I put the checkbox code and row code into separate evaluate methods, that then get invoked by ariaConditionalAttrEvaluate. Alternatively they could have been different checks entirely. I'm not keen on adding a new check every time we have a role for which we want some extra logic, that felt excessive. Then again there's only two of them so maybe they could have just been two checks? I'm open to changing this.

Closes: #3307

import { getRole, allowedAttr, validateAttr } from '../../commons/aria';
import { isFocusable } from '../../commons/dom';
import cache from '../../core/base/cache';
Copy link
Contributor Author

@WilcoFiers WilcoFiers Jan 26, 2023

Choose a reason for hiding this comment

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

I didn't think this cache was useful, so I've left it out on my rewrite. If we ever want to cache getRole, it should be memoized. And unlike this current implementation I'm only looking up the table role once per row, not for every attribute.

Comment on lines -72 to -75
if (validateAttr(attrName) && preChecks[attrName]?.()) {
invalid.push(attrName + '="' + virtualNode.attr(attrName) + '"');
} else if (validateAttr(attrName) && !allowed.includes(attrName)) {
invalid.push(attrName + '="' + virtualNode.attr(attrName) + '"');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was really weird and messy. Took some time refactoring and cleaning this up. Much easier to grok now. The gist of what this does is that if you're using one of these validTreeRowAttrs on a table or grid, it should fail because those attrs are only allowed on treegrid.

Comment on lines +10 to +19
export default function ariaConditionalAttrEvaluate(
node,
options,
virtualNode
) {
const role = getRole(virtualNode);
if (!conditionalRoleMap[role]) {
return true;
}
return conditionalRoleMap[role].call(this, node, options, virtualNode);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this was over-engineering or not. Having two new checks would be fine, but then if we wanted to add more that becomes kind of bloated. These things are mutually exclusive because they run for different roles.

Copy link
Contributor

Choose a reason for hiding this comment

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

They certainly should be their own functions, but having them be "evaluate" methods is interesting, especially since they are two evaluate functions without an associated metadata file or test file.

I can see why having them function like evaluate methods is helpful so you can call data() in them, but you could do that using a destructured return and calling data in here. I think I would prefer them as a common function or something rather than their own evaluate files. That way they can't accidentally be used in a rule, and they can be tested in their own files rather than combining the tests into a single file for aria-conditional-attr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess I can put them into commons/aria then.

@@ -7,7 +7,7 @@
"description": "Ensures ARIA attributes are allowed for an element's role",
"help": "Elements must only use allowed ARIA attributes"
},
"all": [],
"any": ["aria-allowed-attr"],
"all": ["aria-allowed-attr", "aria-conditional-attr"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't like "none" checks. Always messes with my head that false means pass. Can't undo that on the other checks but I don't want to make it worse.

@WilcoFiers WilcoFiers marked this pull request as ready for review January 26, 2023 12:40
@WilcoFiers WilcoFiers requested a review from a team as a code owner January 26, 2023 12:40
Comment on lines 29 to 30
const isChecked = vNode =>
vNode.actualNode ? !!vNode.actualNode.checked : vNode.hasAttr('checked');
Copy link
Contributor Author

@WilcoFiers WilcoFiers Jan 26, 2023

Choose a reason for hiding this comment

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

Using actualNode.checked because if you toggle a checkbox it doesn't update the checked attribute. It's not a prop on virtualNode

@WilcoFiers WilcoFiers marked this pull request as draft January 26, 2023 16:54
@WilcoFiers
Copy link
Contributor Author

Back to draft, need to handle indeterminate checkboxes.

@WilcoFiers WilcoFiers marked this pull request as ready for review January 27, 2023 13:00
doc/check-options.md Show resolved Hide resolved
lib/checks/aria/aria-conditional-row-attr-evaluate.js Outdated Show resolved Hide resolved
Comment on lines +10 to +19
export default function ariaConditionalAttrEvaluate(
node,
options,
virtualNode
) {
const role = getRole(virtualNode);
if (!conditionalRoleMap[role]) {
return true;
}
return conditionalRoleMap[role].call(this, node, options, virtualNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

They certainly should be their own functions, but having them be "evaluate" methods is interesting, especially since they are two evaluate functions without an associated metadata file or test file.

I can see why having them function like evaluate methods is helpful so you can call data() in them, but you could do that using a destructured return and calling data in here. I think I would prefer them as a common function or something rather than their own evaluate files. That way they can't accidentally be used in a rule, and they can be tested in their own files rather than combining the tests into a single file for aria-conditional-attr.

test/checks/aria/aria-conditional-attr.js Outdated Show resolved Hide resolved
test/checks/aria/aria-conditional-attr.js Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers changed the title feat(aria-allowed-attr): no inconsistent aria-checked on HTML checkboxes fix(aria-allowed-attr): no inconsistent aria-checked on HTML checkboxes Apr 17, 2023
@WilcoFiers WilcoFiers merged commit 704043e into develop May 2, 2023
@WilcoFiers WilcoFiers deleted the native-checkbox-aria-checked branch May 2, 2023 13:55
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.

Update allowance of aria-checked on HTML checkbox and radios
2 participants