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

(enh) improve default theme accessibility #3402

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion CHANGES.md
@@ -1,4 +1,11 @@
## Version 11.3.2 (most likely)
## Version 11.4 (most likely)

Themes:

- `Default` is now much closer WCAG AA (contrast) (#3402) [Josh Goebel]
- `Dark` now meets WCAG AA (contrast) (#3402) [Josh Goebel]

These changes should be for the better and should not be super noticeable but if you're super picky about your colors you may want to intervene here or copy over the older themes from 11.3 or prior.

Grammars:

Expand Down
55 changes: 54 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Expand Up @@ -65,6 +65,7 @@
"colors": "^1.1.2",
"commander": "8.2",
"css": "^3.0.0",
"css-color-names": "^1.0.1",
"deep-freeze-es6": "^1.4.1",
"del": "^6.0.0",
"dependency-resolver": "^2.0.1",
Expand All @@ -84,6 +85,7 @@
"should": "^13.2.3",
"terser": "^5.7.0",
"tiny-worker": "^2.3.0",
"typescript": "^4.4.4"
"typescript": "^4.4.4",
"wcag-contrast": "^3.0.0"
}
}
4 changes: 2 additions & 2 deletions src/styles/dark.css
Expand Up @@ -6,7 +6,7 @@ Dark style from softwaremaniacs.org (c) Ivan Sagalaev <Maniac@SoftwareManiacs.Or

.hljs {
color: #ddd;
background: #444;
background: #303030;
}

.hljs-keyword,
Expand Down Expand Up @@ -41,7 +41,7 @@ Dark style from softwaremaniacs.org (c) Ivan Sagalaev <Maniac@SoftwareManiacs.Or
.hljs-quote,
.hljs-deletion,
.hljs-meta {
color: #777;
color: #979797;
}

.hljs-keyword,
Expand Down
10 changes: 5 additions & 5 deletions src/styles/default.css
Expand Up @@ -26,7 +26,7 @@ code.hljs {
/* end baseline CSS */

.hljs {
background: #F0F0F0;
background: #F3F3F3;
color: #444;
}

Expand All @@ -43,7 +43,7 @@ code.hljs {
.hljs-params {}

.hljs-comment {
color: #888888;
color: #697070;
}
.hljs-tag,
.hljs-punctuation {
Expand Down Expand Up @@ -94,13 +94,13 @@ code.hljs {
.hljs-selector-attr,
.hljs-operator,
.hljs-selector-pseudo {
color: #BC6060;
color: #ab5656;
}

/* Language color: hue: 90; */

.hljs-literal {
color: #78A960;
color: #695;
}

.hljs-built_in,
Expand All @@ -118,7 +118,7 @@ code.hljs {
}

.hljs-meta .hljs-string {
color: #4d99bf;
color: #38a;
}


Expand Down
82 changes: 82 additions & 0 deletions tools/checkTheme.js
Expand Up @@ -2,6 +2,13 @@

const fs = require("fs");
const css = require("css");
const wcagContrast = require("wcag-contrast");
const Table = require('cli-table');
const csscolors = require('css-color-names');
// const colors = require('colors/safe.js');



joshgoebel marked this conversation as resolved.
Show resolved Hide resolved
require("colors");

const CODE = {
Expand Down Expand Up @@ -184,6 +191,79 @@ function check_group(group, rules) {
}
}

const round2 = (x) => Math.round(x*100)/100;

class CSSRule {
constructor(rule, body) {
this.rule = rule;
if (rule.declarations) {
this.bg = rule.declarations.find(x => x.property =="background")?.value;
this.fg = rule.declarations.find(x => x.property =="color")?.value;
Comment on lines +196 to +197
Copy link
Member

Choose a reason for hiding this comment

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

Two notes on this one:

  1. Some styles use background-color to differentiate the background for specific elements. I think we should add support for checking background-color for WCAG contrast.

    .hljs-regexp {
    background-color: #fff0ff;
    color: #880088;
    }

  2. Should we have a minimum Node defined somewhere for dev environments? Optional chaining was added in Node 14 and our package.json requires Node 12+ (correctly so, for our users).

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we have a minimum Node defined somewhere for dev environments?

Ah, we probably should, but where... damn old versions of Node. :-)

Some styles use background-color

I'll break this out into a separate issue... the tool doesn't have to be perfect for the main thrust of this to be mergeable.


if (this.bg) {
this.bg = csscolors[this.bg] || this.bg;
}
if (this.fg) {
this.fg = csscolors[this.fg] || this.fg;
}

// inherit from body if we're missing fg or bg
if (this.hasColor) {
if (!this.bg) this.bg = body.background;
if (!this.fg) this.fg = body.foreground;
}
}
}
get background() {
return this.bg
}

get foreground() {
return this.fg
}
get hasColor() {
if (!this.rule.declarations) return false;
return this.fg || this.bg;
}
toString() {
return ` ${this.foreground} on ${this.background}`
}

contrastRatio() {
if (!this.foreground) return "unknown (no fg)"
if (!this.background) return "unknown(no bg)"
joshgoebel marked this conversation as resolved.
Show resolved Hide resolved
return round2(wcagContrast.hex(this.foreground, this.background));
Copy link
Member

Choose a reason for hiding this comment

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

We have some themes that use rgba(), will .hex() work on those values?

.hljs-comment {
color: rgba(149,165,166,.8);
}

}
}

function contrast_report(rules) {
console.log("Accessibility Report".yellow);

var hljs = rules.find (x => x.selectors && x.selectors.includes(".hljs"));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var hljs = rules.find (x => x.selectors && x.selectors.includes(".hljs"));
var hljs = rules.find(x => x.selectors?.includes(".hljs"));

nitpick

Copy link
Member Author

Choose a reason for hiding this comment

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

As you point out this isn't supported everywhere so I'm going to slow down with it until we figure our how/where to specify that dev work requires newer versions...

var body = new CSSRule(hljs);
const table = new Table({
chars: {'mid': '', 'left-mid': '', 'mid-mid': '', 'right-mid': ''},
head: ['ratio', 'selector', 'fg', 'bg'],
colWidths: [7, 40, 10, 10],
style: {
head: ['grey']
}
});

rules.forEach(rule => {
var color = new CSSRule(rule, body);
if (!color.hasColor) return;
table.push([
color.contrastRatio(),
rule.selectors,
color.foreground,
color.background
])
// console.log(r.selectors[0], color.contrastRatio(), color.toString());
joshgoebel marked this conversation as resolved.
Show resolved Hide resolved
})
console.log(table.toString())
}

function validate(data) {
const rules = data.stylesheet.rules;

Expand All @@ -195,6 +275,8 @@ function validate(data) {
check_group(CODE, rules);
check_group(OTHER, rules);
check_group(HIGH_FIDELITY, rules);

contrast_report(rules);
}

process.argv.shift();
Expand Down