diff --git a/app/src/components/ThemeSamplePage.js b/app/src/components/ThemeSamplePage.js index 2d669481..9a09f4ad 100644 --- a/app/src/components/ThemeSamplePage.js +++ b/app/src/components/ThemeSamplePage.js @@ -78,9 +78,15 @@ const Row = ({children}) =>
; -// The labels Settings shows beside its own log-level slider, so the sample reads like the -// control it stands in for rather than a bare number. -const logLevelNames = ['Critical', 'Error', 'Warning', 'Info', 'Debug']; +/* + * The labels Settings shows beside its own log-level slider, so the sample reads like the + * control it stands in for rather than a bare number. + * + * Keyed 1..5 to match `levelNameMap` and the `` in Settings.js exactly. The first + * version of this invented an "Error" step, dropped Trace, and ran 0..4 -- a gallery whose + * job is to show the real control misrepresenting it is worse than not showing it. + */ +const logLevelNames = {1: 'Critical', 2: 'Warning', 3: 'Info', 4: 'Debug', 5: 'Trace'}; /* * One navigation bar, in one of the twelve colors a user can pick for it. @@ -153,9 +159,10 @@ export function ThemeSamplePage() { const [wideConfirmOpen, setWideConfirmOpen] = useState(false); const [comments, setComments] = useState(true); const [hotspot, setHotspot] = useState(true); - // Draggable, because the accent only shows on the FILLED half of the track -- a slider - // parked at either end shows almost none of the color this section is here to check. - const [logLevel, setLogLevel] = useState(2); + // Draggable, and starting at Info rather than at either end: the accent only shows on the + // FILLED half of the track, so a slider parked at a limit shows almost none of the color + // this section is here to check, or none of the unfilled track beside it. + const [logLevel, setLogLevel] = useState(3); const [dismissed, setDismissed] = useState(false); const [modalOpen, setModalOpen] = useState(false); const [realColors, setRealColors] = useState(false); @@ -719,8 +726,8 @@ export function ThemeSamplePage() { setLogLevel(parseInt(e.target.value))} @@ -730,9 +737,10 @@ export function ThemeSamplePage() {

A range slider is drawn by the browser, not by us — so with nothing said about it the UA paints it in its own fixed blue, which no theme can reach. - Check it in night: the accent follows --blue like a link does, - and the unfilled half of the track is dark rather than the UA's pale slab. - Settings and three of the calculators each have one. + Check it in night: the accent follows --blue like a link does. + The accent reaches the thumb and the filled half only; the rest of the + track comes from the browser's own dark palette, which the theme already + selects. Settings and three of the calculators each have one.

diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index 6b36f97f..219f22f6 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -9,7 +9,7 @@ import {VideoCard} from '../Videos'; import {CardPoster, contrastingColor, HelpHeader, LoadStatistic} from '../Common'; import {Notifications} from '@mantine/notifications'; import {clearToasts, toast} from './toast'; -import {monochromeThemes, themeNames} from '../../themes/names'; +import {isDarkTheme, monochromeThemes, themeNames} from '../../themes/names'; import {navColorNames} from '../../themes/navColors'; import {NavBarSample} from '../ThemeSamplePage'; import {DesktopNav, NavIconWrapper} from '../Nav'; @@ -2670,17 +2670,18 @@ describe('a card reads as titles, not as a wall of links', () => { describe('a range slider is painted by the theme, not by the browser', () => { /* * A native `` is drawn by the UA, and with nothing said about it the - * UA uses its own accent -- a fixed blue that no theme can reach. Five of them are on - * screen: the log level on Settings, and the duration sliders in the food, water and - * ration calculators. + * UA uses its own accent -- a fixed blue that no theme can reach. Four are in the product: + * the log level on Settings, and the duration sliders in the food, water and ration + * calculators. (A fifth is on /theme-sample.) * * Night is where this is not merely inconsistent but wrong. The theme exists so a user * reading in the dark keeps their adaptation, and every other token is a red; a slider * arriving in the browser's blue is the brightest non-red thing on the page, on the one * control a user drags back and forth while watching it. * - * `accent-color` takes `--blue` for the same reason links do: it remaps per theme, so - * night gets the dim red and amber the amber with no second rule. + * `accent-color` takes `--blue` for the same reason links do: it remaps per theme. It + * reaches the thumb and the FILLED half of the track; the rest of the track comes from + * the UA palette `color-scheme` selects, which is asserted separately below. */ const mountSlider = (theme) => cy.mountUI( , @@ -2694,29 +2695,71 @@ describe('a range slider is painted by the theme, not by the browser', () => { cy.get('[data-testid="slider"]').should(($slider) => { const expected = toRgb(getComputedStyle(document.documentElement) .getPropertyValue('--blue')); + /* + * `toRgb` THROWS on anything that is not a resolved color, and the computed + * value of `accent-color: auto` is the keyword `auto` -- so this assertion + * already fails on an unstyled slider rather than quietly comparing keywords. + * That is why there is no separate "not auto" test: it would restate this one. + */ expect(toRgb(getComputedStyle($slider[0]).accentColor), `accent in ${theme}`) .to.equal(expected); }); }); }); - it('never leaves it at the UA default, which is what night could not tolerate', () => { + it('is a real constraint: a control the rule does not name keeps the UA default', () => { /* - * The assertions above compare against a token, so they would also pass if the - * accent were `auto` and the token happened to resolve to the same string. This - * states the failure directly: whatever the slider is accented with, it is a real - * color and not the keyword that hands the decision back to the browser. + * `accent-color` is an inherited property, so this also states where the accent comes + * from: the rule names the control types it paints, rather than being set high up and + * inherited by everything. Without this the tests above would pass just as well on a + * page that had tinted the entire document. */ - monochromeThemes.forEach((theme) => { + cy.mountUI(
+ + +
, {theme: 'night'}); + + cy.get('[data-testid="text"]').should(($text) => { + expect(getComputedStyle($text[0]).accentColor, 'a text field is untouched') + .to.equal('auto'); + }); + }); + + /* + * The rest of the track, which `accent-color` does not reach. + * + * The unfilled half and the tick marks are drawn from the UA palette that `color-scheme` + * selects. This PR originally added a `color-scheme: dark` rule for it -- mutating that + * rule to match nothing left these tests green, which is how it emerged that Mantine + * already puts `color-scheme: var(--mantine-color-scheme)` on `:root` and ThemeProvider + * drives it with `forceColorScheme`. The property is inherited, so the control had it + * all along and the rule was deleted. + * + * These stay, because that inheritance is load-bearing and belongs to another module: + * drop `forceColorScheme` from ThemeProvider and night's slider goes back to a pale slab + * behind the thumb, with every accent assertion above still passing. + */ + themeNames.filter(isDarkTheme).forEach((theme) => { + it(`darkens the rest of the track in ${theme}`, () => { mountSlider(theme); cy.get('[data-testid="slider"]').should(($slider) => { - const accent = getComputedStyle($slider[0]).accentColor; - expect(accent, `accent in ${theme}`).to.not.equal('auto'); - expect(() => toRgb(accent), `a resolved color in ${theme}`).to.not.throw(); + expect(getComputedStyle($slider[0]).colorScheme, `color-scheme in ${theme}`) + .to.equal('dark'); }); }); }); + + it('is a real constraint: light leaves the track alone', () => { + // The dark palette follows the theme rather than being forced everywhere: on a light + // page the UA's own is already correct. + mountSlider('light'); + + cy.get('[data-testid="slider"]').should(($slider) => { + expect(getComputedStyle($slider[0]).colorScheme, 'color-scheme in light') + .to.not.equal('dark'); + }); + }); }); describe('a panel wrapped in a link does not read as a link', () => { diff --git a/app/src/themes/tokens.css b/app/src/themes/tokens.css index b2dd9713..a3898724 100644 --- a/app/src/themes/tokens.css +++ b/app/src/themes/tokens.css @@ -321,10 +321,9 @@ a:has(img, svg, .wrolpi-card-icon):hover { /* * Native controls the browser paints itself. * - * A range slider, a checkbox and a radio are drawn by the UA, and with nothing said about - * them the UA uses its own accent -- a fixed blue no theme can reach. Five sliders are on - * screen: the log level on Settings, and the duration sliders in the food, water and ration - * calculators. + * A range slider is drawn by the UA, and with nothing said about it the UA uses its own + * accent -- a fixed blue no theme can reach. Four are in the product: the log level on + * Settings, and the duration sliders in the food, water and ration calculators. * * Night is where that is not merely inconsistent. The theme exists so a user reading in * the dark keeps their adaptation and every token is a red, so a slider arriving in the @@ -333,6 +332,23 @@ a:has(img, svg, .wrolpi-card-icon):hover { * * `--blue` for the same reason links take it: it remaps per theme, so night gets the dim * red and amber the amber without a second rule. + * + * Checkbox, radio and `progress` are the other three types `accent-color` paints, and are + * listed for completeness rather than because they are on screen: the app's checkboxes and + * radios are Mantine's, which draw their own box from tokens, and `Progress` is a div. So + * those three are a guard for the day a native one appears, not a fix for anything visible + * today -- the slider is the one that was actually wrong. + * + * `accent-color` reaches the thumb and the FILLED half of the track and nothing else. The + * rest of the track comes from the UA palette that `color-scheme` selects -- which is + * already dark on the dark themes, because Mantine's stylesheet puts + * `color-scheme: var(--mantine-color-scheme)` on `:root` and ThemeProvider drives it with + * `forceColorScheme`. That property is inherited, so a control needs nothing of its own; a + * rule here restating it was verified to change no pixel and is not present. + * + * Progressive enhancement: `accent-color` wants Safari 15.4+ / Chrome 93+ / Firefox 92+, and + * an older client keeps the UA blue, which is what it had before this rule. A complete fix + * would mean `::-webkit-slider-runnable-track` and `::-moz-range-track` rules of our own. */ input[type="range"], input[type="checkbox"], @@ -341,27 +357,6 @@ progress { accent-color: var(--blue); } -/* - * The half of a slider's track that is NOT filled, and the tick marks beside it, are drawn - * from the UA's own light palette -- `accent-color` reaches the thumb and the filled half - * and nothing else. On a dark ground that leaves a pale slab behind the thumb. - * - * Scoped to the controls rather than declared on `html`: `color-scheme` also governs - * scrollbars, spinners and every other piece of browser chrome on the page, which is a - * larger change than this and one to make deliberately rather than as a side effect. - */ -html[data-theme="dark"] input[type="range"], -html[data-theme="night"] input[type="range"], -html[data-theme="amber"] input[type="range"], -html[data-theme="dark"] input[type="checkbox"], -html[data-theme="night"] input[type="checkbox"], -html[data-theme="amber"] input[type="checkbox"], -html[data-theme="dark"] input[type="radio"], -html[data-theme="night"] input[type="radio"], -html[data-theme="amber"] input[type="radio"] { - color-scheme: dark; -} - a:focus-visible { outline: 2px solid var(--blue); outline-offset: 2px;