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

Theme accessibility #3392

Open
not-my-profile opened this issue Nov 5, 2021 · 23 comments
Open

Theme accessibility #3392

not-my-profile opened this issue Nov 5, 2021 · 23 comments
Milestone

Comments

@not-my-profile
Copy link

not-my-profile commented Nov 5, 2021

Web accessibility is very important. For a syntax highlighting library this is particularly relevant when it's used for a website that is meant to be used by many users, some of whom may not have perfect vision and struggle to read code highlighted with themes that have a low contrast. The WCAG web accessibility standard defines the following AA success criteria:

The visual presentation of text and images of text has a contrast ratio of at least 4.5:1

Currently highlight.js already comes with several themes that meet this criteria. However most themes that come with highlight.js do not meet WCAG AA. I don't think that this is a problem per se, since highlight.js might be used just for e.g. just a personal application where the user chooses can choose their own color theme as they wish.

I do however think that highlight.js should address two things:

  • If highlight.js comes with themes that do not meet WCAG AA, the Demo should raise awareness about which themes have a high contrast and which themes don't. This could be as simple as dividing the themes list in Themes with high contrast, Themes with lower contrast and linking some informative web page that explains when/why this matters.

  • I find it problematic that the Default theme does not meet WCAG AA. I think either it's colors should be changed to meet WCAG AA or it should be renamed to something else (maybe Legacy Default). This is important because accessibility by default is important. (And also because the README currently suggests using default.css).

To get this started I opened the pull request #3390 which calculates the minimum WCAG contrasts for each theme and asserts that they are the same as defined in test/contrast/min_contrasts.json. (Contrasts can still be decreased but not accidentally).

@joshgoebel
Copy link
Member

joshgoebel commented Nov 5, 2021

I find it problematic that the Default theme does not meet WCAG AA.

This seems like a valid criticism. I don't think we can majorly change our default theme in a minor release, but if it would only require some smaller contrast changes I could imagine slipstreaming into the next v11 release.

How far off are we exactly currently?

@not-my-profile
Copy link
Author

not-my-profile commented Nov 5, 2021

The worst contrast of the default theme currently has a ratio of 2.4:1 (as you can tell by looking at the JSON file of my PR). Contrasts can be easily improved with Chrome's developer tools (which let you snap colors to the most similar color that fulfills WCAG AA). To tell how different that looks we'd have to try.

@joshgoebel
Copy link
Member

joshgoebel commented Nov 5, 2021

@not-my-profile Have you visited the https://prismjs.com project yet? If you have an issue open over there (or on any other code highlighters) I'd love a link. In some quick playing around it seems 4.5:1 is actually pretty hard to hit in some cases without severely limiting color selection (I was playing around with our default theme). Before you brought it up I would not have guessed that "most themes that come with highlight.js do not meet WCAG AA"... I'd be very curious to hear some other highlighters perspectives on WCAG AA accessibility and how the standard should be applied to code highlighting (as a sub-genre of text).

Have you done any study of the full Base16 themes I wonder? They should be trivial for you to analyze. I wonder how many of those would be WCAG AA. Perhaps another project for you to look at next that could have a large impact (on many sites, editors, etc) - if you got the theme designers to care.

I think your laser focused bullet points here were good. Having a more accessible default theme certainly can't hurt - users are after all always free to discard it if they don't like it - so it forces no ones hand. I'll make an issue to tighten it up a bit for the next release, though perhaps not fully to 4.5:1... I've added a TODO for making both our light and dark default themes WCAG AA compliant to for version 12. #3219

the Demo should raise awareness about which themes have a high contrast and which themes don't

I'm thinking this could quite likely be 100% done on the client-side be analyzing the colors real-time when a theme is selected so the UI could say "high contrast" or "low contrast" or even show the range... that doesn't "divide" anything but ti does make it immediately clear at a glance what the contract of an individual theme is.

Side note: It's also impossible for us to know the size (or weight, or font) the code is ultimately going to be rendered at in the wild (our CSS doesn't force size), which obviously provides some wiggle room I think - this is obviously a "spectrum" style issue in the real world (where the themes are used) - it's impossible to look at our CSS alone and pass/fail it - you'd need to also know how it was deployed (font size, weight, font itself, etc)... I'm not sure WCAG takes all those things into consideration but obviously it's clear they all impact accessibility with regards to contrast and readability.

I say that only to point out that any contrast numbers calculate off of just the colors alone are going to be subject to some wiggle room...

and linking some informative web page that explains when/why this matters.

I'm not opposed to a small blurb in the README (or link in the demo), but I ultimately this is an implementors issue (and always has been) - doubly so if we implement the two bullets points you suggest above.


