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

perf(a11y): provide accessible header anchors #2040

Merged
merged 7 commits into from
Mar 14, 2023

Conversation

sabicalija
Copy link
Contributor

close #2039

@sabicalija sabicalija force-pushed the fix-header-anchor-aria branch 5 times, most recently from e75eec1 to bb08431 Compare March 6, 2023 14:13
@kiaking
Copy link
Member

kiaking commented Mar 8, 2023

Is there a way to achieve this without wrapping h1 with div? This is going to break styles if someone is customizing h1 style 🤔

@sabicalija sabicalija force-pushed the fix-header-anchor-aria branch from bb08431 to adfcfe4 Compare March 8, 2023 19:17
@brc-dd
Copy link
Member

brc-dd commented Mar 9, 2023

+1, it will break @vuejs/theme too 👀

@sabicalija
Copy link
Contributor Author

sabicalija commented Mar 9, 2023

Yes, but...

Thank you for raising this question. It's actually quite intricate and I'll try to sum ti up to allow for further discussion (with links to more details). I can provide more details (and tests) depending on which direction we head to.

Are you referring to h1 exclusively or did you mean all headers (h1, h2, etc.)?
Do you think about already existing styles would break or would it be problematic with this variant in general?

markdown-it-anchor provides four different permalink (a.k.a. anchor link) styles with the latest releases (since v8.0.0) and a possiblity to create custom permalinks.

Link inside header and ARIA hidden have accessibility issues, therefore I wouldn't consider them any further.

Header Link would provide what you asked for:

<h1 id="what-is-vitepress" tabindex="-1"><a class="header-anchor" href="#what-is-vitepress">What is VitePress?</h1>

The only problem with this variant is, that you cannot use any links inside the "heading".

Link after header looks like this:

<div class="header-wrapper">
  <h1 id="what-is-vitepress" tabindex="-1">What is VitePress?</h1>
  <a class="header-anchor" href="#what-is-vitepress" aria-label="Permalink to &quot;What is VitePress?&quot;">#</a>
</div>

valeriangalliat (package author) is using style Header Link himself and considers this an accessible variant. He also mentioned that this style was used with MDN and HTTP Archive.

I've reviewed the current semantics of MDN. This is how it looks right now:

<h2 id="try_it"><a href="#try_it">Try it</a></h2>

It (still) is just like permalink style Header Link.

However, MDN does not use permalinks with h1 elements. Only with h2, h3, etc.

Additionally, the overall semantics of a page look slightly different, which would be even a more accessible variant. Any chances of using similar semantics with theme-default?

<main id="content">
  <article lang="en-US">
    <h1>The HTML Section Heading elements</h1>
    <section aria-labeledby="try_it">
      <h2 id="try_it"><a href="#try_it">Try it</a></h2>
      <!-- ... -->
    </section/>
    <!-- ... -->
  </article>
  <!-- ... -->
</main>

These are the semantics of VitePress rendered pages at the moment:

<main>
  <div>
    <h1>
      What is VitePress?
      <a class="header-anchor" href="#what-is-vitepress" aria-hidden="true">#</a>
    </h1>
    <!-- ... -->
    <h2>
      Use Cases
      <a class="header-anchor" href="#use-cases" aria-hidden="true">#</a>
    </h2>
    <!-- ... -->
  </div>
  <!-- ... -->
</main>

I guess it would require also changing some of the rendering by markdown-it and introduce more problems in terms of styles. But, the theme would be more accessible for everyone and since VitePress is used for a lot of documentation, many people would benefit from it.

@sabicalija sabicalija changed the title fix(theme): provide accessible header anchors perf(a11y): provide accessible header anchors Mar 10, 2023
@yyx990803
Copy link
Member

yyx990803 commented Mar 13, 2023

Can we change it to something like what Docusaurus is doing? Example here https://docusaurus.io/docs

Screenshot 2023-03-13 at 5 29 22 PM

This would preserve the same DOM structure as we have now. They use :before + CSS to render the # instead of aria-hidden, maybe that's the trick?

@yyx990803 yyx990803 added this to the v1.0.0 milestone Mar 13, 2023
@sabicalija
Copy link
Contributor Author

Can we change it to something like what Docusaurus is doing? Example here https://docusaurus.io/docs

Screenshot 2023-03-13 at 5 29 22 PM

This would preserve the same DOM structure as we have now. They use :before + CSS to render the # instead of aria-hidden, maybe that's the trick?

I think we can achieve this relatively easily with the Custom Renderer and I'll update the PR, after we find the best option.

But I don't think, that it is that relevant, if the # is rendered using :before or simply as the content of the anchor element.

The problem how aria-hidden is used right now, is, that there is content (namely, the anchor link) that is hidden from users with visual impairments.

<main>
  <div>
    <h1>
      What is VitePress?
      <a class="header-anchor" href="#what-is-vitepress" aria-hidden="true">#</a>
    </h1>
    <!-- ... -->
    <h2>
      Use Cases
      <a class="header-anchor" href="#use-cases" aria-hidden="true">#</a>
    </h2>
    <!-- ... -->
  </div>
  <!-- ... -->
</main>

A simple aria-label describing this link, instead of aria-hidden, which hides content from people with visual impairment, would already be sufficient and avoid excluding a certain user group.

<main>
  <div>
    <h1>
      What is VitePress?
      <a class="header-anchor" href="#what-is-vitepress" aria-label="Permalink to &quot;What is VitePress?&quot;">#</a>
    </h1>
    <!-- ... -->
    <h2>
      Use Cases
      <a class="header-anchor" href="#use-cases" aria-label="Permalink to &quot;Use Cases&quot;">#</a>
    </h2>
    <!-- ... -->
  </div>
  <!-- ... -->
</main>

However, the best semantics are provided by MDN. I totally understand if you don't want to introduce breaking changes, but still I'd like to ask, if this would be an option. I guess it is still easier at this point, instead of introducing breaking changes at a later stage (after alpha).

I'm willing to provide PRs for all changes necessary (of course with some guidance by you).

@sabicalija sabicalija force-pushed the fix-header-anchor-aria branch from adfcfe4 to 7b941cd Compare March 13, 2023 22:23
@sabicalija
Copy link
Contributor Author

Can we change it to something like what Docusaurus is doing? Example here https://docusaurus.io/docs

Screenshot 2023-03-13 at 5 29 22 PM

This would preserve the same DOM structure as we have now. They use :before + CSS to render the # instead of aria-hidden, maybe that's the trick?

You're suggestion was perfect! Yes, using :before would be better for rendering the #.

With the way Docusaurus solved it, the screen reader only reads the heading without the mention of #. But when anchor # has focus (e.g. by keyboard navigation using Tab), it's described correctly.

So this would be a perfectly accessible variant. I'll update this PR ASAP.

However, I'd argue that the MDN variant is semantically better/richer. Maybe something to keep in mind for the future.

@sabicalija
Copy link
Contributor Author

sabicalija commented Mar 14, 2023

I've updated the PR with the proposed solution.

It's not made using a custom renderer but with the variant "Link After Header", although a bit hacky. But it's the best solution I could come up with, still respecting all possible user configurations (i hope 🤞).

@yyx990803
Copy link
Member

There is one failing test __tests__/e2e/home.test.ts - the test case is expecting the extra # which is now properly hidden.

LGTM once we fix the test!

@yyx990803 yyx990803 merged commit dc88efe into vuejs:main Mar 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header anchors not accessible to screen reader users.
4 participants