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.
55b35ae to
d7e8992
Compare
d7e8992 to
ffd0a09
Compare
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