So if your goal here is having some immediate impact:

  • I've just created Improve accessibility (slightly) of default light and dark themes #3393 for improving our default themes, which should land in the next release or two, even if it's not "perfect" WCAG AA. This will improve accessibility for anyone who version bumps and uses our default themes.
  • How about providing a suggested blurb for the README about theme accessibility... (or a PR with just that single doc fix)
  • I'll still circle back to your other PR, but right now I'd be a lot more open to doing this high/low contrast analysis on the client-side in the Demo live (as suggested above). IE, simply telling people when clicking thru random themes how accessible the theme they are looking at may or may not be.

@not-my-profile
Copy link
Author

not-my-profile commented Nov 5, 2021

If you have an issue open over there (or on any other code highlighters) I'd love a link.

I have previously raised the same issue with Pygments (pygments/pygments#1718) and they have been very responsive.

In some quick playing around it seems 4.5:1 is actually pretty hard to hit in some cases without severely limiting color selection (I was playing around with our default theme).

As I mentioned with the Chrome dev tools getting WCAG compliance is trivial, you just inspect the offending text and click a button. If that results in two different style colors looking too similar you'll have to change them, but it's really not that hard.

Have you done any study of the full Base16 themes I wonder?

No, I haven't.

I've added a TODO for making both our light and dark default themes WCAG AA compliant to for version 12.

Thanks!

I'm thinking this could quite likely be 100% done on the client-side be analyzing the colors real-time when a theme is selected so the UI could say "high contrast" or "low contrast" or even show the range... that doesn't "divide" anything but ti does make it immediately clear at a glance what the contract of an individual theme is.

You have 242 styles. I really don't think that users should have to click through each of them to find the ones that have a high contrast. I believe highlight.js should make it as easy as possible for implementors to pick an accessible style (if they choose to care about that). (Hence my PR).

It's also impossible for us to know the size (or weight, or font) the code is ultimately going to be rendered at in the wild (our CSS doesn't force size)

Yes for 18 point large text (that's 24px) a contrast ratio of 3:1 suffices to meet WCAG AA. In practice that doesn't apply to most highlighted code because:

image

How about providing a suggested blurb for the README about theme accessibility

Sure that wouldn't hurt. I still think that showing that information on the Demo is more important since any note in the README can be easily missed.

@joshgoebel
Copy link
Member

I wrote a small Python script to check how Pygments styles fare when judged according to the WCAG contrast standard.

Was this not possible with our CSS because of it's complexity? Just posting a simple list like you posted on the Pygments thread would have been super useful (to kickstart the discussion) and sounds like it might have required far less of your time than your original PR.

you just inspect the offending text and click a button

Yes, the key part was "without severely limiting color selection"... I was playing with another tool and it was easy to move the slider but very quickly all the colors turn VERY dark... it seems a very limited dynamic range that fits in 4.5:1... (when your BG is white/light grey)

[Re: Base16 themes] No, I haven't.

Perhaps you'd point your script at them? Here are ones built with our template:

https://github.com/highlightjs/base16-highlightjs/tree/main/themes

These include only #000000 style html colors I believe, so I think you should find them easy to parse... Would be curious to see the results if it's easy for you to get them.

You have 242 styles. I really don't think that users should have to click through each of them to find the ones that have a high contrast. I believe highlight.js should make it as easy as possible for implementors

I take your point. I think we're coming at this from very different perspectives. I imagine most implementors would find a theme they liked first visually - which kind of requires "clicking thru them all"... then perhaps they check how accessible it is... perhaps they'd be happy with 4.3:1... ...where-as you imagine someone saying "I deeply care about accessibility, show me only high contrast". So from my perspective it sounds a little less ridiculous. :-)

If we're going to surface this in Demo I think we we should calculate this during the Demo build process (with a smart analysis tool that doesn't force CSS rules on us and actually examines the final built CSS, not the raw source), not during CI.

