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: a busy button is not a disabled one, and night's danger is not red

Browse source 
Acting on the review of #624.  Two bugs, both real, both confirmed in the
browser against the live stylesheet before fixing.

Mantine renders a loading button as `disabled={disabled || loading}`, so every
busy control in the app is `:disabled` -- an APIButton mid-request, the refresh
button, the flasher's connect.  Mantine's own grey repaint is careful to exclude
`[data-loading]` for exactly that reason; the new rule was not, so "working..."
faded to 0.45 and read as unavailable rather than active.  Measured: a loading
button came back at opacity 0.45.  Both roots now mirror Mantine's exclusion.

Night encodes danger as a dashed border on a transparent fill, because it has no
second hue to spend.  The general disabled rule repaints from `--button-bg` --
the filled red Mantine leaves in the variable -- and at (0,2,1) it tied with
`html[data-theme="night"] .wrolpi-button-danger` and won on source order.
Measured: background rgb(255,87,87), border solid transparent.  So the file
browser's Delete, which is role='danger' and disabled until a selection exists,
became a solid red chip in the one theme where that is not what danger means.
Restated at (0,3,1) so it no longer depends on where it sits in the file.

The specificity note in the comment was also wrong, and wrong in a way that
would mislead the next person: Mantine's disabled paint is
`:where(:disabled:not([data-loading]), ...)`, and `:where()` contributes
nothing, so it scores (0,1,0).  The `html` prefix is not needed to beat that --
it is needed to stay level with the app's own `html ...` overrides, and the
night danger collision is the demonstration.

The fade is now `--disabled-opacity`, read by the tests instead of restated, and
asserted for equality rather than against a band -- a band tolerates an
accidental different fade or an opacity leaking in from elsewhere and still
reports success.

Five new cases from the review: loading versus disabled (paired, so it cannot
pass by nothing fading at all), night's dashed danger with a light-theme inverse
proving the fill is not stripped everywhere, cross-colour distinctness in all
four themes rather than only light -- night and amber are where one hue makes
collapse most likely -- and an outline pair checking text and border, which are
the only things a fill-less variant has to lose.  Each planted: dropping the
loading exclusion reddens one, dropping the night restatement reddens one.

