feat: add native dialog - #482
Conversation
|
I don't use MAUI or native desktop apps. So we may need to organize with #478 |
|
@mathewtaylor could you take a look? |
mathewtaylor
left a comment
mathewtaylor
left a comment
There was a problem hiding this comment.
Thanks for this — it's a well-made PR, and it solves a real problem. I checked it out, built it, and drove it in a browser rather than only reading the diff. Two bugs to fix, one behaviour to decide on, and some smaller things.
A note on process first: thank you for flagging the AI assistance up front. That's the right call and it doesn't count against the PR — the design decisions here are clearly considered.
Must fix
1. Clicking inside the dialog closes it
setupDialog treats any click whose target is the dialog element as a backdrop click:
const onClick = (e) => {
if (e.target === dialog) {
dotNetRef.invokeMethodAsync('JsOnNativeBackdropClick');
}
};That's the usual idiom, and it's only safe when the dialog has no padding of its own. Here it has 24px — GetClassNames() keeps p-6 for both strategies, and the reset in blazorblueprint-input.css sets padding: 0 specifically so the Tailwind class wins.
So the entire 24px band inside the dialog's border counts as backdrop. Reproduced on the demo page:
const d = document.querySelector('dialog[data-state]');
const r = d.getBoundingClientRect();
document.elementFromPoint(r.x + 8, r.y + 8); // → DIALOG#dialog-18-contentClicking that point — visually well inside the box, above the title — closes the dialog. The JS path doesn't have this: it renders the content <div> and the overlay as separate elements, so the same click lands on the content and is correctly ignored. I verified both.
Comparing the click coordinates against getBoundingClientRect() is the usual fix, though it has its own edge case with a scrolled dialog. Wrapping @ChildContent in an inner element and testing !inner.contains(e.target) may be tidier given you already control the markup.
2. Focus isn't restored to the trigger on close
After closing with Escape, document.activeElement is <body>. Keyboard users lose their place, and it's a regression against the JS path.
INativeOverlayService.FocusTriggerAsync is declared and implemented, but nothing calls it — CleanupNativeAsync disposes the listeners and closes the element without touching focus.
Worth knowing why the browser doesn't cover this for you: a native <dialog> does restore focus on close(), but the element is unmounted by @if (Context.IsOpen) rather than closed first, so that never runs. Either call close() and let the browser handle it before unmounting, or call the FocusTriggerAsync you've already written.
Worth deciding before merge
LockScroll is silently ignored in native mode
The parameter is still accepted and still documented, but HandleNativeLifecycleAsync returns before the scroll-lock block. showModal() makes the background inert to interaction, but it doesn't stop it scrolling — with the native dialog open, body computes to overflow: visible, where the JS path sets hidden.
A parameter that quietly does nothing is worse than one that isn't offered, so I'd rather it either applied the same lock or documented the difference explicitly. Your call which.
Same question applies to TrapFocus, though in the other direction — native always traps, so TrapFocus="false" can't be honoured. Probably just needs saying in the XML doc.
Smaller
IsDialogSupportedAsynccaches a failure permanently. If the first call lands while interop is unavailable,dialogSupported = falsesticks for the lifetime of the scope, and the "browser does not support<dialog>" warning then fires on every open in a browser that supports it perfectly well. Caching only the successful result would avoid that._useNativeis computed once inOnInitialized, in both the primitive and the styled component, so changingRenderingStrategyafter first render has no effect. Fine if intended — worth a line in the XML doc if so.FocusDialogAsyncruns immediately aftershowModal(), which has already moved focus per spec. That can override a consumer'sautofocus. It may be redundant.
What's good
Genuinely — several things here are done better than I'd expect:
- Dropping
role="dialog",aria-modal="true"andtabindex="-1"on the native element is correct, and most implementations get it wrong by keeping them. A modal<dialog>has those semantics implicitly andaria-modalis actively discouraged on it. - Backward compatible by default.
JavaScriptstays the default at both the global and per-component level, so nothing changes for anyone who doesn't opt in. - The comment in
native-dialog.jsabout avoiding top-level lexical bindings because WASM's dynamicimport()can re-evaluate a module — that's a real failure mode and I'm glad it's written down rather than just worked around. - Tests, demo page, code examples, API reference entries, changelog and API surface snapshots are all updated. That's the full checklist and it's rare to get it in one pass.
Merge state
Only CHANGELOG.md conflicts, from entries that landed on develop today — trivial to resolve. With that fixed it builds clean and all 83 tests pass, including the overlay convention guards added since you opened this.
One thing to know: #479, which you linked, is now closed. The cause turned out to be a static layout in a per-page-interactive app rather than the portal handshake itself, and the fix there was diagnostics. That doesn't reduce the value of this PR — the sluggishness argument from discussion #376 stands on its own, and native <dialog> is the right direction.
Happy to take another look once the two above are sorted.
d7e8992 to
ffd0a09
Compare
ffd0a09 to
ccc8d66
Compare
mathewtaylor
left a comment
mathewtaylor
left a comment
There was a problem hiding this comment.
Both must-fix items are done, and I checked them in a browser rather than reading the diff.
1. Clicking inside the dialog no longer closes it. The getBoundingClientRect comparison is the right call, and the comment explaining why e.target === dialog is ambiguous for a top-layer element is the part a future reader will need. Verified on the demo: a click 8px inside the top-left corner still resolves to DIALOG#dialog-54-content — the ambiguity is real — and the dialog stays open. A genuine backdrop click 40px outside the box still closes it, so the fix did not cost dismissal.
2. Focus is restored to the trigger. After Escape, document.activeElement is the Open Native Dialog button. It was <body> before. Letting close() run before unmount and taking the browser's own restoration is the cleaner of the two routes I suggested.
On the decisions:
LockScroll— documenting the difference is a fine answer to the question I asked, and the XML doc says plainly that it is not applied underNativeand why. That is what I wanted: not a parameter that quietly does nothing.IsDialogSupportedAsyncnow caches onlytrue, with a comment naming the prerender/disconnect case. That was the whole concern.
Also checked, because this touches shared files: the default JavaScript path is untouched in behaviour — the standard dialog still opens with role="dialog", still locks body to overflow: hidden, still moves focus inside, and does not render a <dialog>. That is the regression risk for everyone who never opts in, and it is clean.
Merged develop in locally to check: builds clean, 178/178 tests pass, no conflicts.
Thanks for seeing this through — the two bugs were subtle ones, and the fixes address the cause rather than the symptom.
2 participants
As outlined in #376 , there are some design limitations of the current dialog component. It requires to have a
BbPortalHostwithin the same circuit. This makes it not possible to render the portal host in SSR but show a dialog within a WebAssembly component. Additionally, due to the network latency and the roundtrip required by aInteractiveServercall for a dialog, they can feel sluggish.This PR implements the support for the native dialog with a fallback to the interactive dialog if the browser does not support it. The native dialog is nowadays well implemented across many browsers. This change also adds an example for opening the dialog using
OpenAsyncand points out the limitation of the DialogRef, since I ran into this limitation in my own app.The change should be backward compatible but I only verified using the Sample project and my own project.
Note
For the sake of transparency, this PR was partially generated by AI. I used it to refactor, organize and write tests. Smoke tests were conducted manually.
Type of Change
Testing Checklist
Related Issues
#479
#376