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 contrast failures for high contrast light theme #33

Merged
merged 14 commits into from
Apr 4, 2024

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented Feb 5, 2024

Closes #30.

This PR updates the a11y-light and a11y-high-contrast-light themes.

For a11y-light, the main changes are:

  • Change highlight color from #e6e5d8 (this is the same as #7971292e blended with #fefefe) to #fdf2e2
  • Decrease the value in the HSV value of two colors in the palette to ensure that all of the colors meet 4.5 against #f2f2f2 (I chose this color as the darkest off-white or gray background color that the theme will support)
  • The end result is summarized in a contrast grid
color grid

For a11y-high-contrast-light:

color grid

Copy link

github-actions bot commented Feb 5, 2024

PR Preview Action v1.2.0
🚀 Deployed preview to https://Quansight-Labs.github.io/accessible-pygments/pr-preview/pr-33/
on branch gh-pages at 2024-03-28 17:01 UTC

@gabalafou gabalafou marked this pull request as draft February 5, 2024 03:03
@gabalafou gabalafou changed the title Fix contrast failures for high contrast light theme WIP - Fix contrast failures for high contrast light theme Feb 5, 2024
@gabalafou
Copy link
Contributor Author

I have some caveats with my work so far on this PR:

  • I'm not sure what exactly the "high contrast" in this theme is supposed to mean. The contrast ratios in the WCAG table for high contrast are almost the same as in the table for a11y-light.
  • I'm not sure what constraints went into creating a11y-high-contrast. For example, was there a constraint that the value for orange must be equal to the value for yellow?
  • Even though this PR fixes the contrast ratio for the text on an off-white background and for the text against a line highlight, there's still a contrast failure: between the background color of the code block (#fefefe on the demo site) and the background color of a highlighted line (#e6e4d9). WCAG guidelines require that the contrast ratio between those two to be 3:1, but they're actually 1.3:1. I suspect that it's not possible (or very hard) to find a color palette with 8 different text colors that each have to contrast at least 4.5 to two different background colors that have to contrast 3:1 to each other. Eric Bailey's solution was to put a top and bottom border on highlighted lines. We could do the same thing but we would have to modify the CSS that we get back from Pygments, which we already do in the generate_css() function in utils.py.

All foreground colors should work against #f2f2f2 (gray background)
and #ffff00 (line highlight)
@gabalafou
Copy link
Contributor Author

@trallard, I still have some misgivings about this PR.

Is it okay for us to so drastically change these themes now that we've published this repo?

@trallard
Copy link
Member

trallard commented Feb 7, 2024

I am not sure we are drastically changing the themes.
I mean - hues are being preserved as much as possible and tweaks are being made to improve the accessibility so I would not be too worried about this.

The only question I have is why the yellow in the light theme? I mean this is a much brighter one that the one we currently have

@gabalafou gabalafou marked this pull request as ready for review February 7, 2024 18:30
@gabalafou
Copy link
Contributor Author

Okay, I think this is ready for review

@gabalafou
Copy link
Contributor Author

gabalafou commented Feb 7, 2024

Note to self. I want to create some issues for this repo based on my experience of updating these two themes:

  • Updating a theme is cumbersome and error-prone (too many manual steps, no clear guide)
  • a11y-light has 7 foreground colors whereas a11y-high-contrast-light has 8
  • Line highlight color against background color does not meet WCAG (3:1)
  • Contributing instructions are incomplete (missing Hatch)

@trallard
Copy link
Member

Looking at the proposed light and high_contrast_light themes above my first impression was that now the colours in both looked different 🤔 so I attempted to make these two closer looking (using the perceptually uniform colour palettes we have)

a11y-light - see the corresponding contrast grid

EightShapes_Contrast_Grid

In the grid, you can see the current colours (with names) vs. the proposed ones (no names).
All the colours meet AA against the reference backgrounds.
I also added a magenta to match the high-contrast theme.

⚠️ You will also notice that I changed the black and grey colours.
Since the slat-gray in the high-contrast theme is the same as we are using in PST I thought we could use an alternative black (and shades) to keep this closer to the original theme's hue (and not directly use the PST theme colours)

See a11y-high-contrast-light and the corresponding contrast grid

EightShapes_Contrast_Grid

With these proposed changes, both themes are more consistent and we keep a higher contrast in the a11y-high-contrast-light theme vs the light one.

I can make the changes unless you are dying to make them @gabalafou or you have major reservations on the proposed changes/updates

@gabalafou
Copy link
Contributor Author

Putting this on hold while we get #42 through

@trallard
Copy link
Member

I was going to review again until I saw the last comment 😓 ping me when this is ready then

@Carreau
Copy link
Collaborator

Carreau commented Mar 28, 2024

#42 is in, this got a bunch of conflicts now. I think most of them should be solvable by just re-generating the files.

@gabalafou
Copy link
Contributor Author

I resolved some merge conflicts, but this is not yet ready for review

@gabalafou gabalafou marked this pull request as draft March 28, 2024 14:19
@gabalafou gabalafou marked this pull request as ready for review March 28, 2024 17:01
@gabalafou
Copy link
Contributor Author

OK this is now ready for review

@gabalafou
Copy link
Contributor Author

@Carreau thanks for approving this!

I think that @trallard should give this one final look and approval before I merge it in.

@Carreau
Copy link
Collaborator

Carreau commented Apr 3, 2024

Let's wait 48 hours and merge otherwise to have one less things on @trallard todo list, we can revise later if we wish.

@Carreau
Copy link
Collaborator

Carreau commented Apr 4, 2024

Seen with @trallard in person, merging.

@Carreau Carreau merged commit 9faf774 into main Apr 4, 2024
6 checks passed
@Carreau Carreau deleted the fix-a11y-high-contrast-light branch April 4, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a11y-light fails WCAG AA contrast with gray background
3 participants