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

review: the color-scheme half of this was already there

Browse source 
Acting on review of #644.

The largest finding came out of a suggestion rather than being in it.  The
reviewer asked for coverage of the `color-scheme: dark` rule, on the grounds
that dropping it would leave night's track wrong with no failing test.  Written
and passing -- then mutated, by pointing the selector at a type that does not
exist, and the tests stayed green.

Mantine's own stylesheet puts `color-scheme: var(--mantine-color-scheme)` on
`:root`, and ThemeProvider drives it with `forceColorScheme`.  The property is
inherited, so a range slider on a dark theme has had a dark track all along and
the rule added here changed no pixel.  It is deleted, and the claim about "the
UA's pale slab" is out of the comments and the gallery copy with it.

The tests stay, re-aimed at what actually supplies it: breaking
`forceColorScheme` fails all three, which is worth guarding because it lives in
another module and every accent assertion passes without it.

The other suggestions:

- The gallery slider misrepresented the control it stands for -- it invented an
  "Error" step, dropped Trace and ran 0..4, where Settings maps 1..5 onto
  Critical / Warning / Info / Debug / Trace.  A gallery whose purpose is showing
  the real control is worse than useless when it shows a different one.  Keyed
  to match `levelNameMap` exactly, and it starts at Info so both halves of the
  track are visible.

- The "never leaves it at the UA default" test restated the four above it:
  `toRgb` throws on anything unresolved, and the computed value of
  `accent-color: auto` is the keyword, so those already fail on an unstyled
  slider.  Replaced with a constraint they do not cover -- `accent-color` is
  inherited, so a text field beside the slider must still read `auto`, which is
  what says the accent comes from this rule and not from a tinted document.

- The comment implied native checkboxes and radios are on screen the way
  sliders are.  They are not: those are Mantine's and Progress is a div, so
  three of the four selectors are a guard rather than a fix.  Said so.

Nits: four product sliders, not five -- the fifth is the sample itself.  And
the progressive-enhancement limit is now stated where the rule is, including
what a complete fix would take.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-08-05 19:51:29 -06:00
parent e15e2fd993
commit 5873e65e8c
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
3 changed files with 97 additions and 51 deletions
Download patch file Download diff file Expand all files Collapse all files

30
app/src/components/ThemeSamplePage.js
View file

@ -78,9 +78,15 @@ const Row = ({children}) => <div style={{display: 'flex', gap: 10, flexWrap: 'wr
{children}
</div>;
// 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 `<datalist>` 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() {
<input
type='range'
id='sample_range'
min={0}
max={4}
min={1}
max={5}
step={1}
value={logLevel}
onChange={e => setLogLevel(parseInt(e.target.value))}
@ -730,9 +737,10 @@ export function ThemeSamplePage() {
<p style={{fontSize: '0.75rem', color: 'var(--muted)', marginTop: 8, marginBottom: 0}}>
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 <code>--blue</code> 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 <code>--blue</code> 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.
</p>
</Panel>
</Section>

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

@ -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 `<input type="range">` 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(
<input type='range' min={1} max={5} defaultValue={3} data-testid='slider'/>,
@ -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(<div>
<input type='range' data-testid='slider'/>
<input type='text' data-testid='text' readOnly value='not an accented control'/>
</div>, {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', () => {

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

@ -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;