From c7fddc55af53ddd5a93b4aaa480aa3d52fb3db99 Mon Sep 17 00:00:00 2001 From: Roland Date: Sat, 1 Aug 2026 17:46:33 -0600 Subject: [PATCH] =?UTF-8?q?test:=20act=20on=20review=20=E2=80=94=20and=20c?= =?UTF-8?q?orrect=20an=20overstated=20claim=20about=20the=20toast=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/src/components/ui/ui-layout.cy.js | 98 +++++++++++++++++++++++++-- app/src/components/ui/ui.css | 27 +++++--- app/src/components/ui/ui.test.js | 31 +++++++++ app/src/themes/tokens.css | 26 ++++--- 4 files changed, 155 insertions(+), 27 deletions(-) diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index b633860c..4e086759 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -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(<> + + + , {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( + + + + + + , + {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 diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index db428889..2db28d2b 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -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. */ diff --git a/app/src/components/ui/ui.test.js b/app/src/components/ui/ui.test.js index 55183eea..eb7d2663 100644 --- a/app/src/components/ui/ui.test.js +++ b/app/src/components/ui/ui.test.js @@ -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 diff --git a/app/src/themes/tokens.css b/app/src/themes/tokens.css index 0e2fc51d..ac2597cb 100644 --- a/app/src/themes/tokens.css +++ b/app/src/themes/tokens.css @@ -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