From 752b3f8c8a23369167a392d3658a08b5b2d25dc5 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 15:26:15 -0600 Subject: [PATCH] ui: only the top modal answers Escape, and the audited widths Two things, both reported from the running app. Nested modals could not exist. Mantine binds a window-level keydown listener per open modal, gated only on whether that modal is open -- nothing checks which one the user is looking at: useWindowEvent('keydown', (event) => { if (event.key === 'Escape' && closeOnEscape && ... && opened) onClose(); }, {capture: true}); Two open modals meant two listeners, so one Escape closed both. Reproduced on the dashboard: the download modal plus the search modal over it, one press, both gone, back to the page. Both also sat at z-index 200, so which one drew on top was DOM order rather than which the user opened. Mantine's own answer is Modal.Stack with a stackId, which is the only thing that gates closeOnEscape. Not used, for two reasons: every call site would have to be rewritten to hoist its modal into a stack, and it sets `__hidden` on everything below the top -- the parent disappears while the child is open. The parent should be dimmed and still there, as it was before the migration. So the wrapper keeps the register itself: a module-level list of open modals, most recent on top, from which each modal takes closeOnEscape, trapFocus and a z-index two above its parent. Module-level rather than React context because the two modals in the bug are not nested in the JSX at all -- the search modal is rendered by KeyboardShortcutsProvider near the app root and the download modal by the dashboard. They are siblings that happen to be open together, and no context relates them. The increment is small on purpose: Mantine puts popovers at 300, and a stack that climbed past it would paint over the select dropdowns inside its own forms. Widths: Roland audited the modal list and chose sizes against Mantine's scale. Sixteen get wider -- twelve to `large` (620), four to `fullscreen` -- and eleven keep the width they had, with the size now written down so the decision is in the source rather than in a default. Flasher was passing Mantine's raw `sm`, which worked but bypassed the naming the audit used; it says `tiny` now. The size table itself is guarded, because it is the single thing that halved every modal during the migration and a quiet edit to it moves seventy at once. Semantic's widths, measured on the pre-migration build still running on the QA Pi: mini 360, tiny 540, small 720, large 1080, no-size 900. Mantine's, read out of its stylesheet: 320/380/440/620/780. A `small` modal went 720 -> 440 and one with no size went 900 -> 440. Three cases now hold the scale, the mapping, and the rule that no call site may name a size the table does not know -- an unknown name is passed to Mantine, ignored, and silently rendered at the default. Tested in a real browser: none of it is visible to jsdom -- the listener is on window with capture, the stacking is z-index, and "still visible behind" is geometry. Note that `should('be.visible')` is the wrong assertion for the parent: it is an occlusion check, so a child drawn correctly over its parent trips it. The test uses a wide viewport and asserts the parent is rendered and shows to both sides of the child. Planting the pre-fix behaviour reddens three. 1124 jest / 62 suites, 8 in modal-stack.cy.js, clean npm run build. The 12 failures in Tags/Channels/Download/Inventory.cy.js are the known pre-existing Semantic-era selector failures, unchanged. --- app/src/DashboardPage.js | 4 +- app/src/Tags.js | 4 +- app/src/components/BulkTagModal.js | 2 +- app/src/components/Channels.js | 2 +- app/src/components/Common.js | 2 +- app/src/components/DonatePage.js | 2 +- app/src/components/FilePreview.js | 2 +- app/src/components/Files.js | 2 +- app/src/components/Flasher.js | 2 +- .../components/KeyboardShortcutsProvider.js | 2 +- app/src/components/Map.js | 2 +- app/src/components/Share.js | 2 +- app/src/components/Zim.js | 4 +- app/src/components/admin/ControllerPage.js | 4 +- app/src/components/admin/Downloads.js | 12 +- .../collections/CollectionTagModal.js | 2 +- .../components/inventory/InventoryRoute.js | 2 +- app/src/components/ui/Overlays.tsx | 86 +++++++- app/src/components/ui/modal-stack.cy.js | 199 ++++++++++++++++++ app/src/components/ui/ui.test.js | 72 +++++++ 20 files changed, 382 insertions(+), 27 deletions(-) create mode 100644 app/src/components/ui/modal-stack.cy.js diff --git a/app/src/DashboardPage.js b/app/src/DashboardPage.js index 82902bc6..a468b82c 100644 --- a/app/src/DashboardPage.js +++ b/app/src/DashboardPage.js @@ -251,7 +251,7 @@ export function Getters() { let getterModal; if (selectedGetter === 'downloads') { - getterModal = handleSetGetter(null, null)} @@ -266,7 +266,7 @@ export function Getters() { ; } else if (selectedGetter === 'upload') { - getterModal = handleSetGetter(null, null)} diff --git a/app/src/Tags.js b/app/src/Tags.js index df08e3df..4bc2beec 100644 --- a/app/src/Tags.js +++ b/app/src/Tags.js @@ -332,7 +332,7 @@ function EditTagsModal() { ]; return <> - + Edit Tags
@@ -567,7 +567,7 @@ export function AddTagsButton({ type='button' disabled={disabled} /> - setOpen(false)}> + setOpen(false)}> {loading && Updating tags…}
Applied Tags
diff --git a/app/src/components/BulkTagModal.js b/app/src/components/BulkTagModal.js index b3228569..22f135d8 100644 --- a/app/src/components/BulkTagModal.js +++ b/app/src/components/BulkTagModal.js @@ -125,7 +125,7 @@ export function BulkTagModal({open, onClose, paths, onComplete}) { : 0; return ( - + Bulk Tag Files {state === 'loading' && ( diff --git a/app/src/components/Channels.js b/app/src/components/Channels.js index 44feaecf..0a9ed3df 100644 --- a/app/src/components/Channels.js +++ b/app/src/components/Channels.js @@ -309,7 +309,7 @@ export function ChannelEditPage() { New Download - setDownloadModalOpen(false)}> + setDownloadModalOpen(false)}>
New Channel Download
- setOpen(false)} > diff --git a/app/src/components/DonatePage.js b/app/src/components/DonatePage.js index 7d024052..454ee6aa 100644 --- a/app/src/components/DonatePage.js +++ b/app/src/components/DonatePage.js @@ -18,7 +18,7 @@ function CoinQRButton({qrCodeValue, header, buttonColor}) { return <> setOpen(true)}/> - setOpen(false)} onOpen={() => setOpen(true)} diff --git a/app/src/components/FilePreview.js b/app/src/components/FilePreview.js index 4e28ba2d..d5990835 100644 --- a/app/src/components/FilePreview.js +++ b/app/src/components/FilePreview.js @@ -420,7 +420,7 @@ export function FilePreviewProvider({children}) { setErrorModalOpen(false); } - const errorModal = Unknown File diff --git a/app/src/components/Files.js b/app/src/components/Files.js index 499ea9ab..5530fd8e 100644 --- a/app/src/components/Files.js +++ b/app/src/components/Files.js @@ -644,7 +644,7 @@ export function SearchFilterModal( >{i} ); - return + return Search Filters diff --git a/app/src/components/Flasher.js b/app/src/components/Flasher.js index 295c2fcc..a70d599e 100644 --- a/app/src/components/Flasher.js +++ b/app/src/components/Flasher.js @@ -943,7 +943,7 @@ export function FlasherPage() { } - setSaveModalOpen(false)} size='sm'> + setSaveModalOpen(false)} size='tiny'> Save firmware configuration + {/* * `wrolpi-search-modal` makes the suggestion list part of the panel rather diff --git a/app/src/components/Map.js b/app/src/components/Map.js index ebe34d32..bb649c1f 100644 --- a/app/src/components/Map.js +++ b/app/src/components/Map.js @@ -115,7 +115,7 @@ function RegionPreviewModal({bbox, name, open, onClose}) { return () => { if (map) map.remove(); }; }, [open, bbox]); - return + return {/* No `.media` here: the bbox highlight is drawn as a map layer on the canvas itself (see the `bbox` source/layers above), so the automatic `canvas` element-type filter already covers it. There is no separate DOM overlay to wrap. */} diff --git a/app/src/components/Share.js b/app/src/components/Share.js index e84c5de6..cf31c14d 100644 --- a/app/src/components/Share.js +++ b/app/src/components/Share.js @@ -38,7 +38,7 @@ export const ShareButton = () => { } return <> - diff --git a/app/src/components/Zim.js b/app/src/components/Zim.js index 83fc0316..c0c8c380 100644 --- a/app/src/components/Zim.js +++ b/app/src/components/Zim.js @@ -110,7 +110,7 @@ export const OutdatedZimsMessage = ({onClick}) => {

- @@ -165,7 +165,7 @@ const ZimSearchEntry = ({zimId, onTag, onUntag, entry}) => { {tagIcon} - setOpen(false)}>
{ setAddOpen(false); setShareAll(true); - }} size='small'> + }} size='large'> Add Samba Share setQrOpen(true)} /> - setQrOpen(false)} closeIcon> + setQrOpen(false)} closeIcon> Scan this code to join the hotspot {/* `media`: filtered as a unit by night mode; a QR code needs real black-on-white diff --git a/app/src/components/admin/Downloads.js b/app/src/components/admin/Downloads.js index 016169cb..c5c31d86 100644 --- a/app/src/components/admin/Downloads.js +++ b/app/src/components/admin/Downloads.js @@ -64,7 +64,7 @@ function DownloadProgressModal({progress, url}) { - setOpen(false)} size='small'> + setOpen(false)} size='fullscreen'> Download Progress {text} : (text) => {text}; - const errorModal = ; } - const editModal = @@ -456,7 +456,7 @@ function OnceDownloadRow({download, fetchDownloads, isSelected, onSelect}) { setErrorModalOpen(true)}/> ) - errorModal = setErrorModalOpen(false)} @@ -470,7 +470,7 @@ function OnceDownloadRow({download, fetchDownloads, isSelected, onSelect}) { if (downloader === Downloaders.Video) { editModal = ( - + return New Inventory setName(e.currentTarget.value)} diff --git a/app/src/components/ui/Overlays.tsx b/app/src/components/ui/Overlays.tsx index f674f238..b72776ea 100644 --- a/app/src/components/ui/Overlays.tsx +++ b/app/src/components/ui/Overlays.tsx @@ -45,6 +45,78 @@ export interface ModalProps extends Omit, 'o closeIcon?: boolean; } +/* + * Nested modals: which one is on top. + * + * Mantine binds a window-level `keydown` listener per open modal, gated only on whether that + * modal is open -- there is no check for whether it is the one the user is looking at. Two + * modals open meant two listeners, so a single Escape closed BOTH: opening the search modal + * over the dashboard's download modal and pressing Escape once left you on the dashboard. + * Semantic handled this; the migration lost it. + * + * Mantine's own answer is `Modal.Stack` with a `stackId` per modal, which is the only thing + * that gates `closeOnEscape`. It is not used here for two reasons: it requires every call + * site to be rewritten to hoist its modal into a stack, and it sets `__hidden` on everything + * below the top -- the parent disappears while the child is open. We want the parent dimmed + * and still visible behind the child, which is what it did before. + * + * So the wrapper keeps the register itself. A module-level list rather than React context, + * because the two modals in the bug are not nested in the JSX at all: the search modal is + * rendered by KeyboardShortcutsProvider near the app root while the download modal is + * rendered by the dashboard. They are siblings that happen to be open at once, and no + * context relates them. Most recently opened is on top. + */ +let openModalIds: string[] = []; +const modalStackListeners = new Set<() => void>(); +const notifyModalStack = () => modalStackListeners.forEach(listener => listener()); + +/** Mantine's default modal z-index; popovers and tooltips sit at 300. */ +const MODAL_Z_INDEX = 200; + +/** + * Where this modal sits in the stack of open ones. + * + * Exported for the tests, which need to assert the register empties -- a modal that failed to + * deregister would leave every later modal believing it was not on top, and nothing would + * close on Escape at all. + */ +export const openModalCount = () => openModalIds.length; + +function useModalStackPosition(opened: boolean): {isTop: boolean; zIndex: number} { + const id = React.useId(); + const [, rerender] = React.useReducer((count: number) => count + 1, 0); + + React.useEffect(() => { + modalStackListeners.add(rerender); + return () => { + modalStackListeners.delete(rerender); + }; + }, []); + + React.useEffect(() => { + if (!opened) return; + // Re-opening an already-registered modal moves it to the top, which is what a user + // who clicked its trigger again means. + openModalIds = [...openModalIds.filter(other => other !== id), id]; + notifyModalStack(); + return () => { + openModalIds = openModalIds.filter(other => other !== id); + notifyModalStack(); + }; + }, [opened, id]); + + const depth = openModalIds.indexOf(id); + return { + isTop: depth === -1 || depth === openModalIds.length - 1, + /* + * Two per level, so a child's overlay clears its parent's content. Kept small on + * purpose: Mantine puts popovers and select dropdowns at 300, and a modal stack that + * climbed past that would paint over the dropdowns inside itself. + */ + zIndex: MODAL_Z_INDEX + Math.max(depth, 0) * 2, + }; +} + function ModalBase({open, opened, size, closeIcon, title, children, ...props}: ModalProps) { const slots: Record = {header: [], actions: [], body: []}; React.Children.forEach(children, child => { @@ -54,13 +126,25 @@ function ModalBase({open, opened, size, closeIcon, title, children, ...props}: M else slots.body.push(child); }); + const isOpen = opened ?? open ?? false; + const {isTop, zIndex} = useModalStackPosition(isOpen); + return {slots.body} diff --git a/app/src/components/ui/modal-stack.cy.js b/app/src/components/ui/modal-stack.cy.js new file mode 100644 index 00000000..911bb899 --- /dev/null +++ b/app/src/components/ui/modal-stack.cy.js @@ -0,0 +1,199 @@ +import React from 'react'; +import {Button, Modal} from './index'; +import {openModalCount} from './Overlays'; + +/* + * Nested modals: only the one on top answers the keyboard, and the one underneath stays on + * screen behind it. + * + * Mantine binds a window-level `keydown` listener per open modal, gated only on whether that + * modal is open -- nothing checks which one the user is looking at. Two open modals meant + * two listeners, so one Escape closed both: opening the search modal over the dashboard's + * download modal and pressing Escape once left you on the dashboard. Reproduced in the + * running app before this was written. + * + * A real browser, because none of it is visible to jsdom: the listener is on `window` with + * `capture: true`, the stacking is z-index, and "still visible behind" is geometry. + */ + +const Nested = ({onParentClose, onChildClose}) => { + const [parent, setParent] = React.useState(false); + const [child, setChild] = React.useState(false); + return <> + + { + setParent(false); + onParentClose?.(); + }}> + Parent + +

The parent's own content.

+ + + + {/* + * A SIBLING of the parent in the JSX, not a descendant. This is the shape the bug + * actually took -- the search modal is rendered by KeyboardShortcutsProvider near + * the app root while the download modal is rendered by the dashboard -- and it is + * why the register is module-level rather than a React context, which could not + * relate these two at all. + */} + { + setChild(false); + onChildClose?.(); + }}> + Child +

The child's own content.

 + + +}; + +const titles = () => [...Cypress.$('.mantine-Modal-title')].map(el => el.textContent); + +const openBoth = () => { + cy.mountUI(); + cy.get('[data-testid="open-parent"]').click(); + cy.contains('.mantine-Modal-title', 'Parent').should('exist'); + cy.get('[data-testid="open-child"]').click(); + cy.contains('.mantine-Modal-title', 'Child').should('exist'); +}; + +// Escape has to come from a real element: Mantine's handler calls +// `event.target.getAttribute(...)`, which throws if the event was dispatched on `document`. +const pressEscape = () => cy.get('body').trigger('keydown', {key: 'Escape', bubbles: true}); + +describe('a modal opened over another', () => { + it('closes only itself on Escape, leaving the one underneath open', () => { + // The bug, exactly: one press used to take both. + openBoth(); + + pressEscape(); + + cy.contains('.mantine-Modal-title', 'Child').should('not.exist'); + cy.contains('.mantine-Modal-title', 'Parent').should('exist'); + }); + + it('lets the next Escape close the one underneath', () => { + // The other half: suppressing Escape on the parent must be temporary, or the parent + // becomes unclosable by keyboard for the rest of its life. + openBoth(); + + pressEscape(); + cy.contains('.mantine-Modal-title', 'Child').should('not.exist'); + pressEscape(); + + cy.contains('.mantine-Modal-title', 'Parent').should('not.exist'); + }); + + it('keeps the parent on screen behind the child, not hidden', () => { + /* + * Mantine's own Modal.Stack sets `__hidden` on everything below the top, so the + * parent vanishes while the child is open. That is the behaviour this deliberately + * does not use -- the parent should be dimmed and still there. + * + * Not `should('be.visible')`: that is an occlusion check, and a child drawn over its + * parent trips it by definition -- at the default component viewport the 440px child + * covers the 620px parent completely and the assertion fails on correct behaviour. + * A wide viewport instead, where the parent is genuinely broader than the child, and + * the claim is stated as what it is: the parent is rendered, and some of it is not + * behind the child. + */ + cy.viewport(1200, 800); + openBoth(); + + cy.get('.mantine-Modal-content').should(($contents) => { + expect($contents, 'both modals are rendered').to.have.length(2); + + const boxes = [...$contents].map(content => ({ + el: content, box: content.getBoundingClientRect(), + display: getComputedStyle(content).display, + visibility: getComputedStyle(content).visibility, + })); + const [child, parent] = [...boxes].sort((a, b) => a.box.width - b.box.width); + + // `__hidden` would show up as either of these. + expect(parent.display, 'the parent is not display:none').to.not.equal('none'); + expect(parent.visibility, 'the parent is not hidden').to.equal('visible'); + expect(parent.box.height, 'the parent still has a box').to.be.greaterThan(0); + + // And it is wider than the child on both sides, so it is actually seen. + expect(parent.box.left, 'parent shows to the left of the child') + .to.be.lessThan(child.box.left - 1); + expect(parent.box.right, 'parent shows to the right of the child') + .to.be.greaterThan(child.box.right + 1); + }); + }); + + it('draws the child above the parent', () => { + // Both modals used to sit at z-index 200, so which one won was DOM order rather than + // which one the user opened. + openBoth(); + + cy.get('.mantine-Modal-inner').should(($inners) => { + const zIndexes = [...$inners].map(inner => + parseInt(getComputedStyle(inner).zIndex, 10)); + + expect(zIndexes, 'two modals are stacked').to.have.length(2); + expect(Math.max(...zIndexes), 'the child clears the parent') + .to.be.greaterThan(Math.min(...zIndexes)); + }); + }); + + it('keeps the stack below the popovers that open inside it', () => { + /* + * A Select or date picker inside a modal renders at Mantine's popover z-index of 300. + * A stack that climbed past that would paint over the dropdowns in its own form, + * which is a worse bug than the one being fixed. + */ + openBoth(); + + cy.get('.mantine-Modal-inner').should(($inners) => { + const highest = Math.max(...[...$inners] + .map(inner => parseInt(getComputedStyle(inner).zIndex, 10))); + expect(highest, 'top modal stays under the popover layer').to.be.lessThan(300); + }); + }); +}); + +describe('the register of open modals', () => { + it('empties as they close', () => { + /* + * The failure mode of keeping a module-level list: a modal that unmounts without + * deregistering leaves a ghost on top forever, and then NOTHING closes on Escape -- + * every real modal believes something else is above it. Silent, permanent, and + * invisible to a test that only ever opens one modal. + */ + openBoth(); + cy.then(() => expect(openModalCount(), 'both registered').to.equal(2)); + + pressEscape(); + cy.contains('.mantine-Modal-title', 'Child').should('not.exist'); + cy.then(() => expect(openModalCount(), 'child deregistered').to.equal(1)); + + pressEscape(); + cy.contains('.mantine-Modal-title', 'Parent').should('not.exist'); + cy.then(() => expect(openModalCount(), 'register is empty').to.equal(0)); + }); + + it('is empty before anything opens, so the counts above mean something', () => { + cy.mountUI(); + cy.then(() => expect(openModalCount()).to.equal(0)); + }); +}); + +describe('a single modal', () => { + it('still closes on Escape', () => { + // The premise for every case above. Suppressing Escape on all but the top is + // worthless if the top does not respond either. + cy.mountUI(); + cy.get('[data-testid="open-parent"]').click(); + cy.contains('.mantine-Modal-title', 'Parent').should('exist'); + + pressEscape(); + + cy.contains('.mantine-Modal-title', 'Parent').should('not.exist'); + expect(titles()).to.not.include('Parent'); + }); +}); diff --git a/app/src/components/ui/ui.test.js b/app/src/components/ui/ui.test.js index c93e6442..192fb2e1 100644 --- a/app/src/components/ui/ui.test.js +++ b/app/src/components/ui/ui.test.js @@ -1487,3 +1487,75 @@ describe('Confirm', () => { expect(screen.getByRole('button', {name: 'Wipe'})).toHaveClass('wrolpi-button-danger'); }); }); + +describe('modal sizes', () => { + /* + * The size table is the single thing that halved every modal in the migration. + * + * Semantic's names were measured on the pre-migration build: mini 360, tiny 540, small + * 720, large 1080, fullscreen 95%. They are mapped onto Mantine's much smaller scale, so + * a modal that said `small` went from 720px to 440px and one that said nothing at all + * went from 900px to 440px. That is deliberate now -- the sizes were audited call site + * by call site against this scale -- but it means the table IS the design, and a quiet + * edit to it moves seventy modals at once. + */ + const MANTINE_PX = {xs: 320, sm: 380, md: 440, lg: 620, xl: 780}; + + it('resolves each name to the width the audit was done against', () => { + // Read out of Mantine's own stylesheet rather than restated, so a version bump that + // changes the scale fails here instead of silently resizing the app. + const css = fs.readFileSync( + path.join(__dirname, '../../../node_modules/@mantine/core/styles.css'), 'utf8'); + + Object.entries(MANTINE_PX).forEach(([name, px]) => { + const declared = css.match( + new RegExp(`--modal-size-${name}:\\s*calc\\(([\\d.]+)rem`)); + expect({name, px: declared && Math.round(parseFloat(declared[1]) * 16)}) + .toEqual({name, px}); + }); + }); + + it('maps every Semantic name onto that scale', () => { + // The mapping the call sites are written against. `fullscreen` is the one that did + // not shrink, and the one several audited modals were moved TO. + const source = fs.readFileSync(path.join(__dirname, 'Overlays.tsx'), 'utf8'); + const table = source.match(/const modalSizes[^=]*=\s*{([^}]*)}/)[1]; + const pairs = Object.fromEntries([...table.matchAll(/(\w+):\s*'([^']+)'/g)] + .map(m => [m[1], m[2]])); + + expect(pairs).toEqual({ + mini: 'xs', tiny: 'sm', small: 'md', large: 'lg', fullscreen: '100%', + }); + }); + + it('never lets a call site name a size the table does not know', () => { + /* + * A name the table has never heard of is passed straight to Mantine, which ignores + * anything it does not recognise and renders the default -- so a typo is a modal that + * silently reverts to 440px with no warning anywhere. Flasher was passing Mantine's + * own `sm` directly, which worked but bypassed the naming the audit used. + */ + const known = new Set(['mini', 'tiny', 'small', 'large', 'fullscreen']); + const offenders = []; + + const walk = (directory) => fs.readdirSync(directory, {withFileTypes: true}) + .flatMap(entry => { + const full = path.join(directory, entry.name); + if (entry.isDirectory()) return entry.name === 'node_modules' ? [] : walk(full); + if (!/\.(js|jsx|tsx)$/.test(entry.name)) return []; + if (/\.(test|cy)\./.test(entry.name)) return []; + return [[full, fs.readFileSync(full, 'utf8')]]; + }); + + for (const [file, source] of walk(path.join(__dirname, '..', '..'))) { + // Only ``; a computed `size={...}` is the caller's business. + for (const match of source.matchAll(/]*?\bsize='([^']+)'/g)) { + if (!known.has(match[1])) { + offenders.push(`${path.basename(file)}: size='${match[1]}'`); + } + } + } + + expect(offenders).toEqual([]); + }); +});