Skip to content

Accessibility - Fix code syntax highlighting contrast errors (dark mode) - #34484

Merged
betatim merged 1 commit into
scikit-learn:mainfrom
DeaMariaLeon:dark-theme
Jul 21, 2026
Merged

Accessibility - Fix code syntax highlighting contrast errors (dark mode)#34484
betatim merged 1 commit into
scikit-learn:mainfrom
DeaMariaLeon:dark-theme

Conversation

@DeaMariaLeon

@DeaMariaLeon DeaMariaLeon commented Jul 15, 2026

Copy link
Copy Markdown
Member

Reference Issues/PRs

Continuation of #34376 (comment)

Towards: #34366

What does this implement/fix? Explain your changes.

Changing the background colour of the code syntax highlighting when using dark mode.
The contrast accessibility errors in the code are now gone.

I used FireFox to test. I couldn't find an online tool that tested in dark mode.

It's equivalent to #34376.

First time contributor introduction

AI usage disclosure

Any other comments?

This PR changes from this (just see the code block please):
Screenshot 2026-07-15 at 18 39 33

To:

Screenshot 2026-07-15 at 18 41 11

@DeaMariaLeon
DeaMariaLeon marked this pull request as ready for review July 15, 2026 16:51
@DeaMariaLeon DeaMariaLeon changed the title Accessibility - Fix code syntax highlighting errors (dark mode) Accessibility - Fix code syntax highlighting contrast errors (dark mode) Jul 15, 2026
@DeaMariaLeon DeaMariaLeon moved this to PR waiting for reviews in Labs Jul 16, 2026
@DeaMariaLeon DeaMariaLeon added the Quick Review For PRs that are quick to review label Jul 17, 2026

@cakedev0 cakedev0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes LGTM, and need/reason of the change was explained live during a sync.

And I also tested on my browser before/after.

@cakedev0 cakedev0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(I wanted to approve, sorry for the noise)

@DeaMariaLeon

Copy link
Copy Markdown
Member Author

Thank you!

@DeaMariaLeon DeaMariaLeon added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jul 20, 2026
@lesteve

lesteve commented Jul 21, 2026

Copy link
Copy Markdown
Member

As mentioned during the meeting, in an ideal world this would be taken care of by the pydata-sphinx-theme and benefit plenty of other projects' documentation, rather than us having to resort to what feels like hacks 😅.

Out of curiosity, Do you have a reference for why this is complicated to do at the pydata-sphinx-theme as you mentioned during the meeting.

If there is no related issue in pydata-sphinx-theme, I am thinking you could create one as well ...

@betatim betatim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd be happy to have these changes via custom.css and, as Loic pointed out, see if they can be upstreamed so that more people benefit from these improvements.

@DeaMariaLeon

DeaMariaLeon commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

Do you have a reference for why this is complicated to do at the pydata-sphinx-theme as you mentioned during the meeting.

This is what's going on:

  1. Pygments' monokai theme only styles the outer div, not the <pre> inside it
  2. pydata-sphinx-theme ships a rule for every <pre> element on the page (used for things like doctest blocks and raw preformatted text, not just syntax-highlighted code)
  3. pydata-sphinx-theme overrides Pygments monokai because we have:
  4. nested: <div class="highlight"><pre>...</pre></div>

In the screenshot bellow "background-color: var(--pst-color-surface)" is what sets the background I'm trying to change.

Screenshot 2026-07-21 at 12 59 13

For more details:

The <pre> HTML element represents preformatted text which is to be presented exactly as written in the HTML file.

So, an issue to pydata-sphinx-theme may not be the solution.

Out of curiosity, Do you have a reference for why this is complicated to do at the pydata-sphinx-theme as you mentioned during the meeting.

I don't know if I should share more details @lesteve, as during the call you also asked why this happens and after I talked you replied "who cares?". Also those were "too many details".
It's not a reproach and I want to think that it was a joke. But I would like have the right balance between sharing too many details and not being specific enough (some one else complained about it).