Yes for 18 point large text (that's 24px) a contrast ratio of 3:1 suffices to meet WCAG AA. In practice that doesn't apply to most highlighted code because:

I was just pointing out it's technically a "sliding scale" as the WCAG even seems to acknowledge: https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html#dfn-large-scale The sudden jump from 3 to 4.5 is a bit well, sudden... it'd be more realistic to scale the contrast ratio with the text size... The point of a standard is having a standard, so given an exact example you can indeed say pass/fail... but I'd suggest that if we're being honest then 23px large text that's 3.2:1 is probably within the "spirit" of the standard, even if not the letter. :-)

@joshgoebel
Copy link
Member

joshgoebel commented Nov 5, 2021

Ok here is what I whipped up really fast on top of our existing theme checker: #3394

Screen Shot 2021-11-05 at 6 06 53 AM

It's not perfect (ie, dealing with all the CSS variations) yet, but even so it's super useful stats... and this feels much more in line with the type of tooling we'd prefer to deploy. It'd be pretty easy to break this out into a tiny analysis lib that just takes the CSS as input... and then provides a stats object with best/worst/median/etc... which could be directly plugged into the Demo builder.

So... given that this seems easy enough to iterate on... the next step would be decided how Demo should leverage this information.

@joshgoebel
Copy link
Member

joshgoebel commented Nov 5, 2021

@Hirse You've played with the Demo stuff a bit I think... any thoughts on UI for this? We need a segmented list or toggle or filter or something... Is it just a dichotomy we want to offer- high vs low contrast? Or perhaps we just leave it as is but add a tiny WCAG badge next to "winning" themes... though I imagine the original requestor here would prefer to see them grouped?

@not-my-profile Did you have any specific ideas of how to surface this on Demo? Should I assume you think showing the actual WCAG numbers is important? If so, which? Worst? Average? Median?

I'm still also struggling with which exact numbers to use for "grouping"... to me a theme isn't bad just because a single rarely-used scope is a bit low contrast if 99% of the text is still high contrast. That's something I think we should make an effort to take into account when segmenting themes. If someone wants to be a stickler about it - that's a good reason to surface the actual WCAG numbers somewhere.

I think we should weight plain text and comments higher than other scopes. Plain text is going to be the majority and faded comments are likely to be the worst abuse in many themes.

@Hirse
Copy link
Contributor

Hirse commented Nov 6, 2021

Thanks @not-my-profile for starting this conversation. I have been thinking of theme accessibility before, but never got started.

@joshgoebel, I was thinking of exposing accessibility classification (and light/dark status) as icons in the theme list. Here's a quick example using emoji where the wheelchair stands for AA/contrast>=4.5 and the guide dog for AAA/contrast>=7.
image
The actual contrast value is available as a tooltip.
image
We could then add a toggle to only show accessible themes.
image
(I would like to find better symbols, preferably something like image, but unicode is just so easy for testing.)

For classification, I used the values calculated in #3390. Generally, I think we should be strict and use the min-value when using the calculation in #3394.

@not-my-profile
Copy link
Author

not-my-profile commented Nov 6, 2021

I agree with @Hirse that you should be strict.

to me a theme isn't bad just because a single rarely-used scope is a bit low contrast if 99% of the text is still high contrast. [..] If someone wants to be a stickler about it [..]

There are certain service sectors where websites are legally required to meet WCAG AA. It's also not uncommon for WCAG AA compliance to be part of a contract for commissioned work. 99% does not cut it in those cases.

I don't think you should display a 🚫 for each stye that doesn't meet AA. I think it makes more sense to display a green AA badge or a green AAA badge to the right of theme names for themes that meet that criteria.

@joshgoebel
Copy link
Member

joshgoebel commented Nov 6, 2021

I don't think you should display a 🚫 for each stye that doesn't meet AA.

Agreed, this isn't a public shaming.

There are certain service sectors where websites are legally required to meet WCAG AA.

I'm not suggesting we should misrepresent whether a theme is WCAG AA or not, but we could have our own designation too... so themes could be high contrast (in our humble opinion), AA, or AAA... but no use in arguing about that until we see what the actual breakdown looks like in the first place. :)


Yeah, maybe instead of icons just a little <span>AA</span> style pills could work... I think that would be an easy start.

@joshgoebel
Copy link
Member

I'm not sure I have time to push this to the home plate fully right now but I'll try to finish up the "rough shape" of things and then update my PR and someone else could pick it up and run with it the rest of the way or it could wait until I have time to circle back.

@Hirse Maybe you'd be interested, all you'd need to do is work on the client-side display/filtering pieces... should be pretty simple. I think we could have the same "pills" (vs icons) beside the title and those serve as button "selectors" to filter for just themes matching that criterion ...

Now I'm thinking we could have "Dark" and "Light" filters also since that out to be easy to do while we're analyzing the colors, but we'll see. :-)

@joshgoebel
Copy link
Member

A beginning. Themes in the Demo now have badges. I updated my PR.

Screen Shot 2021-11-06 at 9 33 27 AM

@joshgoebel
Copy link
Member

joshgoebel commented Nov 6, 2021

@Hirse The whole A display:block will have to be revisited though. :-) Let me know if you might be interested in doing the front-end UI work on this (filtering, etc).

If we're going to include a small blurb or link to a page on accessibility we should decide where to add that... I think it's fair to have a tiny call out that explains what the AA and AAA badges mean and that's where we'd include this.

