[Bug Fix] Overlays: release Command scroll lock on teardown, settle Tooltip exit on cancel - #535
djalmaaraujo wants to merge 2 commits into
Conversation
…tip exit on animationcancel
![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 3 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/command/command_controller.js">
<violation number="1" location="gem/lib/ruby_ui/command/command_controller.js:23">
P2: When another overlay is still open, this teardown clears its shared `overflow-hidden` lock and re-enables page scrolling. Use a shared reference-counted lock, or release the lock only when no other overlay still owns it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| disconnect() { | ||
| // Torn down without dismiss() (Turbo nav/morph) never runs afterExit, which is | ||
| // the only place the <body> scroll lock is released. Release it here regardless. | ||
| document.body.classList.remove("overflow-hidden"); |
There was a problem hiding this comment.
P2: When another overlay is still open, this teardown clears its shared overflow-hidden lock and re-enables page scrolling. Use a shared reference-counted lock, or release the lock only when no other overlay still owns it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/command/command_controller.js, line 23:
<comment>When another overlay is still open, this teardown clears its shared `overflow-hidden` lock and re-enables page scrolling. Use a shared reference-counted lock, or release the lock only when no other overlay still owns it.</comment>
<file context>
@@ -18,6 +18,9 @@ export default class extends Controller {
disconnect() {
+ // Torn down without dismiss() (Turbo nav/morph) never runs afterExit, which is
+ // the only place the <body> scroll lock is released. Release it here regardless.
+ document.body.classList.remove("overflow-hidden");
// Nothing is left to wait for the exit animation, so apply the pending removal now.
if (this.hasPanelTarget) this.settleExit(this.panelTarget);
</file context>
There was a problem hiding this comment.
Valid, but pre-existing and systemic rather than introduced here: dialog, alert_dialog, command and command_dialog all toggle the same overflow-hidden body class and every one already removes it unconditionally on its own close, so closing any one while another is open unlocks scroll on main today. A ref-counted lock is the right fix, but it spans those four controllers and changes their normal-close behaviour — out of scope for this teardown-leak follow-up, worth its own PR. I have narrowed the added removal to fire only when the panel is still open (the actual torn-down-mid-open leak), so it no longer touches the lock on a normal-close disconnect.
…n every disconnect
2 participants
Follow-up to #506. That PR moved every overlay's hide behind the exit animation. Two overlays still leak on an interrupted/torn-down close.
CommandDialog — body stays scroll-locked after teardown
show()addsoverflow-hiddento<body>; it is removed only inafterExit(). When the dialog is destroyed without adismiss()(Turbo navigation,turbo:morphon a persisted body),disconnect()→settleExit(panel)seesdata-state="open"and returns early, soafterExit()never runs and the page stays unscrollable.Fix:
disconnect()releases the lock unconditionally, independent of exit state.Tooltip — exit run cut short orphans the cloned node
Tooltip mounts a clone on
<body>and removes it onanimationend. It never listened foranimationcancel, so an exit animation interrupted by a backgrounded tab or a competing style change leaves the clone in the DOM forever. The other overlays already listen for both events; this brings Tooltip in line. The existinganimationName === "exit"+data-state === "closed"guards already do the right thing for the cancel case.How to test
CommandDialog scroll lock
<body>gainsoverflow-hidden).overflow-hiddenis stuck on<body>. After: the page scrolls normally.Tooltip orphan
document.body. After: it is removed.Both are JS-only lifecycle fixes;
mcp/data/registry.jsonis regenerated from the two controllers.Summary by cubic
Fixes CommandDialog leaving
<body>scroll-locked when Turbo navigation or morph tears it down mid-open, and fixes Tooltip leaving a cloned node behind when its exit animation is cancelled.Bug Fixes
overflow-hiddenscroll lock ondisconnect()only when the panel is still open; the normal close path already releases it inafterExit().animationcancelin addition toanimationend, so a cut-short exit removes the cloned node.mcp/data/registry.jsonis regenerated from the two controllers.Written for commit 93f5af5. Summary will update on new commits.