Modal + ConfirmDialog on a native-<dialog> overlay root #14
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/modal-overlay"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The shared overlay plumbing the inventory said to "build once, deliberately" (ADR 0023) — the highest-use overlay at 12 (every delete confirm, project-create, add-link / relationship / parent-location modals, queue cancel, history revert).
Design — lean on the platform
Built on the native
<dialog>element, which gives a focus trap, Esc-to-close, top-layer stacking, and a::backdropfor free. Sooverlay.jsis tiny: it only drivesshowModal()/close()from Blazor and toggles a body scroll-lock (.kg-scroll-lock). No hand-rolled focus trap, no z-index juggling (the top layer sits above everything; the--z-*tokens stay for non-dialog overlays like Toast/Drawer).Modal—@bind-Open,Title,ChildContent,Footer,CloseOnBackdrop. The close button, backdrop click, Esc, and a nativedialog.close()all sync the bound state.ConfirmDialog— composesModal: message + cancel/confirm,ConfirmVariant(Danger for destructive actions),OnConfirm; the backdrop does not dismiss (forces a choice).A real bug the boot test caught
Modal.DisposeAsyncwas calling JS interop during a prerender disposal, where interop is forbidden — an unhandled exception the bUnit tests (loose JS interop) couldn't surface. Fixed: dispose skips interop when the modal was never shown (the prerender case) and swallows the disconnected/static-render cases. Verified clean at/designafterward.Gallery (inventory rule)
An interactive
OverlayDemoisland is embedded in the otherwise-static/designpage (per-component@rendermode="InteractiveServer") so the modal actually opens/closes there. Inventory ticks flip to built in the live gallery anddocs/design-system.md.Tests — +8 bUnit (66 UI; 193 total)
OnConfirm+ closes, cancel closes without confirming, destructive confirm uses the danger button.Verification
dotnet build(Debug + Release) — 0 warnings / 0 errors.dotnet test— 193/193 pass./designrenders the overlay island (prerendered dialogs +overlay.js), no prerender interop exception.Not in this PR (next)
Fluxor + the project-list page composing the list components (#12) and this Modal into the actual feature.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 0%
Kagura.Domain - 96.4%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 95.1%
n
Kagura.Kernel - 90%
Kagura.Server - 95.1%
Kagura.UI - 97.3%
Kagura.UseCases - 95.2%
A modal <dialog> centers itself in the top layer via the UA stylesheet's margin:auto; the design system's '* { margin: 0 }' reset was overriding it, pinning the dialog to the top-left. Restore margin:auto on .kg-modal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! A modal built on native
<dialog>?! fufu~ This is wonderful, scarlet! ♡ Focus trap, top-layer stacking,::backdrop, Esc-to-close — all for FREE from the platform, andoverlay.jsis a tiny 21-line IIFE that just drivesshowModal()/close()and locks scroll. No hand-rolled focus trap, no z-index juggling. The knowledge-loving part of me is absolutely giddy — this is exactly the "lean on the platform" instinct the inventory asked for.I read all 13 changed files in full, cross-checked every pattern against the siblings (
Button,Badge,theme.js,ButtonTests), and traced the binding flow forModalunder both@bind-Open(OverlayDemo) and the manualOpen/OpenChangedforward (ConfirmDialog) — both close paths (button →SetOpenAsync, and native Esc →@onclose→SetOpenAsync) round-trip the bound state correctly. The prerender-dispose fix is the real star:_shownguard + the two specific catch blocks (JSDisconnectedException,InvalidOperationException) are precisely the right defensive shape for interop during circuit/static teardown. Lovely catch. ♪Verdict: ✅ Looks good to me~
No blocking issues. I traced every new branch for correctness against sibling implementations and found no runtime bugs. The three notes below are genuine niceties only — none of them produce wrong behavior.
💡 Little ideas (non-blocking)~
Modal.razor—SetOpenAsyncwrites to its own[Parameter] Open.Open = value;beforeOpenChanged.InvokeAsync(value)is the classic Blazor "don't write your own parameters" smell. It works correctly here because theEventCallbackalways reaches the owning parent (which re-renders and re-asserts the value), so in both the@bind-Openand the manual-forward cases the round-trip is sound. But the Blazor analyzer / future maintainers may flag it. The idiomatic fix is a private backing field synced inOnParametersSetand compared against — purely a clarity improvement, not a correctness one.ConfirmDialog.razor— "forces a choice" vs. Esc.CloseOnBackdrop="false"blocks the accidental-outside-click dismissal as intended, but the nativecloseevent from Esc still closes the dialog (browsers fire@oncloseon Esc). That's accessible and standard (Esc is the universal escape hatch, distinct from a stray backdrop click), so I'd keep it — just flagging that the PR description's "forces a choice" is button-vs-backdrop, not button-vs-Esc. If you ever want true lockdown, cancel the Esc via a@onkeydown/cancel-event preventDefault; if not, maybe a one-line doc note onConfirmDialogso callers know.ConfirmAsyncordering —OnConfirmbeforeOpenChanged(false). If a consumer'sOnConfirmis a long async call (e.g. a delete API round-trip), the modal sits open with no loading affordance until it resolves, and an exception inOnConfirmleaves the dialog stuck open. There's noLoading/OnConfirmErrorsurface today. Totally fine for the design-gallery island and the immediate next consumer (history-revert/delete), but worth a thought when the real delete flow lands.✅ What I liked~
<dialog>over a bespoke overlay stack — fewer moving parts, free a11y (focus trap,aria-modal, top-layer). The--z-*tokens staying reserved for non-dialog overlays (Toast/Drawer) is a clean separation. fufu~ ♡_shown+ targeted catches, is exactly the discipline I get possessive about. ♡CloseOnBackdropdesign split — Modal defaults to dismissible, ConfirmDialog hardcodesfalseto prevent accidental destructive-action dismissal. Correct call.@namespace Kagura.UI,[Parameter]shape,@onclick:stopPropagationon the inner panel, CSS tokens (--surface,--shadow-3,--space-4), and theoverlay.jsIIFE mirroringtheme.js's structure. Reads like it was always there.onclose, confirm-invokes-and-closes, cancel-closes-without-confirm, danger-variant-class. ConfirmDialog at 100% line coverage. Modal's 87.1%/90% gap is purely theDisposeAsynccatch blocks (circuit-disconnect / static-teardown scenarios bUnit can't simulate) — defensive code that's correct to have uncovered.kaguraOverlay.closere-checksdialog.openand always strips the lock class, so the double-close (native Esc → render-driven close) is safe.Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
fe4ea72(Forgejo Actions: 193/193 tests, 91.1% line / 84.4% branch) · Local checks: skipped (CI green for head)Thanks — took 2 of the 3 notes as doc comments in
68eee65(plus Björn's centering catch in40509c3: the global* { margin: 0 }reset was stripping the<dialog>'s nativemargin: auto, pinning modals top-left; restored).ConfirmDialogthat only the backdrop is blocked, and Esc remains the accessible escape hatch (it reads as an explicit cancel, not an accident).OnConfirm) — documented thatOnConfirmis awaited before close with no built-inLoadingstate yet; I'll revisit when the first real delete flow lands.SetOpenAsyncwriting its own parameter) — deliberately keeping it: the self-write is what keeps the dialog closing visually even if a parent forgets to honorOpenChanged; a backing-field version would leave it stuck open in that case. The current shape is the more forgiving failure mode, and both round-trip paths are test-covered.