@joshgoebel
Copy link
Member

Actually labeling every theme light or dark (right beside it) might be a bit much be we could decide to hide some badges and still use them for filtering... since filtering by light dark would be nice for someone who has ONLY interest in one or the other.

@Hirse
Copy link
Contributor

Hirse commented Nov 7, 2021

@joshgoebel Happy to work on the front-end. :)
I tried your badges, but it indeed gets quite busy. There's also an issue with badges wrapping to the next line:
image

Since every theme is either light or dark, I also tried adding a marker in the front, but I'm not sure the icon is clear enough:
image

@not-my-profile
Copy link
Author

not-my-profile commented Nov 7, 2021

How about dividing themes by light and dark?

Light themes

  • Theme 1
  • Theme 2
  • Theme 3

Dark themes

  • Theme 4
  • Theme 5
  • Theme 6

(but still having both in the same scrolling container). Because I do think displaying two auxiliary information flags for each theme (light/dark + accessibility) is a bit too much information overload.

@joshgoebel
Copy link
Member

I feel like that only works if they are collapsible or we have some sort of table of contents. Or maybe I'm just imagining what the UI would look like. It would have to be clear that if you scroll they were dark themes below. 

That doesn't necessarily solve the badge floating onto the next line problem. You could solve that with a nonbreaking space but then you might end up where the badges are so far to the right that they can't be seen at all. 

@not-my-profile
Copy link
Author

not-my-profile commented Nov 8, 2021

Ok on second thought I think that filtering is the way to go (as opposed to always separating light and dark). Since the sidebar already feels overloaded with the two scrolling lists, may I suggest moving the language categories from the sidebar to an absolutely positioned horizontal tab list on top of the main area? Then we could easily add the theme filters above the theme list.

highlight-js

Also note that I think we should make the .current styling way more prominent.

Also also while you're at revamping the demo, how about making the current selection sharable by sharing the URL? That is updating the location.hash each time you change your selection (e.g. #default/common)?

@joshgoebel
Copy link
Member

joshgoebel commented Nov 9, 2021

how about making the current selection sharable by sharing the URL?

I don't see sharing as a common use case here...

Ok on second thought I think that filtering is the way to go

The filtering stuff looks nice... I'm not convinced any of this requires moving the language filters to the right side though... filtering could easily be added under "Themes" as it stands now. Though I wonder if Light/Dark should just be a dropdown like with contrast...

may I suggest moving the language categories from the sidebar to an absolutely positioned horizontal tab list on top of the main area

Also worth saying I'm not 100% opposed to the idea, but how it's done in the mock-up above isn't catching my fancy at all. I'd suggest we focus on just getting the themes filtering right first rather than turning this into "redesign the whole demo site"... :-)

I think we should make the .current styling way more prominent.

No disagreement here. And then we don't have to align that silly arrow anymore. :-)

@not-my-profile
Copy link
Author

Seeing that https://highlightjs.org/ still does not raise awareness about accessibility, maybe you can take some inspiration from https://pygments.org/ for which I have since implemented such changes.

  • https://pygments.org/styles/ now lists styles with a good contrast first and styles with a lower contrast only after the paragraph:

    The following styles do not meet the WCAG 2.1 AA contrast minimum, so they might be difficult to read for people with suboptimal vision. If you want your highlighted code to be well readable for other people, you should use one of the earlier styles instead.

  • https://pygments.org/demo/ separates the styles in the <select> into two <optgroup> (good contrast & suboptimal contrast) and selecting a style with a suboptimal contrast shows the warning "style may have poor contrast".

@joshgoebel
Copy link
Member

joshgoebel commented Feb 15, 2022

I provided thoughts a day later on your mockup above (in Nov)... but the languages on the right side is bugging me less now than it was at my first glance... do you have this mockup/version actually implemented somewhere? If so a PR that I could play around with in person might lead to some progress here...

@not-my-profile
Copy link
Author

No, sorry that was just a quick mockup, I haven't implemented it. And I am currently busy with other projects.

@joshgoebel
Copy link
Member

See #3483. As we move forward with this we probably should (with v12+) specify a hard-coded "expected" font size in all themes (since contrast depends on font-size) so that the reference values we're using have some baseline meaning... then if someone changes the font-size on their own, that's on them to check that the new font size is accessible or not.

Currently our default theme CSS that all themes are forced to have is :

pre code.hljs {
  display: block;
  overflow-x: auto;
  padding: 1em;
}

... but we've never specified a font-size. In demo we specify 10pt (13px). I'm thinking perhaps we add a default font of 13px or 14px...?

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

No branches or pull requests

3 participants