-
Notifications
You must be signed in to change notification settings - Fork 783
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(rules/region): Treat <section> as a landmark if it has an accessible name #640 #642
Conversation
lib/checks/navigation/region.js
Outdated
@@ -26,7 +26,9 @@ function isLandmark (node) { | |||
if (node.hasAttribute('role')) { | |||
return landmarkRoles.includes(node.getAttribute('role').toLowerCase()); | |||
} else { | |||
return implicitLandmarks.includes(node.nodeName.toUpperCase()); | |||
return implicitLandmarks.some((implicitSelector) => { |
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.
That looks like magic to me. Can you add a comment about what's happening here?
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.
Looks like he's electing to use the matchesSelector
utility to determine whether the node has an implicit landmark role, instead of simply checking whether the array of of implicit landmark roles includes the nodeName. I'm guessing this wasn't working for all nodes, since the nodeName of an element with an implicit landmark role is often different than the landmark role. E.g., given a <header>
element, #nodeName === 'HEADER'
, but its implicit role is 'banner'.
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.
This also allows the values in the role's "implicit" entry in the lookup table to be CSS selectors more complex than just the nodeName. E.g., input[type="button"]
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.
Thanks! I'd still love to see a comment in the code for future reading.
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.
Good stuff!
Also merged into 2x |
@WilcoFiers Thank you |
This closes #640.