-
Notifications
You must be signed in to change notification settings - Fork 9
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 and rename LandmarkHasLabel rule to NavigationHasLabel #64
Conversation
dd1574f
to
5295b93
Compare
cc8a730
to
a95a56f
Compare
👋 Hello and thanks for pinging us! An accessibility first responder will review this soon.
|
include ERBLint::Linters::CustomHelpers | ||
include LinterRegistry | ||
|
||
MESSAGE = "The navigation landmark should have a unique accessible name via `aria-label` or `aria-labelledby`." |
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.
Should we say/ check that users do not include "navigaiton" in the name
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 added it to the message. As for it being a check, I'm open to following up on it separately, maybe as a distinct rule.
Fixes: https://github.com/github/accessibility/issues/3290
This is a lint rule that was written before our teams time that looks like we need to revisit.
This appears to be flagging all
<aside>
,<nav>
, and<section>
without an accessible name.Flagging
<section>
seems incorrect since we don’t know if the<section>
is intended to be a landmark. A<section>
only becomes a region landmark if its labelled, but we don't know if the developer intends it to be.Similarly, I am not sure we about flagging
<aside>
.I think it makes sense to enforce a navigation landmark to have an accessible name given that GitHub sites usually have multiple navigation and it would help for those to be labelled.
I am simplifying this rule significantly from what it is currently doing so it only flag:
nav
orrole="navigation"
has no label.nav
ifrole="navigation"
is being used.Consequently, I renamed the rule to be
NavigationHasLabel
. If we decide we want to add more enforcements around landmark, it should be a more scoped rule.