[Bug Fix] Dialog: play the exit animation before closing the native <dialog> - #517
[Bug Fix] Dialog: play the exit animation before closing the native <dialog>#517tvq wants to merge 8 commits into
Conversation
…dialog> dialog.close() drops the open attribute — and with it display — in the same frame, so the panel and its ::backdrop vanished with a hard cut, and Escape took the native path untouched. Every other overlay animates out since ruby-ui#506; Dialog now does the same. - DialogContent keys its animations on data-state (set by the controller) instead of the open: variant, so the closed state can still render the exit. open:flex stays, so a closed dialog remains hidden. - The backdrop gets backdrop:duration-200: its animationend is dispatched on the <dialog> under the same keyframe name, so both exits must end together or the shorter one closes the dialog early. --tw-duration is registered with inherits: false, so ::backdrop does not pick the panel's duration up on its own. - dismiss() sets data-state="closed", waits for the exit with the shared overlay block, then calls close(). cancel (Escape, requestClose()) is intercepted and routed through it. A close the controller did not start (a second Escape mid-exit is non-cancelable in Chrome) settles the listeners. closedby="any" is not used: it would double-fire with the click handler and Safari support is recent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 4 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/dialog/dialog_controller.js">
<violation number="1" location="gem/lib/ruby_ui/dialog/dialog_controller.js:60">
P2: When a second Escape arrives during an existing exit, this unconditional `preventDefault()` blocks browsers that expose that `cancel` event as cancelable. Return before preventing the event when `data-state` is already `closed`.</violation>
<violation number="2" location="gem/lib/ruby_ui/dialog/dialog_controller.js:75">
P2: `getAnimations()` does not include `::backdrop` animations by default, so this controller never tracks the backdrop exit. The native dialog closes on the panel's event, cutting off the backdrop whenever their timing differs. Track the `::backdrop` animation explicitly and settle only after the required exits complete.</violation>
<violation number="3" location="gem/lib/ruby_ui/dialog/dialog_controller.js:84">
P1: When a dialog is reopened and dismissed again before the prior animation event is delivered, the stale `exit` event matches this name-only map and closes the new run immediately. Track a dismissal generation or the specific animation run so events from older exits cannot settle the current one.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
shadcn's dialog demo opens from <Button variant="outline">; the primary button is reserved for the footer action (Save), with Cancel as outline. Sheet and Drawer docs already follow that convention — this brings Dialog in line so triggers read the same across all overlays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shadcn's DialogContent takes showCloseButton (default true) so a dialog can drop the corner close control and offer its own; the option is unchanged across all three of their current bases. DialogContent renders the × itself, so without an option there is no way to opt out. Adds a "No close button" docs example alongside it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cluded
The exit block matched animationend/animationcancel by keyframe name,
which left two holes on a native <dialog>:
- Reopened mid-exit and dismissed again before the next frame, the first
run's animationcancel still matched "exit" and closed the dialog under
the second run (31 ms instead of 200 ms in headless Chrome).
- getAnimations() does not list ::backdrop, so the block only knew the
panel while the backdrop's events arrived under the same name;
whichever ended first closed the dialog and cut the other as soon as
the durations differed (a duration-* override on DialogContent).
dismiss() now collects the panel's and the backdrop's CSSAnimations
(getAnimations({subtree: true}) filtered to effect.target === dialog),
waits on their `finished` promises and settles only when no later
dismiss or close superseded the run. A second Escape mid-exit is no
longer preventDefault()ed: Chrome makes it non-cancelable anyway, so
browsers that keep it cancelable now close at once as well.
Addresses the review comments on ruby-ui#517.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…op included
The exit block matched animationend/animationcancel by keyframe name,
which left two holes on a native <dialog>:
- Reopened mid-exit and dismissed again before the next frame, the first
run's animationcancel still matched "exit" and closed the dialog under
the second run (32 ms instead of 200 ms in headless Chrome).
- getAnimations() does not list ::backdrop, so the block only knew the
panel while the backdrop's events arrived under the same name;
whichever ended first closed the dialog and cut the other as soon as
the durations differed (a duration-* override on AlertDialogContent).
dismiss() now collects the panel's and the backdrop's CSSAnimations
(getAnimations({subtree: true}) filtered to effect.target === dialog),
waits on their `finished` promises and settles only when no later
dismiss or close superseded the run. A second Escape mid-exit is no
longer preventDefault()ed: Chrome makes it non-cancelable anyway, so
browsers that keep it cancelable now close at once as well.
Same block as Dialog in ruby-ui#517.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…luded
The exit block matched animationend/animationcancel by keyframe name,
which left two holes on a native <dialog>:
- Reopened mid-exit and closed again before the next frame, the first
run's animationcancel still matched "exit" and closed the dialog under
the second run (25 ms instead of 300 ms in headless Chrome).
- getAnimations() does not list ::backdrop, so the block only knew the
panel while the backdrop's events arrived under the same name;
whichever ended first closed the dialog and cut the other as soon as
the durations differed (a duration-* override on SheetContent).
close() now collects the panel's and the backdrop's CSSAnimations
(getAnimations({subtree: true}) filtered to effect.target === dialog),
waits on their `finished` promises and settles only when no later close
superseded the run. A second Escape mid-exit is no longer
preventDefault()ed: Chrome makes it non-cancelable anyway, so browsers
that keep it cancelable now close at once as well. Same block as Dialog
in ruby-ui#517. backdrop:duration-300 stays so both fade in step by default;
the controller no longer depends on the two matching.
Addresses cubic's thread on sheet_content_controller.js in ruby-ui#520.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Ports two review fixes from the sibling PRs: - disconnect() reached this.dialogTarget unconditionally; a <dialog> removed before its wrapper made Stimulus throw and left the body locked (ruby-ui#518). The target-specific teardown is guarded with hasDialogTarget, the lock is lifted either way. - The body class was removed unconditionally on close and disconnect. A Dialog closed or removed on top of another modal <dialog> (a nested Dialog, a Sheet) unlocked the page under the remaining one (ruby-ui#520). The release is now guarded by `dialog:modal`: the class goes only when no modal <dialog> is left in the document. Verified in headless Chrome: dialog removed before its wrapper — no error, lock lifted; second Dialog closed or removed on top of the first — lock kept while the first is modal, released with it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="mcp/data/registry.json">
<violation number="1" location="mcp/data/registry.json:1336">
P2: When a Dialog closes while a Command palette, CommandDialog, or AlertDialog is still open, releaseScrollLock() finds no `dialog:modal` and removes the shared `overflow-hidden` body class, unlocking the page under the still-open overlay. The guard only accounts for dialogs even though the same body lock is set by the Command and AlertDialog controllers (and Sheet doesn't use the body lock at all), so the comment "a Sheet may still be open underneath; the page stays locked for it" doesn't hold. Coordinate the lock with the other overlays (e.g. a shared reference-counted helper) instead of keying release on `dialog:modal` alone.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| { | ||
| "path": "dialog_controller.js", | ||
| "content": "import { Controller } from \"@hotwired/stimulus\"\n\n// Connects to data-controller=\"ruby-ui--dialog\"\nexport default class extends Controller {\n static targets = [\"dialog\"]\n static values = {\n open: {\n type: Boolean,\n default: false\n },\n }\n\n connect() {\n this.dialogTarget.addEventListener(\"close\", this.handleClose)\n if (this.openValue) {\n this.open()\n }\n }\n\n disconnect() {\n this.dialogTarget.removeEventListener(\"close\", this.handleClose)\n document.body.classList.remove(\"overflow-hidden\")\n }\n\n open(e) {\n e?.preventDefault()\n this.dialogTarget.showModal()\n document.body.classList.add(\"overflow-hidden\")\n }\n\n dismiss() {\n this.dialogTarget.close()\n }\n\n backdropClick(e) {\n if (e.target === this.dialogTarget) {\n this.dismiss()\n }\n }\n\n handleClose = () => {\n document.body.classList.remove(\"overflow-hidden\")\n }\n}\n" | ||
| "content": "import { Controller } from \"@hotwired/stimulus\";\n\n// Connects to data-controller=\"ruby-ui--dialog\"\nexport default class extends Controller {\n static targets = [\"dialog\"];\n static values = {\n open: {\n type: Boolean,\n default: false,\n },\n };\n\n connect() {\n this.dialogTarget.addEventListener(\"close\", this.handleClose);\n this.dialogTarget.addEventListener(\"cancel\", this.handleCancel);\n if (this.openValue) {\n this.open();\n }\n }\n\n disconnect() {\n // The <dialog> may already be gone; the scroll lock must be lifted either way.\n if (this.hasDialogTarget) {\n this.dialogTarget.removeEventListener(\"close\", this.handleClose);\n this.dialogTarget.removeEventListener(\"cancel\", this.handleCancel);\n // Nothing is left to wait for the exit animation, so apply the pending close now.\n this.settleExit(this.dialogTarget);\n }\n // Removed while open, the dialog fires no close event; the lock must not outlive it.\n this.releaseScrollLock();\n }\n\n open(e) {\n e?.preventDefault();\n this.dialogTarget.dataset.state = \"open\";\n // Reopened mid-exit the dialog is still open; showModal() on an open dialog throws in older browsers.\n if (!this.dialogTarget.open) this.dialogTarget.showModal();\n document.body.classList.add(\"overflow-hidden\");\n }\n\n dismiss() {\n if (this.dialogTarget.dataset.state === \"closed\") return;\n\n this.dialogTarget.dataset.state = \"closed\";\n this.hideAfterExitAnimation(this.dialogTarget);\n }\n\n afterExit() {\n this.dialogTarget.close();\n }\n\n backdropClick(e) {\n if (e.target === this.dialogTarget) {\n this.dismiss();\n }\n }\n\n // Escape (and requestClose()) fire cancel; route it through the exit animation.\n handleCancel = (e) => {\n // A cancelled file picker inside the dialog bubbles its own cancel event.\n if (e.target !== this.dialogTarget) return;\n // Already on its way out: let a second Escape close natively where the browser allows it.\n if (this.dialogTarget.dataset.state === \"closed\") return;\n\n e.preventDefault();\n this.dismiss();\n };\n\n handleClose = () => {\n this.releaseScrollLock();\n // A close this controller did not start (a second Escape mid-exit) must not leave a pending exit behind.\n this.settleExit(this.dialogTarget);\n };\n\n // A nested Dialog or a Sheet may still be open underneath; the page stays locked for it.\n releaseScrollLock() {\n if (document.querySelector(\"dialog:modal\")) return;\n\n document.body.classList.remove(\"overflow-hidden\");\n }\n\n // Overlay exit — unlike the other overlays this waits on the Animation objects: the ::backdrop animates too,\n // and its events land on the <dialog> under the same keyframe names as the panel's.\n hideAfterExitAnimation(animated) {\n const run = (this.exitRun = {});\n // subtree: true is what lists the ::backdrop's animation; descendants are filtered back out.\n const exitAnimations = animated\n .getAnimations({ subtree: true })\n .filter((animation) => animation instanceof CSSAnimation && animation.effect?.target === animated);\n\n // No exit animation, or no box to run it in: nothing would ever finish.\n if (exitAnimations.length === 0) {\n this.settleExit(animated);\n return;\n }\n\n // A cancelled exit (reopened mid-exit) counts as finished too.\n Promise.allSettled(exitAnimations.map((animation) => animation.finished)).then(() => {\n // A later dismiss or close owns the dialog now; this run is stale.\n if (this.exitRun !== run) return;\n\n this.settleExit(animated);\n });\n }\n\n settleExit(animated) {\n this.exitRun = null;\n // Reopened mid-exit: it is on its way back in, leave it visible.\n if (animated.dataset.state !== \"closed\") return;\n\n this.afterExit(animated);\n }\n}\n" |
There was a problem hiding this comment.
P2: When a Dialog closes while a Command palette, CommandDialog, or AlertDialog is still open, releaseScrollLock() finds no dialog:modal and removes the shared overflow-hidden body class, unlocking the page under the still-open overlay. The guard only accounts for dialogs even though the same body lock is set by the Command and AlertDialog controllers (and Sheet doesn't use the body lock at all), so the comment "a Sheet may still be open underneath; the page stays locked for it" doesn't hold. Coordinate the lock with the other overlays (e.g. a shared reference-counted helper) instead of keying release on dialog:modal alone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp/data/registry.json, line 1336:
<comment>When a Dialog closes while a Command palette, CommandDialog, or AlertDialog is still open, releaseScrollLock() finds no `dialog:modal` and removes the shared `overflow-hidden` body class, unlocking the page under the still-open overlay. The guard only accounts for dialogs even though the same body lock is set by the Command and AlertDialog controllers (and Sheet doesn't use the body lock at all), so the comment "a Sheet may still be open underneath; the page stays locked for it" doesn't hold. Coordinate the lock with the other overlays (e.g. a shared reference-counted helper) instead of keying release on `dialog:modal` alone.</comment>
<file context>
@@ -1333,7 +1333,7 @@
{
"path": "dialog_controller.js",
- "content": "import { Controller } from \"@hotwired/stimulus\";\n\n// Connects to data-controller=\"ruby-ui--dialog\"\nexport default class extends Controller {\n static targets = [\"dialog\"];\n static values = {\n open: {\n type: Boolean,\n default: false,\n },\n };\n\n connect() {\n this.dialogTarget.addEventListener(\"close\", this.handleClose);\n this.dialogTarget.addEventListener(\"cancel\", this.handleCancel);\n if (this.openValue) {\n this.open();\n }\n }\n\n disconnect() {\n this.dialogTarget.removeEventListener(\"close\", this.handleClose);\n this.dialogTarget.removeEventListener(\"cancel\", this.handleCancel);\n // Nothing is left to wait for the exit animation, so apply the pending close now.\n this.settleExit(this.dialogTarget);\n document.body.classList.remove(\"overflow-hidden\");\n }\n\n open(e) {\n e?.preventDefault();\n this.dialogTarget.dataset.state = \"open\";\n // Reopened mid-exit the dialog is still open; showModal() on an open dialog throws in older browsers.\n if (!this.dialogTarget.open) this.dialogTarget.showModal();\n document.body.classList.add(\"overflow-hidden\");\n }\n\n dismiss() {\n if (this.dialogTarget.dataset.state === \"closed\") return;\n\n this.dialogTarget.dataset.state = \"closed\";\n this.hideAfterExitAnimation(this.dialogTarget);\n }\n\n afterExit() {\n this.dialogTarget.close();\n }\n\n backdropClick(e) {\n if (e.target === this.dialogTarget) {\n this.dismiss();\n }\n }\n\n // Escape (and requestClose()) fire cancel; route it through the exit animation.\n handleCancel = (e) => {\n // A cancelled file picker inside the dialog bubbles its own cancel event.\n if (e.target !== this.dialogTarget) return;\n // Already on its way out: let a second Escape close natively where the browser allows it.\n if (this.dialogTarget.dataset.state === \"closed\") return;\n\n e.preventDefault();\n this.dismiss();\n };\n\n handleClose = () => {\n document.body.classList.remove(\"overflow-hidden\");\n // A close this controller did not start (a second Escape mid-exit) must not leave a pending exit behind.\n this.settleExit(this.dialogTarget);\n };\n\n // Overlay exit — unlike the other overlays this waits on the Animation objects: the ::backdrop animates too,\n // and its events land on the <dialog> under the same keyframe names as the panel's.\n hideAfterExitAnimation(animated) {\n const run = (this.exitRun = {});\n // subtree: true is what lists the ::backdrop's animation; descendants are filtered back out.\n const exitAnimations = animated\n .getAnimations({ subtree: true })\n .filter((animation) => animation instanceof CSSAnimation && animation.effect?.target === animated);\n\n // No exit animation, or no box to run it in: nothing would ever finish.\n if (exitAnimations.length === 0) {\n this.settleExit(animated);\n return;\n }\n\n // A cancelled exit (reopened mid-exit) counts as finished too.\n Promise.allSettled(exitAnimations.map((animation) => animation.finished)).then(() => {\n // A later dismiss or close owns the dialog now; this run is stale.\n if (this.exitRun !== run) return;\n\n this.settleExit(animated);\n });\n }\n\n settleExit(animated) {\n this.exitRun = null;\n // Reopened mid-exit: it is on its way back in, leave it visible.\n if (animated.dataset.state !== \"closed\") return;\n\n this.afterExit(animated);\n }\n}\n"
+ "content": "import { Controller } from \"@hotwired/stimulus\";\n\n// Connects to data-controller=\"ruby-ui--dialog\"\nexport default class extends Controller {\n static targets = [\"dialog\"];\n static values = {\n open: {\n type: Boolean,\n default: false,\n },\n };\n\n connect() {\n this.dialogTarget.addEventListener(\"close\", this.handleClose);\n this.dialogTarget.addEventListener(\"cancel\", this.handleCancel);\n if (this.openValue) {\n this.open();\n }\n }\n\n disconnect() {\n // The <dialog> may already be gone; the scroll lock must be lifted either way.\n if (this.hasDialogTarget) {\n this.dialogTarget.removeEventListener(\"close\", this.handleClose);\n this.dialogTarget.removeEventListener(\"cancel\", this.handleCancel);\n // Nothing is left to wait for the exit animation, so apply the pending close now.\n this.settleExit(this.dialogTarget);\n }\n // Removed while open, the dialog fires no close event; the lock must not outlive it.\n this.releaseScrollLock();\n }\n\n open(e) {\n e?.preventDefault();\n this.dialogTarget.dataset.state = \"open\";\n // Reopened mid-exit the dialog is still open; showModal() on an open dialog throws in older browsers.\n if (!this.dialogTarget.open) this.dialogTarget.showModal();\n document.body.classList.add(\"overflow-hidden\");\n }\n\n dismiss() {\n if (this.dialogTarget.dataset.state === \"closed\") return;\n\n this.dialogTarget.dataset.state = \"closed\";\n this.hideAfterExitAnimation(this.dialogTarget);\n }\n\n afterExit() {\n this.dialogTarget.close();\n }\n\n backdropClick(e) {\n if (e.target === this.dialogTarget) {\n this.dismiss();\n }\n }\n\n // Escape (and requestClose()) fire cancel; route it through the exit animation.\n handleCancel = (e) => {\n // A cancelled file picker inside the dialog bubbles its own cancel event.\n if (e.target !== this.dialogTarget) return;\n // Already on its way out: let a second Escape close natively where the browser allows it.\n if (this.dialogTarget.dataset.state === \"closed\") return;\n\n e.preventDefault();\n this.dismiss();\n };\n\n handleClose = () => {\n this.releaseScrollLock();\n // A close this controller did not start (a second Escape mid-exit) must not leave a pending exit behind.\n this.settleExit(this.dialogTarget);\n };\n\n // A nested Dialog or a Sheet may still be open underneath; the page stays locked for it.\n releaseScrollLock() {\n if (document.querySelector(\"dialog:modal\")) return;\n\n document.body.classList.remove(\"overflow-hidden\");\n }\n\n // Overlay exit — unlike the other overlays this waits on the Animation objects: the ::backdrop animates too,\n // and its events land on the <dialog> under the same keyframe names as the panel's.\n hideAfterExitAnimation(animated) {\n const run = (this.exitRun = {});\n // subtree: true is what lists the ::backdrop's animation; descendants are filtered back out.\n const exitAnimations = animated\n .getAnimations({ subtree: true })\n .filter((animation) => animation instanceof CSSAnimation && animation.effect?.target === animated);\n\n // No exit animation, or no box to run it in: nothing would ever finish.\n if (exitAnimations.length === 0) {\n this.settleExit(animated);\n return;\n }\n\n // A cancelled exit (reopened mid-exit) counts as finished too.\n Promise.allSettled(exitAnimations.map((animation) => animation.finished)).then(() => {\n // A later dismiss or close owns the dialog now; this run is stale.\n if (this.exitRun !== run) return;\n\n this.settleExit(animated);\n });\n }\n\n settleExit(animated) {\n this.exitRun = null;\n // Reopened mid-exit: it is on its way back in, leave it visible.\n if (animated.dataset.state !== \"closed\") return;\n\n this.afterExit(animated);\n }\n}\n"
},
{
</file context>
Closing a
Dialogcut the panel and its backdrop off in the same frame:dialog.close()dropsopen(and with itdisplay) immediately, and Escape went through the native path untouched. Every other overlay animates out since #506; Dialog now does too.DialogContentkeys its animations ondata-state(set by the controller) instead of theopen:variant, so the closed state can still render the exit.open:flexstays — a closed dialog remains hidden.backdrop:duration-200so it fades in step with the panel (--tw-durationis registeredinherits: false, so::backdropdoes not pick the panel's duration up). The controller does not depend on the two matching.dismiss()setsdata-state="closed", waits for the panel's and the backdrop's exit animations (getAnimations({subtree: true})on the<dialog>filtered to its owneffect.target, theirfinishedpromises, a token per run so a superseded run cannot settle a later one), then callsclose(). This differs from theanimationendblock in the other overlays because the::backdrop's events land on the<dialog>under the same keyframe names.cancel(Escape,requestClose()) is routed throughdismiss(); a second one mid-exit is left to the browser (non-cancelable in Chrome anyway). A close the controller did not start settles the pending run. Backdrop click, focus restore and body scroll lock are unchanged.closedby="any"is not used: it would double-fire with the click handler and Safari support is recent.disconnect()guardshasDialogTarget(a<dialog>removed before its wrapper no longer throws and the lock still lifts), and the body class is released only when nodialog:modalis left in the document, so a Dialog closed or removed on top of another modal keeps the page locked for the one underneath.AlertDialog and Combobox get the same treatment in separate PRs.
Test:
/docs/dialog, each example — close via ×, Cancel, Escape and a backdrop click: panel and backdrop fade out (~200 ms) before the dialog disappears, focus returns to the trigger, page scroll is restored. Click inside stays open; reopen mid-exit comes back; open/close quickly still plays the full exit; Escape twice closes on the second press and the next cycle animates normally; aduration-*override onDialogContentcloses after the longer of panel and backdrop. Open a second Dialog on top of the first and close or remove it: the page stays locked until the first one closes.🤖 Generated with Claude Code
Summary by cubic
Fixes Dialog closing so the panel and backdrop animate out before the native
<dialog>closes, and adds ashow_close_buttonoption toDialogContent.Bug Fixes
data-stateinstead of theopen:variant, so the closed state can render the exit;open:flexstill keeps a closed dialog hidden.backdrop:duration-200.dismiss()setsdata-state="closed", waits for the panel and backdrop exit animations to finish, then callsclose(); Escape andrequestClose()route through this path.New Features
DialogContentacceptsshow_close_button: falseto hide the corner close button.Button(variant: :outline)to match the Sheet and Drawer docs.Written for commit a9c48ea. Summary will update on new commits.