rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

review: measure what can be measured, and say what is not

Browse source 
Acting on the review of #621.  No bugs were found; these are the four
suggestions and two nits.

The real one: measurement accepted only hex.  `relativeLuminance` returned 0 for
anything else, which is black's luminance and indistinguishable from it -- so a
token written `rgb(92 79 168)` measured as pure black, the "best" foreground was
chosen against a colour the theme does not contain, and the wrong hex was then
frozen into an inline style where CSS could no longer correct it.  Silent, and
worse than not measuring.  `parseColor` now reads `rgb()`/`rgba()` in both the
comma and space forms, percentages, and `#rgba`/`#rrggbbaa`; `isMeasurable` lets
a caller tell "unreadable" from "black"; and navColorsFrom falls back to the
tokens when the background or every candidate is unreadable.  Unreadable
candidates are filtered rather than the whole measurement abandoned, so one
oddly written token does not cost the other two.  This matters because the
header claimed custom YAML themes get correct foregrounds for free, and a
hand-written palette is exactly where a non-hex value will first appear.

The Cypress inverse hardcoded night's `--btn-text` as `#0a0000`.  The unit tests
parse tokens.css precisely so a palette edit cannot leave a stale claim behind,
and that literal was the one place the rule was broken.  It reads the token from
the theme now, asserts it resolved, and additionally pins that red still takes
`--btn-text` while brown no longer does.

Documented rather than changed: the warning indicators still pick their colour
from the `conflictingColors` name list.  It is the same defect as the one fixed
here and measurably worse -- `--danger` and `--warning` are 1.0-1.8:1 against
the bar in every theme, light and dark included, because a red icon on a red bar
is a red icon on a red bar.  The fix is not a better colour: severity has to
stay distinguishable from the bar's text as well as from itself, and a
monochrome theme has no third brightness, so it has to stop being a hue.  That
is a redesign, and the note now sits beside the residue in navColors.ts rather
than only in a PR description.  The gallery draws the indicators as the bar
draws them when nothing is wrong; it does not stage a fake overheating Pi to
exhibit a fault nobody has agreed how to fix.

Also documented: re-measurement is triggered by `data-theme`, so a future
custom-theme editor patching custom properties in place would leave the bar on
the values it last measured.  There is no event for "a custom property was
assigned" to observe instead, so the note says what such an editor must do.

Nits: "Eleven" in a comment where the assertion locks ten (and a stale 2.91:1
beside it, now 3.27:1), and a missing space in `const wrolpiIcon =<img`.

Six new unit cases, planted: dropping the fallback reddens two.
1121 jest / 62 suites, 196 in ui-layout.cy.js, clean npm run build.
This commit is contained in:
Roland 2026-08-02 14:57:17 -06:00
parent 3f1e0b386f
commit 2aa5360932
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
5 changed files with 203 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

2
app/src/components/Nav.js
View file

