composer: a stacked modal dimmed twice, through a viewport-sized shadow
Every modal already mounts a .cx-backdrop element over its layer, so the stacked variant painting another one as `0 0 0 100vmax` of shadow spread was redundant, and the area beneath it was darkened twice. It was also the most expensive declaration in the sheet: a spread the size of the viewport diagonal, on an element inside a fixed layer, with an entry animation running. Removed in favour of a bounded shadow, and a test reads the stylesheet for the pattern -- asserting on node.style in jsdom passes against any rule at all, since no CSS is applied there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3a1c76d64e
commit
34a55c4154
@@ -14,7 +14,20 @@
|
||||
box-shadow: var(--shadow-3);
|
||||
animation: cx-pop var(--dur-2) var(--ease-standard);
|
||||
}
|
||||
.cx-modal-stacked { box-shadow: var(--shadow-3), 0 0 0 100vmax var(--backdrop); }
|
||||
/* A stacked modal gets a deeper shadow and nothing else.
|
||||
*
|
||||
* It used to paint its own backdrop as `0 0 0 100vmax` of shadow spread, which
|
||||
* was redundant -- every modal, stacked or not, already mounts a real
|
||||
* .cx-backdrop element that covers the layer -- so the area under a stacked
|
||||
* modal was dimmed twice. It was also the most expensive declaration in this
|
||||
* sheet by a distance: a shadow spread the size of the viewport diagonal, on an
|
||||
* element inside a fixed layer, with an entry animation running. That is a
|
||||
* standing invitation to a stale compositor layer, and one showed up as the
|
||||
* whole app painted twice, side by side, until a window resize forced a
|
||||
* repaint. */
|
||||
.cx-modal-stacked {
|
||||
box-shadow: 0 20px 48px color-mix(in srgb, var(--shadow-ink) 90%, transparent);
|
||||
}
|
||||
@keyframes cx-pop { from { opacity: 0; transform: translateY(-6px) scale(.99); } }
|
||||
|
||||
.cx-modal-head {
|
||||
|
||||
@@ -1,8 +1,14 @@
|
||||
// Overlay behaviour: stacking, dismissal order, and the promises.
|
||||
|
||||
import { readFileSync } from "node:fs";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { dirname, join } from "node:path";
|
||||
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
const here = dirname(fileURLToPath(import.meta.url));
|
||||
|
||||
import { setupDom, teardownDom, key, fire } from "./_dom.js";
|
||||
import { EDGES, sectorAt, sectorInArc } from "../elements/wheel.js";
|
||||
|
||||
@@ -566,3 +572,35 @@ test("every overlay keeps its z-index bound, not just windows", () => {
|
||||
second.close();
|
||||
anchor.destroy();
|
||||
});
|
||||
|
||||
test("a stacked modal dims what is under it with a real backdrop, once", () => {
|
||||
// The dimming used to come from TWO places at once: the layer's own backdrop
|
||||
// element, and a box-shadow with a 100vmax spread on the card. So the area
|
||||
// under a stacked modal was darkened twice, and a shadow the size of the
|
||||
// viewport diagonal -- on an element in a fixed layer, with an entry
|
||||
// animation running -- is how a stale compositor layer gets made. One showed
|
||||
// up as the whole app painted twice until a resize forced a repaint.
|
||||
const under = composer.modal.generic({ title: "Under", body: composer.text.sm({ text: "a" }) });
|
||||
const over = composer.modal.stacked({ title: "Over", body: composer.text.sm({ text: "b" }) });
|
||||
|
||||
const layer = over.node.closest(".cx-modal-layer");
|
||||
assert.ok(layer, "a stacked modal has no layer of its own");
|
||||
assert.equal(layer.querySelectorAll(".cx-backdrop").length, 1,
|
||||
"a stacked modal must dim through its own backdrop element, exactly once");
|
||||
|
||||
over.close();
|
||||
under.close();
|
||||
});
|
||||
|
||||
test("no overlay paints a backdrop with a viewport-sized shadow spread", () => {
|
||||
// Read from the stylesheet, because jsdom applies no CSS: asserting on
|
||||
// node.style.boxShadow here passes against any rule at all, since the value
|
||||
// lives in the sheet and the inline style is always empty. The first version
|
||||
// of this check did exactly that.
|
||||
const css = readFileSync(join(here, "..", "elements", "overlay.css"), "utf8");
|
||||
const offenders = css.split("\n")
|
||||
.filter((line) => /box-shadow/.test(line) && /vmax|vmin|\b\d{3,}(px|vh|vw)\b/.test(line));
|
||||
assert.deepEqual(offenders, [],
|
||||
"a shadow spread that large is a stale-compositor-layer bug waiting to happen; " +
|
||||
"dim with a .cx-backdrop element instead");
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user