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

test: act on review — and correct an overstated claim about the toast bug

Browse source 
I said all four toast kinds were "the same pixel" in night.  They were not.
Planting a reversion to hue names left the new distinctness test GREEN, because
in night `--blue`, `--green`, `--yellow` and `--red` are four different values
(2.29, 3.14, 4.52, 5.64 against a panel).  What they are is cramped and badly
ranked, with info at 2.29:1 -- below the 3:1 floor, present and invisible.

The byte-identical pairs are real but elsewhere: `--orange` is `--text` and
`--yellow` is `--amber`, which bites a call site that reaches for those names,
not the toast mapping.  The PR body has been corrected too.

So the distinctness assertion is not the one with teeth; the legibility floor
is.  The monochrome toast test now measures every kind against the toast's own
surface, and the hue reversion fails it naming the number: 2.29:1.

Also from review:

`.wrolpi-message-error` still forced `border: 1.5px dashed var(--red)` while its
accent and icon had moved to roles -- nearly the same pixel in night, so no
colour test could see it.  It follows `--message-color` now.  Six more library
rules were naming a hue for a thing that means danger: the night danger button,
invalid form fields, invalid paths, path error text, danger panels and failed
table rows.  All roles now, with a source guard that fails any rule whose
SELECTOR means severity and whose body names a hue.  Focus rings and active-tab
accents deliberately keep `--blue`: those are the primary accent, not severity.

The tokens.css note claimed the brightness ramp held "in every theme" when the
tests deliberately limit it to monochrome.  Narrowed, with the reason spelled
out, because the next person to read it would otherwise have "fixed" light to
match the prose -- which would mean reordering light's hues to satisfy a test.

Added the end-of-chain cases the review asked for: four toasts and four Messages
in night and amber, measured rather than inferred from variable names.

