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.

Sign up for GitHub

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

Jump to bottom

Fix landmarks #1454

Merged
merged 18 commits into from
Dec 7, 2023
Merged

Fix landmarks #1454

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
eeb9191
add header
gabalafou Sep 18, 2023
3c1fc72
fix nav landmarks
gabalafou Sep 18, 2023
2405b5d
only one main
gabalafou Sep 18, 2023
bf611d0
label indices nav
gabalafou Sep 18, 2023
4760d6e
few landmarks
gabalafou Sep 18, 2023
1e461b7
unique id todos
gabalafou Sep 18, 2023
e6b5bd3
Merge branch 'main' into fix-landmarks
gabalafou Sep 26, 2023
d1b4622
fix test fixtures
gabalafou Sep 27, 2023
346cb87
fix translations
gabalafou Sep 27, 2023
6fc4807
Merge branch 'main' into fix-landmarks
gabalafou Sep 27, 2023
b13a1b6
Merge branch 'main' into fix-landmarks
gabalafou Oct 24, 2023
a5f1f8e
Merge branch 'main' into fix-landmarks
gabalafou Nov 9, 2023
87db857
translations?
gabalafou Nov 9, 2023
52b968d
make version warning a landmark
gabalafou Nov 10, 2023
bd26466
revert translation changes
gabalafou Nov 29, 2023
dfa1a26
revert changes to noxfile.py
gabalafou Nov 29, 2023
3bb86f2
update tests to reflect translation reversal
gabalafou Nov 29, 2023
fc12dbc
add TODO comment to make string translatable
gabalafou Nov 29, 2023
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
2 changes: 1 addition & 1 deletion src/pydata_sphinx_theme/assets/scripts/pydata-sphinx-theme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ function showVersionWarningBanner(data) {
inner.appendChild(bold);
inner.appendChild(document.createTextNode("."));
inner.appendChild(button);
document.body.prepend(outer);
(documment.querySelector("header") || document.body).prepend(outer);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All page content should be contained by landmarks

Copy link
Collaborator Author

@gabalafou gabalafou Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One other thing to think about. We currently make this version warning very visually prominent. If we are making it this prominent for sighted users, should we also make it prominent for blind users by wrapping it in its own landmark, possibly with the <aside> tag?

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we also make it prominent for blind users by wrapping it in its own landmark

yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! 😊

"Version warning complementary" is first line in the landmarks menu of the VoiceOver rotor

Note: don't worry about the black outline around the version warning banner in the above screen shot; it's added by VoiceOver.

}

/*******************************************************************************
Expand Down
5 changes: 3 additions & 2 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/indices.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<nav class="sidebar-indices-items">
<p class="sidebar-indices-items__title" role="heading" aria-level="1">{{ _("Indices") }}</p>
{#- TODO use unique html id generator since components can be included in multiple places -#}
<nav class="sidebar-indices-items" aria-labelledby="pst-indices-navigation-heading">
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 TODO comments are because of #1425

<p id="pst-indices-navigation-heading" class="sidebar-indices-items__title" role="heading" aria-level="1">{{ _("Indices") }}</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may have too many navigation landmarks in the theme. Online guides say to use landmarks sparingly.

But I decided to punt on that question for now and just try to improve the existing landmarks and only remove landmarks when they were actually bugs (nav inside nav, two mains, etc.)

<ul class="indices-link">
{%- for rellink in rellinks %}
{%- if rellink[0] == 'genindex' %}
Expand Down
7 changes: 5 additions & 2 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/page-toc.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
{% set page_toc = generate_toc_html() %}
{%- if page_toc | length >= 1 %}
<div class="page-toc tocsection onthispage">
{#- TODO use unique html id generator since components can be included in multiple places #}
<div
id="pst-page-navigation-heading"
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="pst-page-navigation-heading">
{{ page_toc }}
</nav>
{%- endif %}
5 changes: 3 additions & 2 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/sidebar-nav-bs.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{#- TODO use unique html id generator since components can be included in multiple places -#}
<nav class="bd-docs-nav bd-links"
aria-label="{{ _('Section Navigation') }}">
<p class="bd-links__title" role="heading" aria-level="1">{{ _("Section Navigation") }}</p>
aria-labelledby="pst-section-navigation-heading">
<p id="pst-section-navigation-heading" class="bd-links__title" role="heading" aria-level="1">{{ _("In this section") }}</p>
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 needed to change the label on the nav because it was noisy, causing screen readers to say "Section Navigation navigation."

I also thought that "Section Navigation" as a label/header was a bit ambiguous. Section of what? section of the site? section of the page?

My first instinct was to change the heading to "Pages," which means it would be announced by screen readers as "Pages navigation." But then I noticed that the navigation for sections of the page was called "On this page," so I thought maybe it makes sense to call this nav group "In this section," leading to the screen reader announcements "On this page navigation" and "In this section navigation," respectively.

Taken all together, this means that with this PR the PST site has the following navigation landmarks:

  • navigation (no label implies top-level site navigation, I think)
  • In this section navigation
  • Breadcrumb navigation
  • On this page navigation

What do folks think? Should it perhaps be "Pages in this section navigation?"

Copy link
Collaborator

Choose a reason for hiding this comment

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

just a note that changing translated strings (strings inside _(...)) should not be done lightly (they will need to get re-translated, which is not automatic). I'm not insisting to change it back because it seems like you have good reason to do it here.

"section" here means "section of the site", refers to the top-level menu item (user guide is a section, contributor guide is another section, etc). So I think your choice of "in this section" makes sense here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. I'm already regretting my decision. It's complicating my PR 😭

<div class="bd-toc-item navbar-nav">{{ sidebar_nav_html }}</div>
</nav>
10 changes: 7 additions & 3 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html
View file
Open in desktop
Original file line number Diff line number Diff line change
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