[Bug Fix] AlertDialog: render a native <dialog> and play the exit animation - #518
Open
tvq wants to merge 3 commits into
Open
[Bug Fix] AlertDialog: render a native <dialog> and play the exit animation#518tvq wants to merge 3 commits into
tvq wants to merge 3 commits into
Conversation
…mation AlertDialogContent rendered a <template> that the controller cloned to the end of <body> on open and removed on dismiss: closing was a hard cut, there was no Escape handling, no focus trap, no focus restore and the page behind stayed interactive. Render a native <dialog role="alertdialog"> in place and open it with showModal(): top layer, inert background, focus trap and focus return come from the platform. Behaviour matches shadcn/Radix AlertDialog: Escape closes (cancel is intercepted so the exit animates), clicking the backdrop does not, focus lands on Cancel (autofocus), Action dismisses too. Enter and exit animate on the panel and on ::backdrop via data-state and tw-animate-css, settled with the same "Overlay exit" block as ruby-ui#506 (copied unchanged; afterExit() calls dialog.close()). ::backdrop animation events are dispatched on the <dialog> under the same keyframe name, so the backdrop gets backdrop:duration-200 to end together with the panel; with the default 150 ms it settled the close early and cut the panel's exit short. A second Escape during the exit is non-cancelable in Chrome (close watcher) and closes hard; the close listener drops the exit listeners and the body scroll lock so the next open starts clean. The backdrop now uses bg-background/80 like Dialog, Sheet and CommandDialog instead of bg-black/80. Public API and the docs example are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="gem/lib/ruby_ui/alert_dialog/alert_dialog_controller.js">
<violation number="1" location="gem/lib/ruby_ui/alert_dialog/alert_dialog_controller.js:22">
P2: When the dialog target is removed before its controller element, `disconnect()` throws before removing `overflow-hidden`, leaving page scrolling disabled. Guard the target-specific teardown with `hasDialogTarget` while always removing the body class.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+22
to
+25
| this.dialogTarget.removeEventListener("cancel", this.handleCancel); | ||
| this.dialogTarget.removeEventListener("close", this.handleClose); | ||
| // Nothing is left to wait for the exit animation, so apply the pending close now. | ||
| this.settleExit(this.dialogTarget); |
There was a problem hiding this comment.
P2: When the dialog target is removed before its controller element, disconnect() throws before removing overflow-hidden, leaving page scrolling disabled. Guard the target-specific teardown with hasDialogTarget while always removing the body class.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/alert_dialog/alert_dialog_controller.js, line 22:
<comment>When the dialog target is removed before its controller element, `disconnect()` throws before removing `overflow-hidden`, leaving page scrolling disabled. Guard the target-specific teardown with `hasDialogTarget` while always removing the body class.</comment>
<file context>
@@ -11,21 +11,85 @@ export default class extends Controller {
}
+ disconnect() {
+ this.dialogTarget.removeEventListener("cancel", this.handleCancel);
+ this.dialogTarget.removeEventListener("close", this.handleClose);
+ // Nothing is left to wait for the exit animation, so apply the pending close now.
</file context>
Suggested change
| this.dialogTarget.removeEventListener("cancel", this.handleCancel); | |
| this.dialogTarget.removeEventListener("close", this.handleClose); | |
| // Nothing is left to wait for the exit animation, so apply the pending close now. | |
| this.settleExit(this.dialogTarget); | |
| if (this.hasDialogTarget) { | |
| this.dialogTarget.removeEventListener("cancel", this.handleCancel); | |
| this.dialogTarget.removeEventListener("close", this.handleClose); | |
| // Nothing is left to wait for the exit animation, so apply the pending close now. | |
| this.settleExit(this.dialogTarget); | |
| } |
shadcn's alert-dialog demo opens from <Button variant="outline">; the primary button is reserved for the confirming action (Continue), with Cancel as outline. Sheet and Drawer docs already follow that convention — this brings AlertDialog in line so triggers read the same across overlays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Problem
AlertDialogContentrendered a<template>that the controller cloned to the end of<body>and removed on dismiss: closing was a hard cut, no Escape, no focus trap/restore, the page behind stayed interactive.bg-black/80while every other overlay usesbg-background/80.Change
<dialog role="alertdialog">rendered in place, opened withshowModal()— top layer, inert background, focus trap and focus return from the platform. No template/clone, no nested controller.cancelintercepted so the exit animates), backdrop click does not close, focus lands on Cancel (autofocus), Action dismisses too (a link/form submit on it still navigates).::backdropviadata-state+ tw-animate-css, settled with the same "Overlay exit" block as [Bug Fix] Overlays: play the exit animation before hiding #506 (copied unchanged;afterExit()→dialog.close()).::backdropanimation events are dispatched on the<dialog>under the same keyframe name, so the backdrop getsbackdrop:duration-200to end together with the panel (it cannot inherit the panel'sduration-200: Tailwind 4 registers--tw-durationwithinherits: false) — with the default 150 ms it settled the close early and cut the panel's exit short.closelistener clears the exit listeners and the body scroll lock so the next open starts clean.bg-background/80 backdrop-blur-sm. Public API and the docs example unchanged.mcp/data/registry.jsonrebuilt (separate commit).Dialog and Combobox get the same treatment in sibling PRs.
Test
cd gem && bundle exec rake— 13 new tests inalert_dialog_test.rb.AlertDialog(open: true)→ open on page load.animationendat 200 ms on both panel and::backdrop,closefires once, reopen mid-exit and close mid-open settle correctly.🤖 Generated with Claude Code