Colour reads now go through a strict `toRgb` that refuses anything not already
resolved.  The reviewer's concern that `getPropertyValue('--info')` returns an
unresolved `var(--blue)` does not hold -- custom properties are substituted at
computed-value time and it returns `#20629c` -- but feeding an unresolved value
to a luminance calculation yields NaN, and `expect(NaN).to.not.equal(x)` passes.
Worth refusing loudly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-08-01 17:46:33 -06:00 • committed by lrnselfreliance
parent 08c5c233c8
commit c7fddc55af
4 changed files with 155 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -1,11 +1,12 @@
import React from 'react';
import {
ActionInput, Button, Header, Icon, IconStack, Loading, MultiSelect, Panel, PathInput,
Placeholder, Statistic, StatisticGroup, Status, TabBar, Table, tabClassName, TextInput,
Message, Placeholder, Statistic, StatisticGroup, Status, TabBar, Table, tabClassName,
TextInput,
} from './index';
import {contrastingColor} from '../Common';
import {Notifications} from '@mantine/notifications';
import {toast} from './toast';
import {clearToasts, toast} from './toast';
import {monochromeThemes, themeNames} from '../../themes/names';
/* `--panel` is authored as a hex; computed backgrounds come back as rgb(). */
@ -15,6 +16,24 @@ const hexToRgb = (hex) => {
return `rgb(${r}, ${g}, ${b})`;
};
/*
* Normalise a colour to rgb(), and REFUSE anything that is neither a hex nor already
* rgb(). Strictness is the point: feeding an unresolved `var(--blue)` to a luminance
* calculation yields NaN, and `expect(NaN).to.not.equal(x)` passes -- a test that cannot
* fail. Better to blow up naming the value than to quietly measure nothing.
*
* Tokens do resolve: `getPropertyValue('--info')` returns `#20629c` in light, because
* custom properties are substituted at computed-value time. This guards the day that
* stops being true on some engine, and catches the hex/rgb mismatch that is easy to hit
* when reading a Mantine variable rather than a painted property.
*/
const toRgb = (value) => {
const trimmed = String(value).trim();
if (/^rgba?\(/.test(trimmed)) return trimmed;
if (/^#[0-9a-fA-F]{6}$/.test(trimmed)) return hexToRgb(trimmed);
throw new Error(`Not a resolved colour: "${trimmed}"`);
};
/*
* Layout and theming assertions for the component library, run in a real browser.
*
@ -192,13 +211,11 @@ describe('semantic roles read as severity in every theme', () => {
const roleColours = () => {
const root = getComputedStyle(document.documentElement);
const panel = hexToRgb(root.getPropertyValue('--panel').trim());
const panel = toRgb(root.getPropertyValue('--panel'));
return {
panel,
text: hexToRgb(root.getPropertyValue('--text').trim()),
roles: ROLES.map(role => ({
role, colour: hexToRgb(root.getPropertyValue(`--${role}`).trim()),
})),
text: toRgb(root.getPropertyValue('--text')),
roles: ROLES.map(role => ({role, colour: toRgb(root.getPropertyValue(`--${role}`))})),
};
};
@ -954,6 +971,73 @@ describe('a toast is readable in every theme', () => {
});
});
monochromeThemes.forEach((theme) => {
it(`tells its four kinds apart in ${theme}, which is the bug roles exist for`, () => {
/*
* The end of the chain, measured rather than inferred. The jest tests assert the
* Mantine variable NAME on the style attribute, which would still pass if someone
* put four hue names back -- four distinct strings that resolve to two colours here.
* This reads what is painted.
*/
cy.mountUI(<>
<Notifications position='top-right' limit={5}/>
<button type='button' onClick={() => {
/*
* Mantine's notification store is module scope and outlives the mount, so
* a toast raised by an earlier test in this file is still queued here --
* which is how this first ran with five.
*/
clearToasts();
['info', 'success', 'warning', 'error'].forEach(type =>
toast({type, title: type, time: 0}));
}}>Raise all four</button>
</>, {theme});
cy.contains('button', 'Raise all four').click();
cy.get('[class*="mantine-Notification-root"]').should('have.length', 4);
cy.get('[class*="mantine-Notification-root"]').should(($toasts) => {
const colours = [...$toasts].map(t =>
getComputedStyle(t).getPropertyValue('--notification-color').trim());
expect(new Set(colours).size, `four distinct kinds, got ${colours.join(' ')}`)
.to.equal(4);
/*
* Distinctness alone does not catch the regression this is for. Reverting to
* hue names gives four distinct values in night too -- blue, green, yellow and
* red are not the same colour there. What they are is cramped and badly
* ranked, with `--blue` at 2.29:1 against the toast surface: present, and
* invisible. So the floor is the assertion that has teeth.
*/
const surface = toRgb(getComputedStyle($toasts[0]).backgroundColor);
colours.forEach((colour, index) => {
expect(contrast(toRgb(colour), surface), `toast ${index} against its surface`)
.to.be.at.least(3);
});
});
});
it(`tells four Messages apart in ${theme}`, () => {
cy.mountUI(
<Panel>
<Message kind='info' title='info'/>
<Message kind='success' title='success'/>
<Message kind='warning' title='warning'/>
<Message kind='error' title='error'/>
</Panel>,
{theme},
);
cy.get('.wrolpi-message').should(($all) => {
expect($all.length).to.equal(4);
const colours = [...$all].map(m =>
getComputedStyle(m).getPropertyValue('--message-color').trim());
expect(new Set(colours).size, `four distinct kinds, got ${colours.join(' ')}`)
.to.equal(4);
});
});
});
it('keeps the title more prominent than the message', () => {
/*
* Night and amber drop the description's muting, because `--muted` on a panel is 2.0:1

27
app/src/components/ui/ui.css
View file

@ -92,8 +92,8 @@
*/
html[data-theme="night"] .wrolpi-button-danger {
background: transparent;
color: var(--red);
border: 1.5px dashed var(--red);
color: var(--danger);
border: 1.5px dashed var(--danger);
}
html[data-theme="night"] .wrolpi-button-danger:hover:not(:disabled) {
@ -431,7 +431,7 @@ html[data-theme="amber"] .wrolpi-form-field input:focus {
/* An invalid field is dashed, matching every other failure state in the interface. */
.wrolpi-form-field[data-error] input {
border: 1.5px dashed var(--red);
border: 1.5px dashed var(--danger);
}
.wrolpi-form-field input:disabled {
@ -471,7 +471,7 @@ html[data-theme="amber"] .wrolpi-path-input-control:focus-within {
}
.wrolpi-path-input-control[data-error] {
border: 1.5px dashed var(--red);
border: 1.5px dashed var(--danger);
}
.wrolpi-path-input-control[data-disabled] {
@ -533,7 +533,7 @@ html[data-theme="amber"] .wrolpi-path-input-control:focus-within {
.wrolpi-path-input-error {
font-size: 12px;
color: var(--red);
color: var(--danger);
margin-top: 3px;
}
@ -925,8 +925,13 @@ html .mantine-Notification-closeButton:hover {
}
/* Errors take the danger treatment in every theme, so "dashed" stays one rule. */
/*
* The dashed frame tracks the role the message was given, exactly as its accent and
* icon already do. Naming a hue here meant the frame and the accent could disagree --
* invisible in night, where they are nearly the same pixel, but still wrong.
*/
.wrolpi-message-error {
border: 1.5px dashed var(--red);
border: 1.5px dashed var(--message-color);
}
/* --------------------------------------------------------------- Progress */
@ -1019,7 +1024,7 @@ html[data-theme="amber"] .wrolpi-progress-fill {
/* Danger zones are dashed in every theme; it doubles as a colorblind-safe cue. */
.wrolpi-panel-danger {
border: 1.5px dashed var(--red);
border: 1.5px dashed var(--danger);
}
/* ----------------------------------------------------------- Card posters */
@ -1245,16 +1250,16 @@ html[data-theme="amber"] .wrolpi-th-sort:focus-visible {
/* A failed row carries the danger border, matching failed messages. */
.wrolpi-row-failed > td {
border-top: 1.5px dashed var(--red);
border-bottom: 1.5px dashed var(--red);
border-top: 1.5px dashed var(--danger);
border-bottom: 1.5px dashed var(--danger);
}
.wrolpi-row-failed > td:first-child {
border-left: 1.5px dashed var(--red);
border-left: 1.5px dashed var(--danger);
}
.wrolpi-row-failed > td:last-child {
border-right: 1.5px dashed var(--red);
border-right: 1.5px dashed var(--danger);
}
/* Wide tables scroll inside their own container; the page never scrolls sideways. */

31
app/src/components/ui/ui.test.js
View file

@ -663,6 +663,37 @@ describe('Label as a tag', () => {
});
});
describe('the library names severity by role, never by hue', () => {
/*
* A source guard, because a hue here is invisible in review and invisible in most tests.
* In night `--red` and `--danger` are nearly the same pixel, so a rule that reverts to
* `var(--red)` looks right in every screenshot and every distinctness check -- and is
* still wrong, because the whole point is that a theme gets to decide what danger looks
* like. This is how `.wrolpi-message-error` kept a hardcoded `var(--red)` frame through
* the change that moved its accent and icon onto roles.
*
* Focus rings and active-tab accents deliberately stay `--blue`: those are the primary
* accent, not a severity, and `--primary` is a separate token for exactly that reason.
*/
const severity = /(danger|error|failed|invalid|warning|success|complete)/i;
it('uses no severity hue in a rule whose name means severity', () => {
const css = fs.readFileSync(path.join(__dirname, 'ui.css'), 'utf8');
const hues = /var\(--(red|green|amber|yellow|orange)\)/;
// Split into rules, keeping each selector with its body.
const offenders = [];
for (const match of css.matchAll(/([^{}]+)\{([^}]*)\}/g)) {
const [, selector, body] = match;
if (severity.test(selector) && hues.test(body)) {
offenders.push(`${selector.trim().split('\n').pop().trim()} -> ${body.match(hues)[0]}`);
}
}
expect(offenders).toEqual([]);
});
});
describe('nothing paints a label\'s text with an inline colour', () => {
/*
* A source guard, because this defect appeared at three separate call sites and fixing the

26
app/src/themes/tokens.css
View file

@ -72,16 +72,24 @@ html[data-theme="light"] {
* between hue names there is choosing between synonyms. That is why all four toast
* kinds looked the same in night, and why a warning and an error were the same pixel.
*
* The ramps below are built so that in every theme:
* - the order neutral < info < success < warning < danger is strictly increasing in
* contrast against the panel, so severity reads as brightness even without hue;
* - every role except neutral clears 3:1 against the panel;
* - danger reaches that theme's own `--text` brightness, so the loudest role is
* never quieter than ordinary prose;
* - neutral sits deliberately below the rest -- for `pending` and disabled, being
* dim IS the signal.
* In EVERY theme:
* - the five roles resolve to five distinct colours;
* - every role except neutral clears 3:1 against the panel. Neutral sits below the
* rest deliberately -- for `pending` and disabled, being dim IS the signal.
*
* `ui-layout.cy.js` asserts all of that, per theme, in a real browser.
* In the MONOCHROME themes only (night and amber):
* - the order neutral < info < success < warning < danger is strictly increasing in
* contrast against the panel, so severity reads as brightness with no hue to help;
* - danger reaches that theme's own `--text` brightness, so the loudest role is never
* quieter than ordinary prose.
*
* Those last two are NOT true of light and dark and must not be made true. There a role
* is a hue, and the contrast order is an accident of which hues were chosen -- blue is
* darker than green on white, so light "fails" a brightness ramp while looking perfectly
* correct. Reordering light's hues to satisfy a ramp would be changing the design to
* satisfy a test.
*
* `ui-layout.cy.js` asserts exactly this split, per theme, in a real browser.
*
* `--primary` is deliberately NOT one of these. "The colour of a primary button" and
* "the colour that means informational" are different jobs, and a user who recolours