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; +}