EDIT: I corrected the link to <pre> I shared above.

@betatim

betatim commented Jul 21, 2026

Copy link
Copy Markdown
Member

Even if we can't come with a solution that would work for pydata-sphinx-theme directly it would be worth to open an issue pointing out that the syntax highlighting could be improved to make it more accessible.

@betatim

betatim commented Jul 21, 2026

Copy link
Copy Markdown
Member

I'll merge this as it looks like there is agreement that we should fix this and that this PR implements one way to fix it. The discussion about opening an upstream issue (or not) can continue even if this is merged.

@betatim
betatim merged commit 4c710ce into scikit-learn:main Jul 21, 2026
62 of 63 checks passed
@github-project-automation github-project-automation Bot moved this from PR waiting for reviews to Done in Labs Jul 21, 2026
@DeaMariaLeon
DeaMariaLeon deleted the dark-theme branch July 21, 2026 11:41
@DeaMariaLeon

Copy link
Copy Markdown
Member Author

Even if we can't come with a solution that would work for pydata-sphinx-theme directly it would be worth to open an issue pointing out that the syntax highlighting could be improved to make it more accessible.

True. It occurred to me only after I sent my previous comment.

Thanks a lot!

@lesteve

lesteve commented Jul 21, 2026

Copy link
Copy Markdown
Member

Thanks a lot for the details! I would suggest you create an issue explaining the problem in the pydata-sphinx-theme repo so hopefully they or maybe someone else (maybe @DeaMariaLeon even one days who knows what the future holds 😉) can fix it.

@DeaMariaLeon

Copy link
Copy Markdown
Member Author

BTW: these themes are already accessible in case we need/want to switch from pydata-sphinx-theme: https://github.com/Quansight-Labs/accessible-pygments

I would need to build the docs locally to check that they pass the contrast checks.

@lesteve

lesteve commented Jul 21, 2026

Copy link
Copy Markdown
Member

I don't understand the details, but we are already using pydata-sphinx-theme are we not? So you are saying that using accessible-pygments as a dependency should be the thing that we need to do to benefit from more accessibility fixes?

If that's the case there are two things:

  • build the doc locally to make sure they pass the contrast checks, it looks like this is matter of installing the right packages in your env before building?
  • figure out with potentially some help on how to use build_tools/update_environment_and_lock_files.py to update the doc build to contain accessibility-pygments.

@DeaMariaLeon

DeaMariaLeon commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

So you are saying that using accessible-pygments as a dependency should be the thing that we need to do to benefit from more accessibility fixes?

Not quite. I think they are just different themes, and it may be much simpler to install them.

I found out about those themes after the fix for the light theme was already done.
At the time, I thought that we would still have contrast issues. Then I forgot about them and remembered today -- after this PR was merged. 🫣

EDIT: I need to check my assumptions above (that it may be much simpler to install).

@lesteve

lesteve commented Jul 22, 2026

Copy link
Copy Markdown
Member

I am pretty sure the end goal is to improve accessibility in pydata-sphinx-theme, this is part of the NASA ROSES grant.

@DeaMariaLeon

Copy link
Copy Markdown
Member Author

figure out with potentially some help on how to use build_tools/update_environment_and_lock_files.py to update the doc build to contain accessibility-pygments.

We already have the dependency:

build_tools/circle/doc_linux-64_conda.lock:207:
https://conda.anaconda.org/conda-forge/noarch/accessible-pygments-0.0.5-pyhd8ed1ab_1.conda#74ac5069774cdbc53910ec4d631a3999

build_tools/circle/doc_min_dependencies_linux-64_conda.lock:193:
https://conda.anaconda.org/conda-forge/noarch/accessible-pygments-0.0.5-pyhd8ed1ab_1.conda#74ac5069774cdbc53910ec4d631a3999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend No Changelog Needed Quick Review For PRs that are quick to review Waiting for Second Reviewer First reviewer is done, need a second one!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants