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

landmark-unique violation at /user_guide/theme-elements.html #1478

Closed
Tracked by #1428
gabalafou opened this issue Sep 26, 2023 · 7 comments · Fixed by #1607
Closed
Tracked by #1428

landmark-unique violation at /user_guide/theme-elements.html #1478

gabalafou opened this issue Sep 26, 2023 · 7 comments · Fixed by #1607
Assignees
Labels
tag: accessibility Issues related to accessibility issues or efforts

Comments

@gabalafou
Copy link
Collaborator

gabalafou commented Sep 26, 2023

Sub-issue of #1428.

The landmark-unique violation shows up on the Theme-specific elements page because the following directive gets transformed into a <nav> element:

```{contents} Page contents
:local:
```

As there is already another unlabelled <nav> on the page (in the header), this causes the Axe landmark-unique rule to fail.

@gabalafou
Copy link
Collaborator Author

gabalafou commented Sep 26, 2023

One seemingly easy way to solve this issue would be to label the header nav as "Site" or "Main" using aria-label. Currently the header nav is unlabelled and is still unlabelled in PR #1454 (intentionally). However, it is not the best solution for the end user if we fix this issue this way because it masks another problem.

The problem is that some pages, like the theme-specific elements page, can render the same table of contents twice: once at the top of the article, and once in the right sidebar under the heading "On this page." Currently, both of these TOCs are marked up in <nav>s, which means that they appear as two separate landmarks to assistive tech (AT). I do not think it is useful to AT users to create two separate entries in the AT landmarks menu that point to the exact same info, especially if those entries have different labels—for example, if one TOC nav says "Page contents navigation" and the other says "On this page navigation", but they both contain the same table of contents.

So a robust solution to the issue that this test failure exposes may be a bit challenging. A perfectly robust solution would require the renderer to know that there are two duplicated "page content" navs on the page and decide to markup only one of them with a <nav> tag.

However, maybe there are some "good enough" solutions. For example, if we can be pretty confident that most pages will have the "On this page" sidebar navigation, then maybe we can decide that the sidebar TOC gets rendered in a <nav> tag but that a TOC within an article never gets rendered inside a <nav>.

@trallard trallard added the tag: accessibility Issues related to accessibility issues or efforts label Sep 27, 2023
@drammock
Copy link
Collaborator

IMO the in-page TOC on the "theme-specific elements" page should just go away. It is redundant with the right sidebar TOC, which (by default anyway) is always present on all pages that have subsections. (and in general we shouldn't be putting in-page TOCs on pages where we don't suppress the sidebar in-page TOC)

@12rambau
Copy link
Collaborator

fully agreed, nuke the TOC 🚀

@gabalafou
Copy link
Collaborator Author

gabalafou commented Oct 5, 2023

But I think @choldgraf specifically added the TOC to that page to check PST styles with the contents directive.

Furthermore, there are least two examples of its usage (in conjunction with the right-sidebar TOC) in the wild:

Looks like it was Docutils 0.18 that started using <nav> for rendering TOCs in HTML.

Here are the paths forward that I see, in no particular order:

  1. Use a <div> tag instead of a <nav> in the right sidebar.
    Then, when a page has a TOC in two places, only one of them will show up in the page landmarks. However, this means that when the page only has one TOC (in the right sidebar), it will not show up in the page landmarks. Is that bad from an accessibility perspective? I don't know. This is where we hit the limits of my accessibility knowledge. For reference, the W3C WAI pages wrap the "page contents" in a navigation landmark.

  2. If there is a way to detect when a contents-directive is used by a page at build time, then on those pages, we could switch the page-toc.html template to not render the right sidebar TOC. It would look something like this:

    {% if page_does_not_already_have_toc %}
      <nav class="bd-toc-nav page-toc">
        {{ page_toc }}
      </nav>
    {% endif %}
    

    Is there a way to detect that? Maybe it's possible within the generate_toc_html() function?

  3. Don't change anything about the HTML render process. Add a note to our docs somewhere letting our users know that if they include a contents directive on a page, then they should also add :html_theme.sidebar_secondary.remove:. We'll also need to mark the test failure as a known test failure so that the a11y test suite passes.

@12rambau
Copy link
Collaborator

12rambau commented Oct 5, 2023

here are my .2$ on the different suggestions you made:

  1. for me secondary sidebar needs to remain a nav as it is a nav. because for pages without the content directive it should not remain a div.
  2. To my knowledge it's time consuming to run this type of test in the generat_toc_html method and we are already face time computation issues with big API like scipy so I would not go down this path.
  3. The content directive make a lot of sense in sphinx itself because there is no secondary sidebar. As we provide this third column I think it more or less make the "content" directive obsolete. let's keep it for display and add anything needed in the test to avoid a11y failure.

@drammock
Copy link
Collaborator

drammock commented Oct 5, 2023

But I think @choldgraf specifically added the TOC to that page to check PST styles with the contents directive.

If that's the reason, then we can do it in a test rather than on our live site (and validate that it looks right using selenium etc). IMO we shouldn't be adding things to the live site that are considered "bad practice" just to see if they work.

for me secondary sidebar needs to remain a nav as it is a nav.

agreed.

@gabalafou
Copy link
Collaborator Author

IMO we shouldn't be adding things to the live site that are considered "bad practice" just to see if they work.

I opened a PR, #1607, that I think is a good compromise, let me know what you think! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: accessibility Issues related to accessibility issues or efforts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants