From bb65cd0c283ab04aae715de8bc6b81024077a552 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 16:38:41 -0600 Subject: [PATCH] ui: a disabled button keeps its own colour The file browser's footer is eight buttons, six of them disabled until something is selected, so the toolbar a user meets on arriving at /files was an undifferentiated grey row: Delete, Rename, Move, Ignore and Tag rendered as five identical pixels. Colour is how those are told apart at a glance. Mantine repaints a disabled button flat grey -- background, text and border all replaced with `--mantine-color-disabled`. Semantic kept the colour and dropped the whole control to `opacity: 0.45`; measured on the QA Pi, a disabled Delete is still red at 0.45, Rename still yellow, Tag still violet. That is the better signal: it says "not available" without also saying "no longer the delete button". Mantine leaves the variables alone and overrides only the painted properties, so the colour the call site asked for is still sitting in `--button-bg` and can simply be reinstated. Applies to every disabled Button and ActionIcon, not only the file browser's -- it is one convention, and having two would be worse than either. A pre-existing test earned its keep here: the IconStack corner disc was pinned to `--mantine-color-disabled` so it would follow a disabled button's grey, and it reddened the moment the button stopped being grey. That override is gone; `--button-bg` is now correct in both states, so the general rule covers them. Ten new cases. Fill equality is checked against each button's own enabled twin in all four themes, and separately across the five colours at once -- comparing each against its twin would still pass if every pair were grey, which is the actual bug. Opacity is asserted from both sides, because keeping the colour must not cost the affordance. Removing the rule reddens eleven. 1128 jest / 63 suites, 209 in ui-layout.cy.js, clean npm run build. --- app/src/components/ui/ui-layout.cy.js | 117 +++++++++++++++++++++++++- app/src/components/ui/ui.css | 49 +++++++++-- 2 files changed, 157 insertions(+), 9 deletions(-) diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index c1371943..ece9aef4 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -1530,10 +1530,13 @@ describe('an icon stack carries the surface it was placed on', () => { it('follows a filled button when it is disabled', () => { /* - * The file browser's New Folder button is disabled in WROL mode, and Mantine swaps a - * disabled button's fill for --mantine-color-disabled. A disc pinned to `--blue` - * then becomes a blue chip on a grey surface -- the same defect as the original - * hole, inverted. The disc has to follow the button, not the button's enabled fill. + * The file browser's New Folder button is disabled in WROL mode. The disc has to + * follow the button into that state, whatever the state happens to look like -- and + * it has now looked like two different things. Mantine used to repaint a disabled + * button with `--mantine-color-disabled`, so a disc pinned to `--blue` became a blue + * chip on a grey surface; disabled buttons keep their own fill now, so the disc must + * be blue again. This caught the disc still pinned to the old grey the moment that + * changed, which is exactly what it is for. */ cy.mountUI( + + )} + , {theme}); + + themeNames.forEach((theme) => { + it(`keeps every colour recognisable while disabled in ${theme}`, () => { + mountPairs(theme); + + cy.get('[data-testid^="off-"]').should(($disabled) => { + expect($disabled, 'a disabled button per colour').to.have.length(pairs.length); + + pairs.forEach(({color}) => { + const on = Cypress.$(`[data-testid="on-${color}"]`)[0]; + const off = Cypress.$(`[data-testid="off-${color}"]`)[0]; + + expect(getComputedStyle(off).backgroundColor, `${theme}/${color} fill`) + .to.equal(getComputedStyle(on).backgroundColor); + }); + }); + }); + + 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. + mountPairs(theme); + + cy.get('[data-testid="off-red"]').should(($off) => { + const disabled = parseFloat(getComputedStyle($off[0]).opacity); + const enabled = parseFloat( + getComputedStyle(Cypress.$('[data-testid="on-red"]')[0]).opacity); + + 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); + }); + }); + }); + + it('tells two disabled buttons of different colours apart', () => { + /* + * 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. + */ + mountPairs('light'); + + cy.get('[data-testid^="off-"]').should(($disabled) => { + const fills = [...$disabled].map(button => getComputedStyle(button).backgroundColor); + + expect(new Set(fills).size, `five colours, distinct fills: ${fills.join(' ')}`) + .to.equal(pairs.length); + }); + }); + + it('does the same for icon-only buttons', () => { + // The file browser's row actions are IconButtons, and they disable the same way. + cy.mountUI(
+ {/* Filled, so there is a colour to lose. IconButton defaults to Mantine's + `default` variant, which is a white face, and comparing white with white + would prove nothing. */} + + +
); + + cy.get('[data-testid="icon-off"]').should(($off) => { + const on = Cypress.$('[data-testid="icon-on"]')[0]; + + expect(getComputedStyle($off[0]).backgroundColor, 'icon button keeps its fill') + .to.equal(getComputedStyle(on).backgroundColor); + expect(parseFloat(getComputedStyle($off[0]).opacity), 'and is faded') + .to.be.lessThan(0.7); + }); + }); +}); diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index 259ba06e..e35df77e 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -54,7 +54,7 @@ /* * A stack inside a button is the one case the call site should not have to describe, * because the surface changes underneath it: Mantine paints a filled button from - * `--button-bg` but a disabled one from `--mantine-color-disabled`, whatever its variant. + * `--button-bg`, and the disc has to follow it into whatever state the button is in. * The file browser's New Folder button is disabled in WROL mode, so a disc pinned to the * enabled fill would be a blue chip on a grey button -- the original hole, inverted. * @@ -70,10 +70,13 @@ background: var(--button-bg, var(--icon-stack-bg, var(--bg))); } -.mantine-Button-root:disabled .wrolpi-icon-stack-corner, -.mantine-Button-root[data-disabled] .wrolpi-icon-stack-corner { - background: var(--mantine-color-disabled); -} +/* + * There is deliberately no `:disabled` override here any more. There used to be one pinning + * the disc to `--mantine-color-disabled`, because Mantine repainted a disabled button grey + * and a disc left on `--blue` became a blue chip on a grey surface. Disabled buttons now + * keep their own fill (see the disabled-button rule further down), so `--button-bg` is + * correct in both states and the rule above covers them. + */ /* ---------------------------------------------------------------- Buttons */ /* @@ -1455,3 +1458,39 @@ html[data-theme="amber"] .wrolpi-th-sort:focus-visible { .wrolpi-table-scroll { overflow-x: auto; } + +/* + * A disabled button keeps its own colour. + * + * Mantine repaints one flat grey -- background, text and border all replaced with + * `--mantine-color-disabled`. Semantic kept the colour and dropped the whole control to + * `opacity: 0.45`, and that is the better signal: it says "not available" without also + * saying "no longer the delete button". + * + * The file browser is where the difference shows. Its footer is eight buttons, six of them + * disabled until something is selected, so the toolbar a user meets on arriving at /files was + * an undifferentiated grey row -- Delete, Rename, Move, Ignore and Tag all the same pixel. + * Colour is how those are told apart at a glance. + * + * Mantine leaves the variables alone and overrides only the painted properties, so the colour + * 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`. + */ +html .mantine-Button-root:disabled, +html .mantine-Button-root[data-disabled] { + background: var(--button-bg); + color: var(--button-color); + border: var(--button-bd); + opacity: 0.45; +} + +html .mantine-ActionIcon-root:disabled, +html .mantine-ActionIcon-root[data-disabled] { + background: var(--ai-bg); + color: var(--ai-color); + border: var(--ai-bd); + opacity: 0.45; +}