feat(web): shared native-dialog Modal primitive + migrate form modals (TASK-2023)#881
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of the PLAN-1984 Web-UX audit (24 hand-rolled backdrop modals, only 2 trapped Tab focus, none restored focus, most lacked dialog ARIA).
Adds one shared
Modalprimitive built on the native<dialog>element +dialog.showModal(), and migrates the form modals onto it.The primitive —
web/src/lib/components/common/Modal.svelteNative
showModal()gives us the focus trap, Escape dismissal, and the top-layer (no z-index juggling) for free. The primitive adds:document.activeElementon open, restores it on close (incl. teardown-while-open).open— an$effectreads theopenprop + the DOM'sdialogEl.openand writes no reactive state, so it can't self-invalidate (CONVE-1688). Escape routes throughoncancel→oncloseso the parent owns the state.aria-labelledby(wired to each modal's heading id) /ariaLabel.closeOnBackdrop, default on),prefers-reduced-motionrespected.--modal-bg/--modal-radius/--modal-shadowCSS vars so modals that differed keep their exact look.Prop/snippet API:
<Modal {open} {onclose} labelledby ariaLabel closeOnBackdrop maxWidth placement={'top'|'center'} class --modal-bg/-radius/-shadow>{children}</Modal>— children carry each modal's own header/body/footer markup.Migrated (8)
ShareDialog,ConnectWorkspaceModal,ImportFromUrlModal,CreateCollectionModal,EditCollectionModal,CreateWorkspaceModal,admin/UserModal,OpenChildrenDialogEach swap is structural — existing styling, props, and behavior preserved. Removed ~400 lines of duplicated backdrop/overlay boilerplate, per-modal
svelte:windowEscape handlers, and two hand-rolled focus traps (UserModal, OpenChildrenDialog).app.cssprint rule extended withdialog[open]so native dialogs stay hidden in print.Skipped (out of scope / follow-up)
connected-appsrevoke confirm,[collection]unsaved-card leave dialog,graphoverlays. The roles page already uses nativeshowModal().Notes
CreateCollectionModalnow scrolls its body within a fixed header/footer (was whole-modal scroll) — a consequence of the flex-column dialog.node, no jsdom/testing-library, andshowModal()is unavailable there), so no unit test was added for the primitive.Test plan
npm run check— 0 errors (only pre-existing unrelated warnings)npm run build— succeedsnpm run test— 128/128 passdialog.modal{display:flex}tied the UAdialog:not([open]){display:none}rule on specificity; fixed with an explicitdialog.modal:not([open]){display:none}).Refs TASK-2023, PLAN-1984.