From 73e81b0263fae7340db07eb3ccb04aec43c66360 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:38:07 +0300 Subject: [PATCH] composer: bind every overlay's z-index, not just windows A rung compacts when a claim is released, so an overlay's z can move without it doing anything. Six of seven call sites read z once at open time and kept a number that was later handed to the next overlay: two of them at one level, DOM order deciding which wins and which eats the clicks. Reproduced with two menus and a third opened after closing the first -- both ended up at 501. claimFor(rung, node) binds the node, so there is nothing left to forget, and the raw claim() stays for the one case with no node. The test walks the registry rather than naming overlays, because the bug was one site remembering and six forgetting. Also drops a no-op ternary that picked the modal rung either way. --- app/composer/elements/floating.js | 14 +++---- app/composer/elements/modal.js | 7 ++-- app/composer/elements/popover.js | 5 +-- app/composer/elements/status.js | 5 +-- app/composer/elements/wheel.js | 5 +-- app/composer/internals/zlayer.js | 17 +++++++++ app/composer/tests/overlays.test.js | 58 ++++++++++++++++++++++++++++- 7 files changed, 89 insertions(+), 22 deletions(-) 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(); +});