1128 jest / 63 suites, 216 in ui-layout.cy.js, clean npm run build.
This commit is contained in:
Roland 2026-08-02 16:53:18 -06:00
parent bb65cd0c28
commit e28c5740ce
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
2 changed files with 151 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -2892,35 +2892,133 @@ describe('a disabled button keeps its own colour', () => {
});
it(`still marks them as unavailable in ${theme}`, () => {
// Keeping the colour must not cost the affordance: a disabled button that looks
// identical to an enabled one is a worse bug than a grey one.
/*
* Keeping the colour must not cost the affordance: a disabled button that looks
* identical to an enabled one is a worse bug than a grey one.
*
* Against `--disabled-opacity` rather than a band. A band tolerates an
* accidentally different fade, or an opacity leaking in from somewhere else, and
* still reports success -- and the fade is an authored value, so it can be read.
*/
mountPairs(theme);
cy.get('[data-testid="off-red"]').should(($off) => {
const authored = parseFloat(getComputedStyle(document.documentElement)
.getPropertyValue('--disabled-opacity'));
const disabled = parseFloat(getComputedStyle($off[0]).opacity);
const enabled = parseFloat(
getComputedStyle(Cypress.$('[data-testid="on-red"]')[0]).opacity);
expect(authored, 'the token resolved').to.be.within(0.1, 0.9);
expect(enabled, `${theme} enabled is solid`).to.equal(1);
expect(disabled, `${theme} disabled is faded`).to.be.lessThan(0.7);
expect(disabled, `${theme} disabled is still legible`).to.be.greaterThan(0.25);
expect(disabled, `${theme} disabled matches the token`).to.equal(authored);
});
});
});
it('tells two disabled buttons of different colours apart', () => {
themeNames.forEach((theme) => {
it(`tells two disabled buttons of different colours apart in ${theme}`, () => {
/*
* The claim stated as the user meets it, and the inverse of the bug: five grey
* blobs were five EQUAL greys. Comparing each against its enabled twin above
* would still pass if every pair were grey, so this compares them to each other.
*
* Every theme, not just light. Night and amber are where it is both most
* valuable and most fragile: one hue means the five differ only in brightness,
* so if any theme is going to collapse them back together it is those two.
*/
mountPairs(theme);
cy.get('[data-testid^="off-"]').should(($disabled) => {
const fills = [...$disabled]
.map(button => getComputedStyle(button).backgroundColor);
expect(new Set(fills).size,
`${theme}: five colours, distinct fills: ${fills.join(' ')}`)
.to.equal(pairs.length);
});
});
});
it('does not fade a button that is merely busy', () => {
/*
* The claim stated as the user meets it, and the inverse of the bug: five grey blobs
* were five EQUAL greys. Comparing each against its enabled twin above would still
* pass if every pair were grey, so this compares them against each other.
* Mantine renders a loading button as `disabled={disabled || loading}`, so every busy
* control in the app is `:disabled` -- an APIButton mid-request, refresh, the
* flasher's connect. Mantine's own grey repaint excludes `[data-loading]` for
* exactly this reason, and the first version of this rule did not, so "working..."
* faded to 0.45 and read as unavailable rather than active.
*/
mountPairs('light');
cy.mountUI(<div>
<Button color='blue' loading data-testid='busy'>Saving</Button>
<Button color='blue' disabled data-testid='off'>Save</Button>
<IconButton icon='trash' label='Deleting' color='red' variant='filled' loading
data-testid='busy-icon'/>
</div>);
cy.get('[data-testid^="off-"]').should(($disabled) => {
const fills = [...$disabled].map(button => getComputedStyle(button).backgroundColor);
cy.get('[data-testid="busy"]').should(($busy) => {
expect(parseFloat(getComputedStyle($busy[0]).opacity), 'a busy button is solid')
.to.equal(1);
// Paired with the disabled one, so this cannot pass by nothing fading at all.
expect(parseFloat(getComputedStyle(Cypress.$('[data-testid="off"]')[0]).opacity),
'while a disabled one beside it still fades').to.be.lessThan(1);
expect(parseFloat(getComputedStyle(Cypress.$('[data-testid="busy-icon"]')[0]).opacity),
'and the same for an icon button').to.equal(1);
});
});
expect(new Set(fills).size, `five colours, distinct fills: ${fills.join(' ')}`)
.to.equal(pairs.length);
it('keeps night\'s dashed danger treatment when disabled', () => {
/*
* Night has no second hue, so danger is a dashed border on a transparent fill rather
* than a red one. The general disabled rule repaints from `--button-bg` -- the
* filled red Mantine leaves in the variable -- and at equal specificity it won on
* source order, turning the file browser's disabled Delete into a solid red chip in
* the one theme where that is not what danger means.
*/
cy.mountUI(<div>
<Button role='danger' disabled data-testid='danger-off'>Delete</Button>
</div>, {theme: 'night'});
cy.get('[data-testid="danger-off"]').should(($button) => {
const style = getComputedStyle($button[0]);
expect(style.backgroundColor, 'no fill').to.equal('rgba(0, 0, 0, 0)');
expect(style.borderStyle, 'still dashed').to.equal('dashed');
expect(toRgb(style.borderTopColor), 'in the danger colour')
.to.equal(toRgb(style.getPropertyValue('--danger') || '#ff5757'));
expect(parseFloat(style.opacity), 'and still faded').to.be.lessThan(1);
});
});
it('is a real constraint: light DOES fill a disabled danger button', () => {
// Without this the case above would also pass on a rule that stripped the fill from
// every disabled danger button in every theme, which would break the other three.
cy.mountUI(<div>
<Button role='danger' disabled data-testid='danger-off'>Delete</Button>
</div>, {theme: 'light'});
cy.get('[data-testid="danger-off"]').should(($button) => {
expect(getComputedStyle($button[0]).backgroundColor, 'light danger is filled')
.to.not.equal('rgba(0, 0, 0, 0)');
});
});
it('restores text and border on variants that carry no fill', () => {
/*
* `default` and `outline` buttons say what they are with their text and border, not
* their background, so a background-only check would pass on them while they were
* still painted Mantine's disabled grey.
*/
cy.mountUI(<div>
<Button variant='outline' color='red' data-testid='outline-on'>Delete</Button>
<Button variant='outline' color='red' disabled data-testid='outline-off'>Delete</Button>
</div>);
cy.get('[data-testid="outline-off"]').should(($off) => {
const on = getComputedStyle(Cypress.$('[data-testid="outline-on"]')[0]);
const off = getComputedStyle($off[0]);
expect(off.color, 'text colour survives').to.equal(on.color);
expect(off.borderTopColor, 'border colour survives').to.equal(on.borderTopColor);
});
});

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

@ -1193,6 +1193,9 @@ html[data-theme="amber"] .wrolpi-progress-fill {
*/
:root {
--card-poster-max-height: 200px;
/* How far a disabled control fades. Semantic used 0.45 and it reads correctly against
every theme's panel; the tests read this rather than repeating the number. */
--disabled-opacity: 0.45;
}
/*
@ -1476,21 +1479,50 @@ html[data-theme="amber"] .wrolpi-th-sort:focus-visible {
* the call site asked for is still in `--button-bg` and can simply be reinstated. `--button-bd`
* is a whole border shorthand, not a colour, so it is set with `border`.
*
* The `html` prefix beats Mantine's own `.m_hash:disabled` at (0,2,0) without needing
* `!important`.
* `:not([data-loading])` mirrors Mantine, and is not decoration. Mantine renders a loading
* button as `disabled={disabled || loading}`, so every busy control in the app is `:disabled`
* -- every APIButton mid-request, the refresh button, the flasher's connect. Its own grey
* repaint is careful to exclude them; without the same exclusion here, "working..." fades to
* 0.45 and reads as unavailable rather than busy.
*
* On specificity: Mantine's disabled paint is
* `.m_hash:where(:disabled:not([data-loading]), ...)`, and `:where()` contributes nothing, so
* it scores (0,1,0) -- a single class. The `html` prefix is not needed to beat THAT. It is
* needed to stay level with the app's other `html ...` overrides, and the night danger rule
* below shows why that matters: at equal specificity the later rule wins, which is how this
* one silently repainted night's dashed danger buttons until it was told not to.
*/
html .mantine-Button-root:disabled,
html .mantine-Button-root[data-disabled] {
html .mantine-Button-root:disabled:not([data-loading]),
html .mantine-Button-root[data-disabled]:not([data-loading]) {
background: var(--button-bg);
color: var(--button-color);
border: var(--button-bd);
opacity: 0.45;
opacity: var(--disabled-opacity);
}
html .mantine-ActionIcon-root:disabled,
html .mantine-ActionIcon-root[data-disabled] {
html .mantine-ActionIcon-root:disabled:not([data-loading]),
html .mantine-ActionIcon-root[data-disabled]:not([data-loading]) {
background: var(--ai-bg);
color: var(--ai-color);
border: var(--ai-bd);
opacity: 0.45;
opacity: var(--disabled-opacity);
}
/*
* Night keeps its dashed danger treatment when disabled.
*
* Night has no second hue to spend, so danger is encoded by a dashed border on a transparent
* fill rather than by a red fill. The general rule above repaints from `--button-bg`, which
* is the filled red Mantine put in the variable, and at (0,2,1) it tied with the night rule
* and won on source order -- turning the file browser's disabled Delete into a solid red chip
* in the one theme where a solid red chip is not what danger looks like.
*
* (0,3,1), so it does not depend on where it sits in the file. No `background`/`color`
* shorthand games: it restates the night treatment, and the fade comes from the rule above.
*/
html[data-theme="night"] .wrolpi-button-danger:disabled:not([data-loading]),
html[data-theme="night"] .wrolpi-button-danger[data-disabled]:not([data-loading]) {
background: transparent;
color: var(--danger);
border: 1.5px dashed var(--danger);
}