Prevent accidental contrast degradations for styles via test

[not-my-profile]

Contrasts are important for accessibility ... many people don't have perfect vision, in which case a poor contrast can quickly hinder readability.

This pull request therefore introduces a new test case that prevents the accidental degradations of contrasts and facilitates the systematic improvement of style contrasts (because contrast improvements will show up in the diff of min_contrasts.json). The calculated minimum contrasts can in the future additionally be used to highlight styles that meet the WCAG AA criteria on the demo page.

To facilitate the parsing of the CSS files the first two commits do some minor changes that shouldn't change any visuals.

For more details refer to the commit messages. (Please don't squash the commits)

[joshgoebel]

This is a very interesting idea. :-) Thanks for the contribution!

First technical thoughts:

To facilitate the parsing of the CSS files the first two commits do some minor changes that shouldn't change any visuals.

This requirement is not at all desirable - We don't believe in adding additional requirements just to make [new] tooling happy. I would rather not have such arbitrary limitations on our CSS. If we're going to consider this then any tooling needs to be able to understand our CSS files "as-is"...

Then policy-wise:

that prevents the accidental degradations of contrasts and facilitates the systematic improvement of style contrasts

I think I'm on board with the intention of this PR - accessibility is a very good thing. Yet this may require discussion from a policy perspective. It's never really been our desire to "police" our themes - tell our theme authors what is good or bad (other than encouraging them to support new scopes). Themes are very personal/stylistic and that is very subjective even if their accessibility can be measured objectively.

This leads to all sorts of questions (just OTTOMH):

• If a theme author decides to make a theme less accessible, should that really result in a test failure?
• Related: Is making themes less accessible not allowed?
• What is the minimal accessibility threshold we "allow" and what does "allow" mean?
• Do we really want to divide our first party themes into the "blessed" vs the "not blessed" on the website? IE, if accessibility is so important as to call out certain themes, perhaps the real problem is having inaccessible themes at all - perhaps they should be 3rd party add-ons?
• We definitely apply different standard to first party grammars for example vs 3rd party grammars (where it's the wild-west on what is permissible)... so we have precedent here...

As you might imagine these questions are a bit interwoven. :-) Thanks so much for starting this discussion though!

CC @highlightjs/core Any thoughts?

[not-my-profile]

Hi Josh, thanks for your quick and thorough reply :)

Yes, I agree that the question of how to deal with theme accessibility needs discussion. However I don't think this pull request is the right place for it. I think it should take place in an issue ... I just opened #3392 for that.

I don't want this PR to be blocked by policy discussion, so I just updated the last commit so that the test case just asserts that the current contrast ratio is the same as the one defined in min_contrasts.json, i.e. you can still decrease contrasts, just not accidentally. If you decrease a contrast the test now fails with:

Error: minimum contrast decreased for style default.css (2.4 < 3.4)
if this is intentional please run tools/updateContrasts.js -f

Running the script as instructed makes the test pass.

This requirement is not at all desirable - We don't believe in adding additional requirements just to make [new] tooling happy. I would rather not have such arbitrary limitations on our CSS.

This is not about making tooling happy, this is about making users happy. I would consider not being able to access a website because it has a horrible contrast to be a significantly less desirable limitation than having to keep a couple of things in mind when contributing themes to this project. Not only does highlight.js have way more users than contributors, while contributors choose to contribute, users might not choose to use highlight.js, they might just want/need to access a website that happens to use highlight.js.

Besides the limitations I introduced are not "arbitrary" ... they were very much intentionally chosen to reduce the complexity of parsing the CSS. If the base foreground and background color can be arbitrarily split up across different CSS rules, you need an actual CSS parser to extract them. Sure you can use csstree but csstree is a low-level CSS parser, you'll need much code to handle all the ways colors can be specified in CSS. Let me elaborate:

• red is parsed as { type: 'Identifier', name: 'red' } ... you'll additionally need another dependency like css-color-names to resolve the color name to an actual value
• hsl(0, 100%, 50%) and rgb(255, 0, 0) is parsed as a { type: 'Function', children: [...] } where each argument and comma is represented as a separate child. To handle HSL we'll probably need a full-blown color library like color.
• If you don't want any limitations you also have to account for transparent colors. So you'll additionally need to deal with #ff0000aa, rgba(), hsla(). Is that worth it considering that there currently are only a handful of colors with transparency?
• The background property can become quite complex. If you really want to support arbitrary values you also need to deal with e.g. background: url(./something.jpg) repeat scroll left top orange; Note that csstree parses every word after the URL as an identifier, so you'll have to loop over every identifier to check if it's an actual color.

Instead of bothering with all of this it's probably easier to craft an .html file that demonstrates all token classes on top of the hljs class and then testing it with something like axe-core, which spins up a Chromium instance with Selenium ... but setting that up would also take more time than I can spare, besides significantly increasing the devDependencies (by pulling in a whole browser) and probably also test performance of highlight.js.

I apologize but my time is limited. This is by far not the only project I am trying to improve the accessibility of. I really cannot be bothered to spent hours implementing the above, so if you cannot accept these "arbitrary limitations on your CSS" then I hope you find another way to address the limitations you currently (be it inadvertently and indirectly) impose on some of your users.

[not-my-profile]

Closing in favor of #3394.

[joshgoebel]

Closing in favor of #3394.

Thanks. I think that'll get us close to your original goals if they were improved accessibility overall, better education, and helping people make better choices. I can for sure imagine how this "accessibility via CI" approach could benefit some projects, but I don't think it's a win for us - I feel it's solving a problem we really don't have (themes "downgrading" accessibility over time)... having badges in Demo (that would disappear if we backtracked) and analysis in our theme checker seems much better... if that proves insufficient we can circle back later with more data.

This is not about making tooling happy, this is about making users happy.

This is a false dichotomy. I think here we can have our cake and it too, so we'll first see if that's possible.

---

There is also a lot of project context you didn't have access to... that you can't be blamed for not knowing (see my meta note after):

• we may be using a more transparency in the future (Discuss: Higher fidelity language highlighting (in general) #2500)
• overall themes rarely change [are rarely added, plus I'm not really sure we need any more themes at this point]
• we already have a theme checker that is the better place for such things
• we already use a CSS parser and build themes dynamically
• if we did every do CI on themes in the future it'd likely be via the theme checker

---

On a higher meta-level note:

I'm not sure if we're an anomaly (wrt OSS projects) but I think you possibly could have saved yourself some time if you'd come to us with a more "pure data" approach more like the analysis thread you pointed to for pygments - simply pointing out the problem with some high level suggestions (like #3392) - rather than assuming a specific solution to the problem. Anyways, hopefully it wasn't a large amount of effort on your part and the progress we make when #3392 finally close makes it all worth while.

Truly, thanks again for kicking us on this. :-)

[not-my-profile]

Right, if you only seldomly change themes / add new themes tracking the contrasts in git doesn't give you that much of a benefit. Especially if you don't want to "police" your themes.

This is a false dichotomy.

That's not what I meant. I meant that I wasn't doing this PR "just to make [new] tooling happy" as you kinda implied.

Yeah, you're right I could've saved myself some time. Not much though, since I spent most of it on the data extraction part ... writing the test case / update script didn't take that long.

Truly, thanks again for kicking us on this. :-)

You're welcome :)

[not-my-profile]

I do however want to add that tracking the contrasts in Git really helps with systematically improving the contrasts of your existing themes. Because with each PR you can just see the contrast change in the diff. Otherwise you'd need a GitHub bot for that ... which would take way more time to develop / set up.