[Bug Fix] Sheet: render a native <dialog> and close on Escape by tvq · Pull Request #520 · ruby-ui/ruby_ui

tvq · 2026-08-29T10:38:29Z

Problem

SheetContent rendered a <template> that the controller cloned to the end of <body> on open: no Escape, no focus trap/restore, the page behind stayed interactive, and re-cloning the panel threw away anything the user had typed into it.
Gaps against the shadcn Sheet API: no SheetClose, no showCloseButton equivalent (so MobileSidebar hid the corner button with a [&>button]:hidden hack), no default width (every docs example had to pass sm:max-w-sm by hand), and no data-side to target one side from the outside.

Change

Native <dialog> rendered in place, opened with showModal() — top layer, inert background, focus trap and focus return from the platform. No template/clone. In shadcn all four registry variants build Sheet from the Dialog primitive ("Extends the Dialog component"), which is where these come from there.
Escape closes: cancel is intercepted so the exit animates first, then dialog.close(). The controller waits on the exit animations themselves, as Dialog does in [Bug Fix] Dialog: play the exit animation before closing the native <dialog> #517: close() sets data-state="closed", collects the panel's and the ::backdrop's CSSAnimations (getAnimations({subtree: true}) on the <dialog>, filtered to its own effect.target), waits on their finished promises with a token per run so a superseded run cannot settle a later one, then calls close(). This differs from the animationend block of [Bug Fix] Overlays: play the exit animation before hiding #506 because getAnimations() alone never lists ::backdrop and the backdrop's events land on the <dialog> under the same keyframe names: that block closed on whichever exit ended first, and a stale animationcancel from a previous run could close a new one early. A second Escape mid-exit is left to the browser (non-cancelable in Chrome anyway); a close the controller did not start settles the pending run.
The backdrop gets backdrop:duration-300 so it fades out in step with the panel's 300 ms exit (--tw-duration is registered inherits: false, so ::backdrop does not pick the panel's duration up). The controller does not depend on the two matching: a duration-* override on SheetContent closes after the longer of the two.
The body scroll lock is released only when no modal <dialog> is left open (dialog:modal), so a Sheet nested in a Dialog, or one disconnecting behind another, keeps the page locked.
Backdrop click closes (shadcn parity). On a <dialog> a click on the panel's own padding targets the same element, so backdropClick hit-tests the panel box and ignores it.
UA <dialog> resets: a modal dialog is pinned to all four edges by inset: 0, so each side releases the opposite one, plus m-0 max-w-full max-h-full. not-open:hidden keeps a caller's bare flex from overriding the UA display: none when closed — the docs' own theme sheet passes one.
API parity, closed here rather than in a follow-up since the component was being rewritten anyway: SheetClose wrapper, show_close_button: on SheetContent (replaces the [&>button]:hidden hack), default w-3/4 sm:max-w-sm for left/right, and data-side on the panel. A caller's own classes still win through tailwind_merge.
Both controllers stay: ruby-ui--sheet#open on the wrapper, ruby-ui--sheet-content#close on the dialog, so the public action strings apps already have in their markup keep working.
Docs: the Side example now shows all four sides, plus a new "No close button" section.
Unchanged on purpose: backdrop stays bg-background/80 backdrop-blur-sm (consistent with Dialog/AlertDialog, not shadcn's bg-black/50), and header/footer padding stays on the panel — moving it would reflow every existing sheet.
mcp/data/registry.json rebuilt (separate commit).

Test

cd gem && bundle exec rake — 15 tests in sheet_test.rb.
Manual, on the Sheet docs page:
1. Open → panel slides in over 500 ms, backdrop fades in, page scroll is locked.
2. Escape / the corner X / the footer's Cancel → exit animation plays, then it closes, focus returns to the trigger, scroll is restored.
3. Click the backdrop → closes. Click the panel's own padding next to the content → stays open.
4. All four sides, the "No close button" example (Escape and the footer Close still work), and the mobile menu on a narrow viewport.
Timing and geometry checked in headless Chrome (CDP): exit reaches animationend at 300 ms on both panel and ::backdrop and close fires once; reopen mid-exit comes back; reopen + close again inside one frame still plays the full 300 ms exit (25 ms before the controller change); a duration-* override on the panel or the backdrop closes after the longer one; requestClose() mid-exit closes at once and a second Escape mid-exit (non-cancelable) settles; removal mid-exit and an external close() both leave scroll unlocked; each side lands on its edge, and the default width resolves to min(75vw, 24rem).

🤖 Generated with Claude Code

Summary by cubic

Renders SheetContent as a native <dialog> opened with showModal() instead of cloning a <template> to the end of <body> — the page behind is now inert, focus is trapped and restored, Escape closes the sheet, and reopening no longer throws away typed content. The existing ruby-ui--sheet#open and ruby-ui--sheet-content#close action strings keep working unchanged.

Bug Fixes

Escape is intercepted so the exit animation plays before the dialog closes; a second Escape mid-exit settles correctly.
The close now waits on the panel's and ::backdrop's animation finished promises, so a stale exit run from reopening mid-close can't close the dialog early, and a mismatched duration can't cut either exit short.
Backdrop clicks close the sheet; clicks on the panel's own padding do not.
The body scroll lock is released only when no modal <dialog> is left open, so one sheet closing behind another no longer unlocks the page.

New Features

Adds a SheetClose wrapper and a show_close_button: option on SheetContent; MobileSidebar now uses show_close_button: false instead of its [&>button]:hidden hack.
Left and right sides get a default w-3/4 sm:max-w-sm width, and the panel exposes data-side for targeting one side from outside.

^{Written for commit 3d7d85c. Summary will update on new commits.}

Sheet was the last overlay built on the old pattern: a <template> cloned onto <body> with insertAdjacentHTML, wrapped in plain divs. That shape has no Escape handling, no focus trap, no inert background and no focus restore, and re-cloning the panel on every open threw away whatever the user had typed into it. In shadcn all four registry variants build Sheet out of the Dialog primitive ("Extends the Dialog component"), which is where those behaviours come from. SheetContent is now a native <dialog> opened with showModal(), so the browser provides them. The cancel event is intercepted so Escape plays the exit animation before the dialog actually closes, matching Dialog (ruby-ui#517) and AlertDialog (ruby-ui#518) and reusing their exit block. Since the component was rewritten anyway, the gaps against the shadcn API are closed here too rather than in a follow-up: a SheetClose wrapper, a show_close_button: option (which replaces the [&>button]:hidden hack in MobileSidebar), the default w-3/4 sm:max-w-sm width for the left and right sides, and data-side on the panel so a caller can target one side. A modal <dialog> is pinned to every edge by inset: 0, so each side now releases the opposite one and the box gets m-0/max-w-full/max-h-full. not-open:hidden guards the UA display: none against a caller passing a bare flex. The backdrop's exit lasts as long as the panel's because its animationend is dispatched on the <dialog> under the same keyframe name. Clicking the backdrop closes; clicking the panel's own padding, which targets the same element, does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

cubic-dev-ai

1 issue found across 9 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/sheet/sheet_close.rb">

<violation number="1" location="gem/lib/ruby_ui/sheet/sheet_close.rb:6">
P2: SheetClose renders a non-interactive <div> as the close control, so a bare SheetClose with no button child is not focusable, not keyboard-activatable, and not announced by screen readers. This PR's goal is alignment with the shadcn Sheet API, where SheetClose is a <button>, and the component's own close_button is already a real <button>. Render a <button type="button"> so the wrapper is interactive regardless of its contents.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

cubic-dev-ai · 2026-08-29T10:45:08Z

+module RubyUI
+  class SheetClose < Base
+    def view_template(&)
+      div(**attrs, &)


P2: SheetClose renders a non-interactive
as the close control, so a bare SheetClose with no button child is not focusable, not keyboard-activatable, and not announced by screen readers. This PR's goal is alignment with the shadcn Sheet API, where SheetClose is a , and the component's own close_button is already a real . Render a so the wrapper is interactive regardless of its contents.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/sheet/sheet_close.rb, line 6: <comment>SheetClose renders a non-interactive <div> as the close control, so a bare SheetClose with no button child is not focusable, not keyboard-activatable, and not announced by screen readers. This PR's goal is alignment with the shadcn Sheet API, where SheetClose is a <button>, and the component's own close_button is already a real <button>. Render a <button type="button"> so the wrapper is interactive regardless of its contents.</comment> <file context> @@ -0,0 +1,17 @@ +module RubyUI + class SheetClose < Base + def view_template(&) + div(**attrs, &) + end + </file context>

Keeping the <div>. SheetClose follows this library's wrapper convention: SheetTrigger, DialogTrigger, PopoverTrigger (and DrawerClose in #519) are <div>s carrying the action, with the caller supplying the Button. That is the usage every docs example shows (SheetClose { Button(variant: :outline) { "Cancel" } }), the equivalent of shadcn's asChild. Rendering a <button> here would nest a button inside a button in that documented usage, which is invalid HTML. A bare SheetClose { "Cancel" } is as non-interactive as a bare SheetTrigger { "Open" }; the wrapper is the API.

Review follow-ups on ruby-ui#520: - The body scroll lock was lifted unconditionally, by `ruby-ui--sheet` on disconnect and by `ruby-ui--sheet-content` on close. A Sheet disconnecting behind another open Sheet (or a Dialog) unlocked the page under the remaining modal, and a <dialog> removed while open fires no close event, so its lock outlived it. The release now lives in the content controller alone, guarded by `dialog:modal`: the class goes only when no modal <dialog> is left in the document. The wrapper's `disconnect()` is gone; the content controller disconnects whenever the wrapper does, and also when only the <dialog> is replaced. - Merge the two identical corner-button tests; the survivor also checks `type="button"`, so the button cannot submit a form around the content. - Docs: Save is a plain Button in both examples, as in the Dialog docs; a submit button without a form did nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

cubic-dev-ai

1 issue found across 6 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="gem/lib/ruby_ui/sheet/sheet_content_controller.js">

<violation number="1" location="gem/lib/ruby_ui/sheet/sheet_content_controller.js:16">
P2: When Stimulus disconnects this controller while the dialog is still open, the helper treats that dialog as another lock owner and leaves `overflow-hidden` on the body. Close the still-open dialog during teardown, or exclude the disconnected element from the modal check while retaining checks for other dialogs.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

cubic-dev-ai · 2026-09-04T22:15:27Z

+    // Nothing is left to wait for the exit animation, so apply the pending close now.
+    this.settleExit(this.element);
+    // Removed while open, the dialog fires no close event; the lock must not outlive it.
+    this.releaseScrollLock();


P2: When Stimulus disconnects this controller while the dialog is still open, the helper treats that dialog as another lock owner and leaves overflow-hidden on the body. Close the still-open dialog during teardown, or exclude the disconnected element from the modal check while retaining checks for other dialogs.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/sheet/sheet_content_controller.js, line 16: <comment>When Stimulus disconnects this controller while the dialog is still open, the helper treats that dialog as another lock owner and leaves `overflow-hidden` on the body. Close the still-open dialog during teardown, or exclude the disconnected element from the modal check while retaining checks for other dialogs.</comment> <file context> @@ -12,6 +12,8 @@ export default class extends Controller { // Nothing is left to wait for the exit animation, so apply the pending close now. this.settleExit(this.element); + // Removed while open, the dialog fires no close event; the lock must not outlive it. + this.releaseScrollLock(); } </file context>

…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>

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>

tvq and others added 2 commits August 29, 2026 12:33

chore(mcp): rebuild registry after the Sheet change

9ed43db

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

tvq requested a review from cirdes as a code owner August 29, 2026 10:38

cubic-dev-ai Bot reviewed Aug 29, 2026

View reviewed changes

cirdes requested review from djalmaaraujo and removed request for cirdes August 31, 2026 11:54

tvq and others added 2 commits September 5, 2026 00:03

chore(mcp): rebuild registry after the Sheet scroll-lock change

ea5ef2d

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

cubic-dev-ai Bot reviewed Sep 4, 2026

View reviewed changes

tvq and others added 2 commits September 5, 2026 00:20

chore(mcp): rebuild registry after the Sheet exit block change

3d7d85c

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

tvq mentioned this pull request Sep 4, 2026

[Bug Fix] Dialog: play the exit animation before closing the native <dialog> #517

Open

Navigation Menu

Uh oh!

[Bug Fix] Sheet: render a native <dialog> and close on Escape - #520

[Bug Fix] Sheet: render a native <dialog> and close on Escape#520
tvq wants to merge 6 commits into
ruby-ui:mainfrom
tvq:fix_sheet

tvq commented Aug 29, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot Aug 29, 2026

Uh oh!

tvq Sep 4, 2026

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

cubic-dev-ai Bot Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

tvq commented Aug 29, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Test

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

tvq Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

tvq commented Aug 29, 2026 •

edited by cubic-dev-ai Bot

Loading

cubic-dev-ai Bot left a comment •

edited

Loading