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: Add "skip to main content" a11y feature #12408

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

kaycebasques
Copy link
Contributor

Subject: Improve usability of core Sphinx themes for people who rely on Tab-based navigation

Feature or Bugfix

  • Bugfix

Purpose

  • Improve the accessibility of core Sphinx themes

Relates

@kaycebasques kaycebasques marked this pull request as draft June 3, 2024 22:51
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

Heya cheers, see the review comments

sphinx/themes/basic/static/basic.css_t Outdated Show resolved Hide resolved
sphinx/themes/basic/layout.html Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
@kaycebasques
Copy link
Contributor Author

Hi @chrisjsewell picking this back up. I addressed your first round of comments. LMK if you want to proceed with this and I will add the same a11y fix/feature to all the other builtin themes.

@trallard
Copy link

Hey @kaycebasques, thanks for pushing this forward.
I left a question as the choice of the target seemed a tad unusual to me.

For reference, in PyData Sphinx theme, we use the main landmark as the target directly: https://github.com/pydata/pydata-sphinx-theme/blob/0d0154b30bc29ad96bb28bc5c8ae88fe7b00177d/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html#L39

Also, in the same line linked above, you will see that we use a d-print-none class in the skip-link itself.
I think you should add this here, too. Otherwise, the Skip to main content button will show when users print stuff.

@kaycebasques
Copy link
Contributor Author

OK, I've got this working with all built-in themes. See the attached screencast for verification.

verify.webm

@kaycebasques kaycebasques marked this pull request as ready for review July 22, 2024 23:30
@kaycebasques
Copy link
Contributor Author

@trallard thanks for the review, I updated the feature to jump directly to the main content container and made sure that the "skip to main content" node does not leak through when printing

#skiplink:focus {
top: 10px;
left: 10px;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For agogo and a few other built-in themes it's necessary to duplicate the CSS because they don't inherit basic.css. When that stylesheet is inherited we don't need the duplication.

@@ -51,7 +51,7 @@ <h2 class="heading"><span>{{ title|striptags|e }}</span></h2>
<div class="topnav" role="navigation" aria-label="Top">
{{ nav() }}
</div>
<div class="content" role="main">
<div class="content" role="main" id="main">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this theme and a few others we need to duplicate the node ID because they override the default HTML from basic a bit and the ID gets lost in the override.

@kaycebasques
Copy link
Contributor Author

hi @chrisjsewell this should be good to go now

Copy link

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thank you! I just did a quick pass and overall it looks good.
I can check the tab order more in detail tomorrow when I am back in front of a computer.

@trallard
Copy link

trallard commented Jul 31, 2024

@kaycebasques I was trying to check the tab order (building the documentation first) and it seems the skiplink is not working.

When inspecting the source I can the main id seems to be missing from <div class="body" role="main">
Further inspecting the outline of the webpage confirms the main target element is missing (see screenshot attached)

Sphinx_—Sphinx_documentation-_Polypane

Is this something you are also seeing on your end?

Note I am only starting to familiarise with sphinx codebase so it is likely I am missing something on how layout/styles/themes relate to each other so thought I would check first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "skip to main content" a11y feature
3 participants