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

WIP - ENH - Fix PyData brand colour contrast issues #1086

Closed
wants to merge 8 commits into from
Closed

WIP - ENH - Fix PyData brand colour contrast issues #1086

wants to merge 8 commits into from

Conversation

trallard
Copy link
Collaborator

@trallard trallard commented Dec 16, 2022

Done

This PR Closes #1052

For easy comparison, I have created a table with the corresponding colours and WCAG compliance, some things to note:

  • To ensure that text meets the colour contrast criteria of at least 4.5 for WCAG AA it was necessary to add light and dark variants to the primary and secondary colours (I used this instead of the alternative changes from @drammock of separating text and non-text colours - you mentioned wanting to keep the PyData colours for structural elements, but I believe this approach would add complexity in terms of having multiple slight variants of the colours as well as adding a more neutral theme. ) For reference see how the badges and buttons look now in the "Web components" section of the documentation in the image below, which I believe achieves what you were aiming for:
    Sphinx_Design_Components_—_PyData_Theme_0_12_1rc1_dev0_documentation

  • To ensure the .headerlink permalinks pass the contrast criteria, I had to change the opacity to 0.7

  • I noticed that the link hover colour was assigned from the warning colour, since I tweaked the PyData colours to ensure enough contrast without being too jarring I swapped the link hover to secondary. This also fixes the contrast issue for links in <code> elements.

  • The trickiest (and most tedious part) was checking the admonition colours (and those of elements with opacity), so I have another comparison table for you all.

    • I had to manually get the colours from the colours with an opacity different to 1 (rows in the table)
    • The only colours I made adjustments to are the warning and success ones to ensure there is enough contrast where needed (admonition icons and buttons meet at least 3:1)
    • Almost all the admonition the icons meet the WCAG contrast requirement of 3:1 for non-text elements and the title has enough contrast against the background (the only exception is the attention one but I added a note below)
  • Updates to the documentation to better reflect some changes and added some accessibility resources too (though it seems the Danger note in https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/styling.html#configure-pygments-theme needs updating , I am not entirely sure)

  • Also I made some adjustment to the dark-theme shadow as I noticed @drammock had these too in his WIP-commits

TODO

Contrast issues still present:

  1. dark-theme - link colour on announcement banner (currently 4.33 between #05AEC2 and #152a4a
  2. light-theme - link colour on announcement banner (currently 3.61 between #098292 and #d8e6fe
  3. the buttons text (see image above) do not have enough contrast against the background in both themes (it seems the function that handles the text-color needs some adjustments)
    Sphinx_Design_Components_—_PyData_Theme_0_12_1rc1_dev0_documentation
  4. Not enough colour contrast for code elements on the sidebar for example:
    Sphinx_Design_Components_—_PyData_Theme_0_12_1rc1_dev0_documentation
  5. Need to make a slight tweak to the attention colour for the light theme as currently, the contrast is 2.4:1 and it needs to be 3:1 for non-text elements (like admonition icons)

Perhaps 3 and 4 need to be done separately to avoid too many changes in this PR and would be best suited as part of #1079 or other PRs aimed at improving the theme as a whole.

A review in the meantime would be appreciated.

Suggested links to look at

added by @choldgraf

This PR Dev
Admonitions Admonitions
Generic items Generic items
Sphinx design components Sphinx design components

@drammock
Copy link
Collaborator

I have created a table with the corresponding colours and WCAG compliance

That is super helpful! Thanks. Are we supposed to be picking between the light/dark "secondary" and the light/dark "secondary-alt"? Which one was used in the screenshot of the badges above?

it was necessary to add light and dark variants to the primary and secondary colours

I think this is a better solution than what I had proposed (having primary + primary_text and secondary + secondary_text).

to ensure the .headerlink permalinks pass the contrast criteria, I had to change the opacity to 0.7

no objection.

  • I swapped the link hover to secondary.

good; logically it makes more sense to me anyway to have link=primary and link-hover=secondary.

This table was harder for me to follow, so I regrouped the rows/columns, which made me notice that there is no row for "success-light". It also made me realize that for all those 0.1-opacity colors, all we really need to support is either dark text (in light mode) or light text (in dark mode), as on this page from the PR build, and maybe also code spans (I don't think you can have links in an admonition title, but maybe we should check that too to be safe). So in summary we need to check "text-base" (normal text), "primary" (links), "secondary" (link hovers) and "inline-code" colors on the 0.1-opacity semantic colors. I've done that in this edited table and it looks good (apart from the missing success-light row).

it seems the Danger note in https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/styling.html#configure-pygments-theme needs updating, I am not entirely sure

I think what it should say is that we ignore/don't respect the sphinx setting. Or perhaps just change "overwritten" to "overridden"?

I made some adjustment to the dark-theme shadow

awesome, it looks good on the PR build

it seems the function that handles the text-color needs some adjustments

Hmm yeah, here's another table showing the badge/button issue. Black text is fine on any admonition color badge (as you've defined them), but white text is not. Perhaps we should ditch that function and always use black text? I guess the risk there is that when downstream users tweak the admonition colors it might cause bad contrast. But if we make it really legible to start off, maybe they'll be less likely to tweak them. 🤔

@choldgraf
Copy link
Collaborator

choldgraf commented Dec 17, 2022

Thanks for putting all of this together @trallard. In general this looks good to me - thanks for all of the contextual links, those are very helpful and I love that color matrix thingy. A few quick thoughts / proposals from me:

I find the primary color harder to distinguish links. I know there's a tension here because text is black, and the background is white, and we need a color that is easily distinguishable from both. But I just wanted to note that I find it harder to quickly parse the links within text because of the darker shade of our primary color. I don't think it's a dealbreaker, but maybe there is some best-practice about this out there?

And two related suggestions that we remove color styling and instead just go with black/white. I don't think either of these should block this PR if there isn't general agreement, but my feeling is that by removing these colors we're less-opinionated while still following common practices, and also by using black/white we run less risk of choosing the wrong color from an a11y standpoint.

Remove the primary color from the section headers. I think that the darker color makes sense from an a11y perspective, but I feel that it doesn't look as good with "big banner" status because it is "dark but not quite black". I looked at the other themes in our inspiration section, and don't believe any of them use a color for their section headers (e.g., Docusaurus, Furo, GitBook, and GitHub does this too). Instead they denote section headers with a larger font size and a bold weight. I'd propose that we do the same thing here.

Remove the inline code highlighting color. I think the change to in-line code highlighting is OK here, but IMO it would be simpler if we just kept in-line code as black with a grey boxed background, rather than giving it a different color. This also matches what GitBook, Docusaurus, Furo, and GitHub do. If I recall, the original intention for this was to distinguish in-line code from surrounding text. However, that was before we included the greyed box background on top of it. I think the combination of the greyed box text + the change in font style makes it pretty easy to distinguish.

@trallard
Copy link
Collaborator Author

trallard commented Dec 17, 2022

Adding a couple more thoughts as I was planning to open a separate PR for links.
I checked the contrast between primary colour and background and primary colour and surrounding text. They both meet 4.5:1 or greater which is the WCAG criteria. I can try and bump the luminance of the light teal to bump up the contrast without affecting contrast criteria conformance.
The difficulty parsing might be due to perceived contrast (which is not accounted for in WCAG 2 but might be in WCAG 3 with the APCA criteria but this is still in draft and would not meet accessibility requirements).
The second part - to be WCAG conformant we need to meet the "not rely solely on colour" SC 1.4.1 and as best practice links should be presented with an underline.
But I can add this here too so we meet these two criteria out of the box.

As per headings colour - I agree - most documentation engines use a darker/lighter shade of the text colour rather than an accent and bold font.
But in the spirit of keeping PRs focused it might be best to discuss this as part of the "neutral theme" discussion WDYT?
I find that in general the pure white and pure black (or almost pure black) backgrounds could be improved by finding shades that would provide a softer perceived contrast and elevation throughout, but again I would prefer having this in a separate discussion and keep this PR more on the "quick fixes without too many big changes".

As far as removing colours from links or inline code - while the surrounding box would provide context along with the monospace font. I believe keeping distinctive colours is useful from a cognitive point of view and especially helpful for neurodiverse folks and to help with quick scan ability in general. So I'd vote for keeping these or work towards a grey (box/text) colour combination that obviates the inline code and has enough contrast from surrounding text and background.

@choldgraf
Copy link
Collaborator

Makes sense to me - I opened two issues to track discussion and/or design decisions on the two points I raised:

I suggest that if there is clear consensus on those issues, we can implement them as part of this PR, but we shouldn't block this PR on them. Does that make sense?

The second part - to be WCAG conformant we need to meet the "not rely solely on colour" SC 1.4.1 and as best practice links should be presented with an underline.

+1 - would also be helpful to link these in the contributing docs so we know where these guidelines came from!

@choldgraf
Copy link
Collaborator

choldgraf commented Dec 19, 2022

I came across a nice blogpost from GitLab that outlines their rationale behind a recent color revamp they did in 2018. It sounds like they were dealing with several of the same issues that we are as well, so maybe it's useful for inspiration or helping us make decisions (they even have a few "admonition and button"-style colors that they had to choose from)

@trallard
Copy link
Collaborator Author

Hey @choldgraf I am going to update/change #1079 to add some overall design thoughts as I was looking into this precisely over the weekend.

@trallard
Copy link
Collaborator Author

I am going to close this PR as this will be superseded by work following #1079

@trallard trallard closed this Jan 12, 2023
@trallard trallard deleted the trallard/accessible-brand-colours branch July 3, 2023 09:56
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.

Accessibility Colour Contrast Issues
3 participants