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([]); + }); +});