Skip to content

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented May 8, 2025

Fixes #2199.

This should also:

  • fix the a11y-tests CI check which is currently failing on all new PRs
  • fix a warning that occurs whenever Sphinx >= 7.3 is used
  • fix a warning that occurs when the latest/dev version of Sphinx is used

How to test

Run a contrast checker on the following preview build pages.

Check each page both in light mode and dark mode:

  1. Read the Docs - PR 2204 - Kitchen Sink - Tables - specifically check "list tables"
  2. Read the Docs - PR 2204 - Kitchen Sink - Admonitions - specifically check the following five admonitions: topic, version added, version changed, deprecated and version removed

Screenshots

The following two screenshots, one for light mode and one for dark mode, show the topic admonition (with higher contrast link) above a regular admonition to show the difference between the regular contrast link color versus the higher contrast link color.

light mode topic admonition shown next to normal admonition dark mode topic admonition shown next to normal admonition

The following two screenshots, one for light mode and one for dark mode, show the four different version-related admonitions, which use higher contrast links, below a warning admonition, which uses regular contrast.

five light mode admonitions shown together, four of which have higher contrast links

Note that in the version changed admonition (dark mode), the link color is even higher contrast (teal-300) than the other higher contrast links (teal-400).

five dark mode admonitions shown together, four of which have higher contrast links

The following two screenshots show the same table in light and dark mode (all of the links use the higher contrast color).

light mode table with links dark mode table with links

@gabalafou gabalafou marked this pull request as ready for review May 8, 2025 08:36
@gabalafou gabalafou requested a review from trallard May 8, 2025 08:37
@gabalafou
Copy link
Collaborator Author

I think all of the test failures are unrelated to this PR except maybe one, docs-checks / build PST docs (ubuntu-latest, 3.9, 6.1) (pull_request), which contains the following in the log output:

kitchen-sink/admonitions.rst:161: ERROR: Unknown directive type "versionremoved".

.. versionremoved:: v0.1.1

   Here's a version removed message.

   We also support *italic*, **bold**, ``code``, `links <https://www.sphinx-doc.org/en/master/>`_, and more.

I'm not sure why versionremoved produces an "unknown directive type" error.

@gabalafou gabalafou added tag: accessibility Issues related to accessibility issues or efforts tag: CSS CSS and SCSS related issues labels May 8, 2025
@gabalafou gabalafou added this to the 0.17.0 milestone May 8, 2025
@gabalafou gabalafou added impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship kind: bug Something isn't working impact: high Something that is relevant to nearly all users and removed impact: block-release Should block a release from happening. Only use if this is a critical problem we don't want to ship labels May 8, 2025
@gabalafou
Copy link
Collaborator Author

I have labeled this PR as high impact for two reasons:

  1. Every PR created until this gets merged will have a failing CI check for accessibility
  2. We say that our goal is to conform with WCAG 2.1 Level AA

@trallard
Copy link
Collaborator

trallard commented May 8, 2025

I think all of the test failures are unrelated to this PR except maybe one, docs-checks / build PST docs (ubuntu-latest, 3.9, 6.1) (pull_request), which contains the following in the log output:

Do you mean all a11y-tests related failures? I am not sure, but since we have had the failures on our CI for a while it has been hard to track new failures.
Could you do a quick check on other runs of the workflow just to rule out this being introduced here?

@gabalafou
Copy link
Collaborator Author

I'm re-running the workflows and checking

Tests against the latest/dev Sphinx version have been seeing:

> WARNING: The config value `linkcheck_allowed_redirects' has type `NoneType'; expected `dict'.
@gabalafou gabalafou changed the title Increase link contrast where it fails WCAG AA Fix three separate test failures (a11y, Sphinx dev, Sphinx 7.3) May 8, 2025
@trallard
Copy link
Collaborator

trallard commented May 8, 2025

Nice, thanks @gabalafou
Seems now we only have the actual link_check failure (gettext has been a problem from the beginning, so I might skip them altogether).

And the new lighthouse one (ref GoogleChrome/lighthouse#16404) that we can address separately

trallard
trallard previously approved these changes May 8, 2025
Copy link
Collaborator

@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.

Thanks @gabalafou this looks good to me, just left one comment that is not a merge-blocker, so will leave it to you to decide if you will leave as is or change.

@gabalafou
Copy link
Collaborator Author

And the new lighthouse one (ref GoogleChrome/lighthouse#16404) that we can address separately

I think the Lighthouse failures are the result of a combination of two factors. One factor is that our Lighthouse test site is not loading and applying our CSS to the test pages. I think that's probably been true for some time. And then more recently a new check was added to Lighthouse, which you referenced.

Comment on lines -104 to -105
cp -r docs/examples/kitchen-sink $docs_dir/site/kitchen-sink
printf "Test\n====\n\n.. toctree::\n\n kitchen-sink/index\n" > $docs_dir/site/index.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting that this was working without any globbing before.
Good catch tho.

@trallard trallard merged commit 441c98d into pydata:main May 12, 2025
29 of 31 checks passed
@gabalafou gabalafou deleted the a11y-fix-link-color branch May 13, 2025 11:42
trallard added a commit that referenced this pull request May 28, 2025
This PR adds the following:

- Adds missing traces from
#2175
- Updates the `docs` workflow pinned SHA following
#2204 (this should be
the last change we need to fix the `publish` workflow and have nightly
releases back up again)
- Updates the `regex` to skip URL checks for `gnu` URLS

Closes #2175

---------

Co-authored-by: Tania Allard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: high Something that is relevant to nearly all users kind: bug Something isn't working tag: accessibility Issues related to accessibility issues or efforts tag: CSS CSS and SCSS related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a11y - Colour contrast issues (hyperlinks)
3 participants