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 landmarks #1454

Merged
merged 18 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,9 @@ function showVersionWarningBanner(data) {
return;
}
// now construct the warning banner
var outer = document.createElement("div");
var outer = document.createElement("aside");
// TODO: add to translatable strings
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to do this in a follow-up PR not this one because this PR has been open for a while and has gotten stuck on translation stuff, so I would like to tackle the translation issues separately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry what is the translation issue ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's been a while so I don't actually remember what issues I was running into, but in a previous version of this PR, I had changed a translatable string, so then I tried to go through the nox -s translate -- extract and nox -s translate -- update steps but I ran into issues or didn't understand what was going on. So I decided to revert the change to the translatable string _("Section Navigation").

Similarly, here I don't know how to mark this string as translatable (because it's a .js file) and even if I did know how to mark it as translatable, I would still want to defer the work of putting it through the translation machinery until later, in the interest of getting this PR merged now and handling translation stuff in a subsequent PR. Does that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The string here, "Version warning", is used to label the landmark (<aside> => role = complementary) that I added to the version warning banner. (The rationale is that since we make the version warning banner so visually prominent, we should also make it prominent for assistive tech, hence the landmark.)

Eventually the string needs to be translated. It will be surfaced by assistive tech when showing or announcing landmarks on the page: "Version warning, complementary" (something like that)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you open a new issue to remind us to come back to this (question about marking strings as translatable if they're generated within JavaScript) please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#1587 😃

outer.setAttribute("aria-label", "Version warning");
const middle = document.createElement("div");
const inner = document.createElement("div");
const bold = document.createElement("strong");
Expand Down Expand Up @@ -509,7 +511,8 @@ function showVersionWarningBanner(data) {
inner.appendChild(bold);
inner.appendChild(document.createTextNode("."));
inner.appendChild(button);
document.body.prepend(outer);
const skipLink = document.getElementById("pst-skip-link");
skipLink.after(outer);
}

/*******************************************************************************
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{# Displays links to the Sphinx-generated indices (genindex, modindex, py-modindex). #}
<nav class="sidebar-indices-items">
<p class="sidebar-indices-items__title" role="heading" aria-level="1">{{ _("Indices") }}</p>
{%- set indices_navigation_heading_id = unique_html_id("pst-indices-navigation-heading") -%}
<nav class="sidebar-indices-items" aria-labelledby="{{ indices_navigation_heading_id }}">
<p id="{{ indices_navigation_heading_id }}" class="sidebar-indices-items__title" role="heading" aria-level="1">{{ _("Indices") }}</p>
<ul class="indices-link">
{%- for rellink in rellinks %}
{%- if rellink[0] == 'genindex' %}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
{# Displays the current page's Table of Contents. #}
{% set page_toc = generate_toc_html() %}
{%- if page_toc | length >= 1 %}
<div class="page-toc tocsection onthispage">
{%- set page_navigation_heading_id = unique_html_id("pst-page-navigation-heading") -%}
<div
id="{{ page_navigation_heading_id }}"
class="page-toc tocsection onthispage">
<i class="fa-solid fa-list"></i> {{ _("On this page") }}
</div>
<nav class="bd-toc-nav page-toc">
<nav class="bd-toc-nav page-toc" aria-labelledby="{{ page_navigation_heading_id }}">
{{ page_toc }}
</nav>
{%- endif %}
12 changes: 8 additions & 4 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@

{# A button hidden by default to help assistive devices quickly jump to main content #}
{# ref: https://www.youtube.com/watch?v=VUR0I5mqq7I #}
<a class="skip-link" href="#main-content">{{ _("Skip to main content") }}</a>
<a id="pst-skip-link" class="skip-link" href="#main-content">{{ _("Skip to main content") }}</a>

{%- endblock %}

Expand Down Expand Up @@ -81,14 +81,18 @@
<div class="search-button__overlay"></div>
<div class="search-button__search-container">{% include "../components/search-field.html" %}</div>
</div>

<header>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also had to do with All page content should be contained by landmarks.

Note that this PR does not fix all instances of this error everywhere in the theme. (For example, the Read The Docs switcher violates this rule.)

{%- if theme_announcement -%}
{% include "sections/announcement.html" %}
{%- endif %}
{% block docs_navbar %}
<nav class="bd-header navbar navbar-expand-lg bd-navbar">
<div class="bd-header navbar navbar-expand-lg bd-navbar">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The <nav> that I change here used to contain a nav. Nesting navs is bad practice.

This outer nav contains the logo, the version switcher, the list of header links, the search bar, the theme switcher, and the icon links to Twitter, GitHub, PyPI and PyData.

The inner nav contains the list of header links, so I think it better matches the nav element semantics.

Furthermore, on mobile this outer nav changes so that it only shows the left hamburger (site and section navigation), the logo, the search, and the right hamburger (page navigation). Whereas the inner nav (based on the Jinja navbar-nav.html component), contains the same content on desktop and mobile.

On mobile, the semantics become very clear. On mobile the sidebar contains two navigation sections: one for the site, one for the section of the site that you're in:

So I think the inner nav should keep the nav semantics, whereas the nav semantics here on this line should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As an aside, there's something that feels not quite right to me about putting external links under a heading that says "Site Navigation" but maybe that's an issue for another day.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As an aside, there's something that feels not quite right to me about putting external links under a heading that says "Site Navigation" but maybe that's an issue for another day.

yes let's punt on that. Users like to do it (have external links in the main topbar nav) and would be mad if we removed the ability to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. But maybe the heading should be "Main Navigation" or something like that instead of "Site Navigation"? Or maybe we get rid of the visible heading altogether and set aria-label="Main" on the nav tag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm OK with "Main navigation" instead of "Site navigation"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After thinking about this some more, may I make an argument to simply remove the heading?

  1. Consistency between mobile and desktop, because this heading is not shown on desktop, but is shown on mobile
  2. Frees up valuable screen real estate
  3. Reduces number of strings to translate
  4. Solves the problem of screen reader noise: "Site Navigation navigation" or "navigation Site navigation"
  5. I'm not sure it's really needed in the mobile sidebar as the row of icon links followed by a horizontal line already creates strong visual break between the two nav sections:
    icons and horizontal line separate site from section nav

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you open a new issue to discuss further the idea of removing the "Site navigation" title?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#1588 :)

{%- include "sections/header.html" %}
</nav>
</div>
{% endblock docs_navbar %}
</header>

<div class="bd-container">
<div class="bd-container__inner bd-page-width">
{# Primary sidebar #}
Expand All @@ -107,7 +111,7 @@
{% block docs_body %}
{# This is empty and only shows up if text has been highlighted by the URL #}
{% include "components/searchbox.html" %}
<article class="bd-article" role="main">
<article class="bd-article">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duplicate main landmark (there should only be one on the page). If you look just a few lines above, you'll find a <main> tag.

I think this was an oversight of #1019.

{% block body %}{% endblock %}
</article>
{% endblock docs_body %}
Expand Down
Loading