search

valeriangalliat / markdown-it-anchor

A markdown-it plugin that adds an `id` attribute to headings and optionally permalinks.
The Unlicense
291 stars 72 forks source link

Option to remove aria-hidden from output #58

Closed zachleat closed 4 years ago

zachleat commented 5 years ago

I believe that adding aria-hidden is incorrect for these links. These links should be accessible and would be useful to screen reader users too!

Notably this throws an error when using the axe-core accessibility linting tool.

Violation of "aria-hidden-focus" with 6 occurrences! Ensures aria-hidden elements do not contain focusable elements. Correct invalid elements at:

The options to fix here are to remove aria-hidden (thus, this issue) or to set tabindex="-1" on the link, which wouldn’t be right either. I want these links to be focusable.

Thank you!

nagaozen commented 4 years ago

Hi @zachleat, thank you for pointing this out!

Initially I thought we made the right call on this because, as you said:

"... elements do not contain focusable elements ..."

So, I went to the to source looking for the aria-hidden on the header tag but instead found it on the a tag. Well, that tag does not contain focusable elements, it is the focusable element! And because of https://webaccessibility.com "Avoid placing inactive elements in the focus order" Best Practice that states:

Elements on the page that are not interactive (those that do not trigger an action) such as labels, headings, etc. should not be in the keyboard focus order. Providing focus to non-active elements may give users of assistive technology the impression that the element is interactive and cause keyboard users to have to use extra keystrokes to navigate. In some specific situations (such as dire warnings or disabled controls that require changes for them to become enabled) it may be acceptable and necessary to place a limited number of non-interactive elements in the focus order. Setting focus programmatically to certain content that is not interactive such as error messages may also be acceptable.

It made sense that for a better UX it was the right call to remove that inactive element from the focus order...

But after further research I found out that MDN clearly says:

According to the fourth rule of ARIA, aria-hidden="true" should not be used on a focusable element. Additionally, since this attribute is inherited by an element's children, it should not be added onto the parent or ancestor of a focusable element.

And an HTMLAnchorElement with a href is, hands down, considered to be a focusable element.

So I think this should be changed.

nagaozen commented 4 years ago

Any additional words on this topic @valeriangalliat?

nagaozen commented 4 years ago

While we are still discussing this, what about changing it to: role="presentation"?

nagaozen commented 4 years ago

Fixed on 5.2.5

binyamin commented 3 years ago

@zachleat @nagaozen I would like to revisit this point. How will screen-readers know to ignore the permalinkSymbol? Also, the devtools tell me that GitHub puts aria-hidden="true" on its markdown permalink thingies.

markdown permalink symbols

I like https://github.com/valeriangalliat/markdown-it-anchor/issues/58#issuecomment-542390344, though I think it's role="none" currently. Edit: axe cli doesn't like that suggestion.