@ -208,7 +208,7 @@ export function NavBar() {
* user's colour, not taken from a single token. See themes/navColors.
*/
const navColors = useNavColors(navColor);
const wrolpiIcon =<img src='/icon.svg' height='32px' width='32px' alt='WROLPi Home Icon'/>;
const wrolpiIcon = <img src='/icon.svg' height='32px' width='32px' alt='WROLPi Home Icon'/>;
const name = <i>{NAME || wrolpiIcon}</i>;
const topNavText = wrolModeEnabled ? <>{name}&nbsp; <Icon name='lock'/></> : name;

19
app/src/components/ui/ui-layout.cy.js
View file

@ -2199,8 +2199,13 @@ describe('the navigation bar is legible on every colour a user can pick', () =>
* The inverse. Every case above would pass on the old fixed foreground in the two
* themes where `--btn-text` happens to be the right end of the palette, so without
* this the suite could go green on a revert. Night is where the difference is
* largest: `--brown` takes `--white` (#ff8a8a) and `--red` keeps `--btn-text`, so
* the bar's foreground is demonstrably not one value.
* largest: `--brown` takes `--white` and `--red` keeps `--btn-text`, so the bar's
* foreground is demonstrably not one value.
*
* `--btn-text` is READ from the theme rather than written out as a hex. The unit
* tests parse tokens.css precisely so a palette edit cannot leave a stale claim
* behind, and a literal here would be the one place that rule was broken -- it would
* either go stale or quietly stop testing anything if night's token moved.
*/
cy.mountUI(<div>
<NavBarSample color='brown'/>
@ -2211,11 +2216,19 @@ describe('the navigation bar is legible on every colour a user can pick', () =>
cy.get('[data-nav-sample="red"] .wrolpi-navbar-link').then(($red) => {
const brown = getComputedStyle($brown[0]).color;
const red = getComputedStyle($red[0]).color;
const btnText = getComputedStyle(document.documentElement)
.getPropertyValue('--btn-text');
// The premise: the token resolved. `toRgb` throws on an unresolved value
// rather than letting the comparison below pass by comparing nothing.
expect(toRgb(btnText), 'night --btn-text resolved').to.match(/^rgb/);
expect(brown, 'brown and red bars take different foregrounds')
.to.not.equal(red);
// And the one that changed is the one that was unreadable.
expect(toRgb(brown), 'brown no longer takes --btn-text')
.to.not.equal(toRgb('#0a0000'));
.to.not.equal(toRgb(btnText));
// While red, which was always legible, still does.
expect(toRgb(red), 'red still takes --btn-text').to.equal(toRgb(btnText));
});
});
});

56
app/src/themes/contrast.ts
View file

@ -8,14 +8,14 @@
* its existing callers are unaffected.
*/
/** A hex colour as [r, g, b], accepting both `#rrggbb` and the shorthand `#rgb`. */
/** A hex colour as [r, g, b], accepting `#rgb`, `#rrggbb`, and `#rrggbbaa`. */
export function hexToRGBArray(color: string): number[] | undefined {
let hex = (typeof color === 'string' ? color : '').trim().replace(/^#/, '');
if (hex.length === 3) {
// #abc means #aabbcc, not #0a0b0c.
if (hex.length === 3 || hex.length === 4) {
// #abc means #aabbcc, not #0a0b0c. A fourth digit is alpha; see parseColor.
hex = hex.split('').map(digit => digit + digit).join('');
}
if (!/^[0-9a-fA-F]{6}$/.test(hex)) {
if (!/^[0-9a-fA-F]{6}([0-9a-fA-F]{2})?$/.test(hex)) {
return undefined;
}
const rgb: number[] = [];
@ -23,6 +23,46 @@ export function hexToRGBArray(color: string): number[] | undefined {
return rgb;
}
/**
* Any colour these measurements can actually read, as [r, g, b] — or undefined.
*
* Undefined matters as much as the value. `relativeLuminance` used to return 0 for
* everything it could not parse, which is indistinguishable from black: a token written as
* `rgb(90, 79, 168)` measured as pure black, so the "best" foreground was chosen against a
* colour the theme does not contain, and the wrong one was frozen into the bar's inline
* style. A caller that cannot tell "unreadable" from "black" cannot fall back, and the
* custom YAML themes ahead of us are exactly where a non-hex token will first appear.
*
* Alpha is deliberately dropped rather than rejected: WCAG contrast is defined on composited
* colour, and what a translucent value composites over is not knowable here. Every token in
* tokens.css is opaque, so this only arises for a hand-written theme, where measuring the
* colour itself is a better answer than refusing.
*/
export function parseColor(color: string): number[] | undefined {
const value = (typeof color === 'string' ? color : '').trim();
if (!value) return undefined;
if (value.startsWith('#')) return hexToRGBArray(value);
// `rgb(90 79 168 / 0.5)` and `rgba(90, 79, 168, 0.5)` are both current syntax.
const functional = value.match(/^rgba?\(([^)]*)\)$/i);
if (functional) {
const parts = functional[1].split(/[\s,/]+/).filter(Boolean).slice(0, 3);
if (parts.length !== 3) return undefined;
const channels = parts.map((part) => {
// A percentage is 0-100 of full scale; a bare number is already 0-255.
const numeric = parseFloat(part);
if (Number.isNaN(numeric)) return NaN;
return part.includes('%') ? (numeric / 100) * 255 : numeric;
});
if (channels.some(Number.isNaN)) return undefined;
return channels.map(channel => Math.min(255, Math.max(0, channel)));
}
// A named colour, `hsl()`, `color-mix()`, or an unresolved `var()`. Not measurable
// here, and saying so is the point.
return undefined;
}
/**
* WCAG relative luminance: sRGB channels linearised first, then Rec. 709 weighted.
*
@ -31,8 +71,11 @@ export function hexToRGBArray(color: string): number[] | undefined {
* the wrong text colour for them.
*/
export function relativeLuminance(color: string | number[]): number {
const rgb = (typeof color === 'string') ? hexToRGBArray(color) : color;
const rgb = (typeof color === 'string') ? parseColor(color) : color;
if (!rgb) {
// Zero is black's luminance, so this is a lie -- but the signature has no way to
// say otherwise, and every caller that needs to distinguish the two asks
// `isMeasurable` first.
return 0;
}
const linear = rgb.map(value => {
@ -48,6 +91,9 @@ export function contrastRatio(a: string | number[], b: string | number[]): numbe
return (luminances[0] + 0.05) / (luminances[1] + 0.05);
}
/** Whether a colour can be measured at all, as distinct from measuring as black. */
export const isMeasurable = (color: string): boolean => parseColor(color) !== undefined;
/**
* Whichever candidate actually reads best on `background`.
*

88
app/src/themes/navColors.test.js
View file

@ -1,6 +1,6 @@
import fs from 'fs';
import path from 'path';
import {contrastRatio} from './contrast';
import {contrastRatio, isMeasurable} from './contrast';
import {defaultNavColor, navColorNames, navColorsFrom} from './navColors';
import {themeNames} from './names';
import {semanticUIColorMap} from '../components/Vars';
@ -150,11 +150,11 @@ describe('the navbar foreground', () => {
describe('what measurement cannot fix', () => {
/*
* Eleven of the forty-eight combinations still sit under 4.5:1, and no choice of
* Ten of the forty-eight combinations still sit under 4.5:1, and no choice of
* foreground will lift them, because the shortfall is in the BACKGROUND.
*
* A monochrome theme resolves all twelve colour names onto one red (or amber) ramp, and
* seven of night's land in the middle of it -- #b03030 is 2.91:1 from night's darkest red
* six of night's land in the middle of it -- #b03030 is 3.27:1 from night's darkest red
* and 2.52:1 from its brightest, so the best available option is the one this picks and
* it is still short. There is no foreground in a one-hue palette that reads on a
* mid-tone of that same hue; the fix, if the residue matters, is to the palette.
@ -242,3 +242,85 @@ describe('when the tokens cannot be read', () => {
.toEqual(paletteOf('light')[`--${defaultNavColor}`]);
});
});
describe('a token this cannot measure', () => {
/*
* Every token in tokens.css is a hex, so this is about the custom YAML themes ahead of
* us -- the first place a palette will be written by hand, and the first place something
* other than a hex will appear.
*
* The failure it guards was silent and worse than doing nothing. An unparseable colour
* measured as luminance zero, indistinguishable from black, so the bar chose its
* foreground against a colour the theme does not contain and then froze the wrong hex
* into an inline style, where CSS could no longer correct it. Falling back leaves the
* bar painted by CSS: wrong about the foreground at worst, rather than about both.
*/
const hexPalette = () => paletteOf('light');
/** The light palette with one token rewritten into some other notation. */
const readerWith = (overrides) => {
const palette = {...hexPalette(), ...overrides};
return (token) => palette[token] || '';
};
it('measures rgb() as readily as hex, since both are colours', () => {
// `--violet` is #5c4fa8 in light. The same colour, written the other way, must
// produce the same answer rather than falling back.
const asHex = navColorsFrom(readerFor('light'), 'violet');
const asRgb = navColorsFrom(readerWith({'--violet': 'rgb(92, 79, 168)'}), 'violet');
expect(asRgb.color).toEqual(asHex.color);
expect(asRgb.ratio).toBeCloseTo(asHex.ratio, 4);
});
it('handles the modern space-separated form too', () => {
const asHex = navColorsFrom(readerFor('light'), 'violet');
const asRgb = navColorsFrom(readerWith({'--violet': 'rgb(92 79 168 / 1)'}), 'violet');
expect(asRgb.ratio).toBeCloseTo(asHex.ratio, 4);
});
it('falls back rather than measuring a background it cannot read', () => {
// `color-mix` is the shape a hand-written theme is most likely to reach for, and
// resolving it needs a browser.
const colors = navColorsFrom(
readerWith({'--olive': 'color-mix(in srgb, #6f7a24 60%, white)'}), 'olive');
expect(colors).toEqual({
background: 'var(--olive)', color: 'var(--btn-text)', ratio: null,
});
});
it('falls back when no candidate foreground can be read', () => {
const colors = navColorsFrom(readerWith({
'--btn-text': 'hsl(0 0% 100%)', '--black': 'rebeccapurple', '--white': 'hsl(0 0% 0%)',
}), 'violet');
expect(colors.ratio).toBeNull();
});
it('still measures when only SOME candidates are unreadable', () => {
/*
* The narrow case, and the reason the unreadable ones are filtered out rather than
* the whole measurement abandoned. A theme that writes one token oddly should not
* lose the measurement for the other two.
*/
const colors = navColorsFrom(
readerWith({'--white': 'oklch(0.9 0.1 20)'}), 'violet');
expect(colors.ratio).not.toBeNull();
expect([hexPalette()['--btn-text'], hexPalette()['--black']]).toContain(colors.color);
});
it('is a real constraint: an unreadable colour is NOT treated as black', () => {
/*
* Without this the fallback could be removed and every case above would still pass
* on `relativeLuminance` returning zero -- which is precisely the bug, because zero
* is a legitimate luminance and nothing downstream could tell the two apart.
*/
expect(contrastRatio('color-mix(in srgb, red, blue)', '#ffffff'))
.toBeCloseTo(contrastRatio('#000000', '#ffffff'), 4);
expect(isMeasurable('color-mix(in srgb, red, blue)')).toBe(false);
expect(isMeasurable('#000000')).toBe(true);
});
});

59
app/src/themes/navColors.ts
View file

@ -1,5 +1,5 @@
import React from 'react';
import {bestForeground, contrastRatio} from './contrast';
import {bestForeground, contrastRatio, isMeasurable} from './contrast';
/*
@ -23,8 +23,31 @@ import {bestForeground, contrastRatio} from './contrast';
* night's "white" is #ff8a8a, a bright red, and amber's is #ffc875.
*
* Measuring at runtime rather than shipping a paired `--on-red`/`--on-olive` token for every
* colour also means the custom YAML themes still ahead of us get this for free: a theme
* supplies a palette, and the correct foreground for each entry falls out of it.
* colour also means a theme only has to supply a palette: the correct foreground for each
* entry falls out of it, with nothing extra to declare and nothing to keep in step.
*
* What is NOT measured, and should be: the warning indicators. NavBar still picks their
* colour from `conflictingColors`, a hardcoded list of navbar names that swaps `yellow`/`red`
* for `null`/`black` on the five bars those hues would blend into. It is the same defect
* this replaced -- a name list standing in for a measurement -- and it is worse than the one
* fixed here rather than better: `--danger` and `--warning` measure 1.0-1.8:1 against the bar
* in EVERY theme, light and dark included, because a red icon on a red bar is a red icon on a
* red bar. It is left alone deliberately, because the fix is not a better colour. Those
* icons have to stay distinguishable from each other AND from the bar's own text, and in a
* monochrome theme there is no third brightness to spend, so severity has to stop being a
* hue -- a filled badge, or a shape. That is a redesign of how severity reads, not a
* substitution, and it is not this change.
*
* /theme-sample draws the indicators as they are, taking the measured bar colour, which is
* what the bar does when nothing is wrong. It does not stage a fake overheating Pi to
* exhibit a fault nobody has agreed how to fix.
*
* Two limits on the measuring that IS done here, both of which the custom YAML themes will
* meet before anything else does. A token has to be a colour these measurements can read -- hex or `rgb()`; anything
* else falls back to CSS rather than being measured (see navColorsFrom). And re-measurement
* is triggered by `data-theme` changing, which is how a theme is applied today; a future
* editor that patches custom properties in place without touching the attribute would leave
* the bar on the values it last measured (see useNavColors).
*/
/**
@ -73,17 +96,28 @@ export function navColorsFrom(read: (name: string) => string, navColor: string):
*/
const candidates = [read('--btn-text'), read('--black'), read('--white')].filter(Boolean);
if (!background || candidates.length === 0) {
const measurable = candidates.filter(isMeasurable);
if (!isMeasurable(background) || measurable.length === 0) {
/*
* No stylesheet to read: jsdom, or a very early render. Fall back to the tokens
* themselves so the bar is still painted by CSS and still changes with the theme --
* this is the pre-measurement behaviour, and it is only ever wrong about the
* foreground.
* Nothing readable to measure, so measure nothing and let CSS do its job.
*
* Two cases reach here. The ordinary one is that there is no stylesheet yet -- jsdom,
* or a very early render -- and the reader returns empty strings. The other is a
* token this cannot parse: a hand-written theme writing `rgb(90 79 168)` or
* `color-mix(...)` rather than a hex. That one used to be silent and worse than
* useless: an unparseable colour measured as luminance zero, indistinguishable from
* black, so the bar picked its foreground against a colour the theme does not contain
* and then froze the wrong hex into an inline style, where CSS could not correct it.
*
* Falling back to the tokens themselves is the pre-measurement behaviour: the bar is
* painted by CSS, follows the theme, and is only ever wrong about the foreground --
* which is a great deal better than being confidently wrong about both.
*/
return {background: `var(--${name})`, color: 'var(--btn-text)', ratio: null};
}
const color = bestForeground(background, candidates) as string;
const color = bestForeground(background, measurable) as string;
return {background, color, ratio: contrastRatio(color, background)};
}
@ -126,6 +160,13 @@ export function navBarStyle(colors: NavColors): React.CSSProperties {
*
* The attribute is also stamped by the inline script in index.html before React loads, and
* that is a change no context can report at all.
*
* This assumes token VALUES only change when `data-theme` does, which holds for every way a
* theme is applied today. It will not hold for a custom-theme editor that rewrites custom
* properties in place: the measured pair is a snapshot of hexes, so the bar would keep the
* old one until the theme name or the chosen colour changed. When that editor exists it
* should either re-stamp the attribute or announce the change for this to observe -- there
* is no event for "a custom property was assigned" to watch instead.
*/
export function useNavColors(navColor: string): NavColors {
const [colors, setColors] = React.useState<NavColors>(