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(label-title-only): allow hidden labels #3183

Merged
merged 1 commit into from Oct 8, 2021
Merged

fix(label-title-only): allow hidden labels #3183

merged 1 commit into from Oct 8, 2021

Conversation

macjohnny
Copy link
Contributor

@macjohnny macjohnny commented Oct 5, 2021

take into account labels with aria-hidden="true" when evaluating the label-title-only rule.

Closes issue:
fixes #3174

@macjohnny macjohnny requested a review from a team as a code owner October 5, 2021 14:31
@CLAassistant
Copy link

CLAassistant commented Oct 5, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should instead update the labelVirtual code to no check if the aria-labelledby element is visible. I'll get back to you once I've had a chance to talk to @WilcoFiers about it.

@macjohnny
Copy link
Contributor Author

@straker

I'm wondering if we should instead update the labelVirtual code to no check if the aria-labelledby element is visible. I'll get back to you once I've had a chance to talk to @WilcoFiers about it.

I tried that as an alternative, however, the build failed when trying to use arialabelledbyText inside labelVirtual
Moreover, the doc states it only returns visible labels, which is why I went for the current solution.

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Alright, talked to Wilco and we decided that aria-hidden labels should pass, but visually hidden labels should not. That means we can just remove the true argument in the visualVirtual call in labelVirtual.

return vNode ? visibleVirtual(vNode) : '';

In conjunction with that change, we should update the description to the label-title-only rule to state what the rule is really looking for: that the element has a visual label.

    "description": "Ensures that every form element has a visible label and is not solely labeled using hidden labels, or the title or aria-describedby attributes",

@macjohnny
Copy link
Contributor Author

Ok, will update the PR soon

@macjohnny
Copy link
Contributor Author

@straker updated the PR according to your suggestions, please take a look :)

Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the pr and the updates.

@straker
Copy link
Contributor

straker commented Oct 8, 2021

Reviewed for security

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.

Possible false positive for label-title-only, caused by aria-describedby
3 participants