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