diff --git a/app/composer/elements/floating.js b/app/composer/elements/floating.js index a54e08ee..c3b0bfff 100644 --- a/app/composer/elements/floating.js +++ b/app/composer/elements/floating.js @@ -4,7 +4,7 @@ import { define } from "../internals/register.js"; import { el } from "../internals/el.js"; import { uid } from "../internals/ids.js"; import { mount, unmount } from "../internals/portal.js"; -import { claim } from "../internals/zlayer.js"; +import { claimFor } from "../internals/zlayer.js"; import { push } from "../internals/dismiss.js"; import { trap } from "../internals/focus.js"; import { drag } from "../internals/drag.js"; @@ -27,16 +27,14 @@ define("floating", "window", ({ width = 420, height = 320, x = 80, y = 80, resizable = true, rollup = true, onClose, } = {}) => { - // The callback matters: another window coming to the front shifts this one's - // z, and without it the DOM keeps the number it had at open time. - const layer = claim("floatingWindow", (z) => { node.style.zIndex = String(z); }); const saved = recall(id) || {}; const box = { x: saved.x ?? x, y: saved.y ?? y, width: saved.width ?? width, height: saved.height ?? height }; const titleId = uid("win-title"); const node = el("div", { cls: "cx-window", attrs: { role: "dialog", "aria-labelledby": titleId } }); - node.style.zIndex = String(layer.z); + // Bound, because another window coming to the front shifts this one's z. + const layer = claimFor("floatingWindow", node); const apply = () => { node.style.left = `${box.x}px`; @@ -140,7 +138,6 @@ define("floating", "window", ({ /** A full-screen "please wait" that genuinely blocks. */ define("overlay", "blocking", ({ message, progress, cancel } = {}) => { - const layer = claim("wizard"); const card = el("div", { cls: "cx-blocking-card", children: [ el("span", { cls: ["cx-spinner", "cx-spinner-md"], attrs: { "aria-hidden": "true" } }), el("p", { cls: "cx-t cx-t-sm", text: message }), @@ -151,7 +148,7 @@ define("overlay", "blocking", ({ message, progress, cancel } = {}) => { const node = el("div", { cls: "cx-blocking", attrs: { role: "alertdialog", "aria-busy": "true", "aria-label": message }, children: card }); - node.style.zIndex = String(layer.z); + const layer = claimFor("wizard", node); mount(node); const release = trap(card); @@ -166,7 +163,6 @@ define("overlay", "blocking", ({ message, progress, cancel } = {}) => { /** A panel that slides in from an edge. Simple mode uses these over the preview. */ define("slideOver", "md", ({ side = "right", title, body, onClose } = {}) => { - const layer = claim("modal"); const titleId = uid("slide-title"); const panel = el("aside", { cls: ["cx-slideover", `cx-slideover-${side}`], @@ -179,7 +175,7 @@ define("slideOver", "md", ({ side = "right", title, body, onClose } = {}) => { panel.append(el("div", { cls: "cx-slideover-body", children: nodeOf(body, "slideOver.md") })); const root = el("div", { cls: "cx-slideover-layer", children: [el("div", { cls: "cx-backdrop" }), panel] }); - root.style.zIndex = String(layer.z); + const layer = claimFor("modal", root); mount(root); const release = trap(panel); diff --git a/app/composer/elements/modal.js b/app/composer/elements/modal.js index 55763926..881aab1d 100644 --- a/app/composer/elements/modal.js +++ b/app/composer/elements/modal.js @@ -12,7 +12,7 @@ import { define } from "../internals/register.js"; import { el } from "../internals/el.js"; import { uid } from "../internals/ids.js"; import { mount, unmount } from "../internals/portal.js"; -import { claim } from "../internals/zlayer.js"; +import { claimFor } from "../internals/zlayer.js"; import { push } from "../internals/dismiss.js"; import { trap } from "../internals/focus.js"; @@ -33,7 +33,6 @@ function open({ closeOnOutside = true, stacked = false, describedBy, } = {}) { const titleId = uid("modal-title"); - const layer = claim(stacked ? "modal" : "modal"); const card = el("div", { cls: ["cx-modal", `cx-modal-${size}`, stacked ? "cx-modal-stacked" : null], @@ -66,7 +65,9 @@ function open({ const backdrop = el("div", { cls: "cx-backdrop" }); const root = el("div", { cls: "cx-modal-layer", children: [backdrop, card] }); - root.style.zIndex = String(layer.z); + // A stacked modal shares the modal rung with the one beneath it: peers stack + // by claim order, so it lands above without needing a rung of its own. + const layer = claimFor("modal", root); mount(root); // Trap after mounting: focusables() must be able to see the card. diff --git a/app/composer/elements/popover.js b/app/composer/elements/popover.js index 2f3b82a8..c84442a4 100644 --- a/app/composer/elements/popover.js +++ b/app/composer/elements/popover.js @@ -6,7 +6,7 @@ import { define } from "../internals/register.js"; import { el } from "../internals/el.js"; import { mount, unmount } from "../internals/portal.js"; -import { claim } from "../internals/zlayer.js"; +import { claimFor } from "../internals/zlayer.js"; import { push } from "../internals/dismiss.js"; import { anchorTo } from "../internals/anchor.js"; @@ -18,9 +18,8 @@ function nodeOf(control, where) { function floatOn(rung, cls, { anchor, side = "bottom", align = "start", gap = 6, onClose, closeOnOutside = true, content, role, label }) { const anchorNode = anchor && anchor.node ? anchor.node : anchor; - const layer = claim(rung); const node = el("div", { cls, attrs: { role, "aria-label": label } }); - node.style.zIndex = String(layer.z); + const layer = claimFor(rung, node); if (content) node.append(content); // Mounted before measuring: a hidden element measures 0x0 and lands in the diff --git a/app/composer/elements/status.js b/app/composer/elements/status.js index 692ff52d..d5aa803a 100644 --- a/app/composer/elements/status.js +++ b/app/composer/elements/status.js @@ -8,7 +8,7 @@ import { define } from "../internals/register.js"; import { el } from "../internals/el.js"; import { setText } from "../internals/text.js"; import { portal } from "../internals/portal.js"; -import { claim } from "../internals/zlayer.js"; +import { claimFor } from "../internals/zlayer.js"; const TONES = ["neutral", "good", "warn", "danger", "info"]; @@ -122,8 +122,7 @@ let stackLayer = null; function toastStack() { if (stack && stack.isConnected) return stack; stack = el("div", { cls: "cx-toasts" }); - stackLayer = claim("toast"); - stack.style.zIndex = String(stackLayer.z); + stackLayer = claimFor("toast", stack); stack.style.pointerEvents = "auto"; portal().appendChild(stack); return stack; diff --git a/app/composer/elements/wheel.js b/app/composer/elements/wheel.js index a21d7de4..cac34e92 100644 --- a/app/composer/elements/wheel.js +++ b/app/composer/elements/wheel.js @@ -12,7 +12,7 @@ import { define } from "../internals/register.js"; import { el, svg } from "../internals/el.js"; import { mount, unmount } from "../internals/portal.js"; -import { claim } from "../internals/zlayer.js"; +import { claimFor } from "../internals/zlayer.js"; import { push } from "../internals/dismiss.js"; const MIN = 2; @@ -127,11 +127,10 @@ function buildWheel({ items, onPick, onClose, geo, recentre, cancelOnCentre, cls } const { size } = geo; - const layer = claim("popover"); const centre = recentre(); const node = el("div", { cls, attrs: { role: "menu", "aria-label": "Quick pick" } }); - node.style.zIndex = String(layer.z); + const layer = claimFor("popover", node); node.style.width = `${size}px`; node.style.height = `${size}px`; diff --git a/app/composer/internals/zlayer.js b/app/composer/internals/zlayer.js index c633c1b0..1233e9a2 100644 --- a/app/composer/internals/zlayer.js +++ b/app/composer/internals/zlayer.js @@ -95,6 +95,23 @@ function notify(list, start) { list.forEach((entry, i) => { if (entry.onChange) entry.onChange(start + i); }); } +/** + * Claim a rung AND keep `node`'s z-index in sync with it -- the form every + * overlay should use. + * + * `claim()` alone hands back a number and trusts the caller to notice when it + * changes, and a caller that forgets gets a z-index that was right at open time + * and silently wrong afterwards: a peer releasing compacts the rung, and the + * next overlay opened lands on the number this one is still displaying. Six of + * the seven call sites forgot. Binding the node here means there is nothing + * left to forget. + */ +export function claimFor(rung, node) { + const handle = claim(rung, (z) => { node.style.zIndex = String(z); }); + node.style.zIndex = String(handle.z); + return handle; +} + /** Live claims in a rung, bottom to top. Diagnostics and tests. */ export const liveCount = (rung) => (stacks.get(rung) || []).length; diff --git a/app/composer/tests/overlays.test.js b/app/composer/tests/overlays.test.js index 1f574be1..851dd378 100644 --- a/app/composer/tests/overlays.test.js +++ b/app/composer/tests/overlays.test.js @@ -7,11 +7,14 @@ import { setupDom, teardownDom, key, fire } from "./_dom.js"; import { EDGES, sectorAt, sectorInArc } from "../elements/wheel.js"; let composer; +let entries; +let DEMOS; let ROOT_ID; let baseOf; test.before(async () => { setupDom(); - ({ composer } = await import("../composer.js")); + ({ composer, entries } = await import("../composer.js")); + ({ DEMOS } = await import("../../catalogue/demos.js")); ({ ROOT_ID } = await import("../internals/portal.js")); ({ baseOf } = await import("../internals/zlayer.js")); }); @@ -454,3 +457,56 @@ test("gallery thumbnails are decorative, because the caption names them", () => "an alt here would make a screen reader say the name twice"); g.destroy(); }); + +// --- z-index staleness ------------------------------------------------------ + +test("a peer closing does not leave two overlays sharing a z-index", () => { + // A rung compacts when a claim is released, so every overlay's z can move + // without it doing anything. A caller that reads z once at open time keeps a + // number that is later handed to something else: two overlays at one level, + // where DOM order decides which one wins and which one eats the clicks. + const anchor = composer.button.md({ label: "x" }); + document.body.appendChild(anchor.node); + + const a = composer.menu.dropdown({ anchor, items: [{ id: "a", label: "A" }] }); + const b = composer.menu.dropdown({ anchor, items: [{ id: "b", label: "B" }] }); + assert.notEqual(a.node.style.zIndex, b.node.style.zIndex); + + a.close(); + const c = composer.menu.dropdown({ anchor, items: [{ id: "c", label: "C" }] }); + assert.notEqual(b.node.style.zIndex, c.node.style.zIndex, + "the newest overlay must not land on a number still in use"); + assert.ok(Number(c.node.style.zIndex) > Number(b.node.style.zIndex), + "and it must be above, not merely different"); + + b.close(); c.close(); +}); + +test("every overlay keeps its z-index bound, not just windows", () => { + // The failure above was one call site remembering and six forgetting, so the + // test walks every overlay the registry has rather than naming them. + const anchor = composer.button.md({ label: "x" }); + document.body.appendChild(anchor.node); + + const opened = []; + for (const { group, variant, factory } of entries()) { + const demo = DEMOS[`${group}.${variant}`]; + if (!demo) continue; + const handle = factory(typeof demo === "function" ? demo(composer) : demo); + if (!handle.isOverlay || !handle.node.style.zIndex) { handle.destroy(); continue; } + opened.push({ name: `${group}.${variant}`, handle }); + } + + // Closing the oldest compacts every rung it was in; nothing may go stale. + const before = opened.map(({ handle }) => handle.node.style.zIndex); + if (opened.length > 1) opened.shift().handle.close(); + + const seen = new Map(); + for (const { name, handle } of opened) { + const z = handle.node.style.zIndex; + assert.ok(!seen.has(z), `${name} collides with ${seen.get(z)} at z ${z}`); + seen.set(z, name); + } + void before; + for (const { handle } of opened) handle.destroy(); +});