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(heading-order): use aria-level on headings in addition to role=header elements #3028

Merged
merged 8 commits into from
Jun 25, 2021

Conversation

clottman
Copy link
Contributor

Allow aria-level on hn elements to override the semantic level of the element.

Closes issue: #2971

@clottman clottman requested a review from straker June 22, 2021 16:04
@clottman clottman requested a review from a team as a code owner June 22, 2021 16:04
// default aria-level for a heading is 2 if it is
// not set or set to an incorrect value
// @see https://www.w3.org/TR/wai-aria-1.1/#heading
if ((headingRole && isNaN(level)) || level < 1 || level > 6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to fail on <h1 role="heading">. Not sure if that's a new problem, but it's a problem.


if (level) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will return the aria-level for non-header elements like <iframe aria-level="2"></iframe>, when we actually need the iframe to be level=-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rule doesn't run on iframes, as far as I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rule doesn't, but we use a selector inside the evaluate to look for iframes and assign them a level of -1 so in the after we can reconstruct heading order correctly for all iframes on the page.

const selector = 'h1, h2, h3, h4, h5, h6, [role=heading], iframe, frame';
// TODO: es-modules_tree
const vNodes = querySelectorAllFilter(axe._tree[0], selector, vNode =>
isVisible(vNode.actualNode, true)
);
headingOrder = vNodes.map(vNode => {

@clottman clottman dismissed stale reviews from straker and WilcoFiers June 22, 2021 21:30

addressed

lib/checks/navigation/heading-order-evaluate.js Outdated Show resolved Hide resolved
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
@clottman clottman requested a review from straker June 23, 2021 18:39
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 believe this is the last thing. Everything else looks good. Good work.

test/checks/navigation/heading-order.js Outdated Show resolved Hide resolved
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
* @see https://www.w3.org/TR/wai-aria-1.1/#heading
* @see https://codepen.io/straker/pen/jOBjNNe
*/
if (isNaN(ariaLevel) || ariaLevel < 1 || ariaLevel > 6) {
Copy link
Contributor

@straker straker Jun 25, 2021

Choose a reason for hiding this comment

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

Alright. Wilco and I talked and we came to the conclusion that we want to change how we're handling positive values. Since aria-level > 6 is handled differently by different screen readers, we're going to flag it as part of a new check for aria-valid-attr-value and call out its lack of support.

But for heading-order we're just going to accept what the user gave us so there isn't two different failures caused by the same problem.

Suggested change
if (isNaN(ariaLevel) || ariaLevel < 1 || ariaLevel > 6) {
if (isNaN(ariaLevel) || ariaLevel < 1) {

@straker straker merged commit caccd38 into develop Jun 25, 2021
@straker straker deleted the 2971-heading-order-aria-level branch June 25, 2021 16:16
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.

None yet

3 participants