[Feature] Combobox: animate the popover open and close - #516
Open
tvq wants to merge 2 commits into
Open
Conversation
The popover had no enter/exit animation, and as a native `auto` popover its outside-click and Escape dismissal ran inside the browser: the hide `beforetoggle` is not cancelable, so there was no point at which an exit could play. Switch it to popover="manual" (still top layer, showPopover/hidePopover and the toggle event) and route every close path — trigger, outside click, Escape, radio selection — through closePopover(). data-state drives the tw-animate-css enter/exit keyframes as in ruby-ui#506; the exit block is copied unchanged, and afterExit() calls hidePopover() and stops autoUpdate only then, so the panel keeps following the trigger while it fades. data-side follows Floating UI's resolved placement so the slide-in comes from the trigger's side. Outside click and Escape are window-level actions on the root: clicking a non-focusable part of the popover moves focus to body, and Escape only prevents default while the popover is showing. Both guard hasPopoverTarget, since a Combobox can render without a popover. Restarting positioning stops the previous autoUpdate first (focusin and click on the input trigger used to leak one). Drop role="popover" (not an ARIA role) and add duration-100 plus the directional slide-in classes for parity with shadcn/ui's Combobox. 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.
1 issue found and verified against the latest diff
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/combobox/combobox_controller.js">
<violation number="1" location="gem/lib/ruby_ui/combobox/combobox_controller.js:259">
P3: The `computePosition().then` callback here makes unconditional async writes to `this.popoverTarget` — including the newly added `dataset.side` — with no `isConnected`/run guard. The sibling popover_controller.js (source of this pattern) guards with `if (!content.isConnected) return;` and run-bound teardown precisely so a stale or detached run cannot overwrite position or write to a removed node. Add an `isConnected` check before writing, and consider using the same run-scoped teardown.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| top: `${y}px`, | ||
| }); | ||
| // flip() can resolve to the opposite side, so the slide-in direction follows the resolved value. | ||
| this.popoverTarget.dataset.side = placement.split("-")[0] |
There was a problem hiding this comment.
P3: The computePosition().then callback here makes unconditional async writes to this.popoverTarget — including the newly added dataset.side — with no isConnected/run guard. The sibling popover_controller.js (source of this pattern) guards with if (!content.isConnected) return; and run-bound teardown precisely so a stale or detached run cannot overwrite position or write to a removed node. Add an isConnected check before writing, and consider using the same run-scoped teardown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At gem/lib/ruby_ui/combobox/combobox_controller.js, line 259:
<comment>The `computePosition().then` callback here makes unconditional async writes to `this.popoverTarget` — including the newly added `dataset.side` — with no `isConnected`/run guard. The sibling popover_controller.js (source of this pattern) guards with `if (!content.isConnected) return;` and run-bound teardown precisely so a stale or detached run cannot overwrite position or write to a removed node. Add an `isConnected` check before writing, and consider using the same run-scoped teardown.</comment>
<file context>
@@ -174,19 +244,30 @@ export default class extends Controller {
top: `${y}px`,
});
+ // flip() can resolve to the opposite side, so the slide-in direction follows the resolved value.
+ this.popoverTarget.dataset.side = placement.split("-")[0]
});
});
</file context>
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
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.
The Combobox popover opened and closed in a hard cut. It now animates in and out like the other overlays.
What changed
ComboboxPopovergets thedata-stateenter/exit classes from [Bug Fix] Overlays: play the exit animation before hiding #506,duration-100and thedata-sideslide-in — parity with shadcn/ui's Combobox.popover="manual". A nativeautopopover's light dismiss hides it directly and the hidebeforetoggleis not cancelable, so no exit could ever play. The controller now owns every close path (trigger, outside click, Escape, radio selection) throughclosePopover(), which waits for the exit animation beforehidePopover()— the [Bug Fix] Overlays: play the exit animation before hiding #506 block, copied unchanged. Positioning keeps running until the popover is hidden, so it does not drift while fading.role="popover"(not an ARIA role). API and examples unchanged.Tested
cd gem && bundle exec rakedata-side=top, keyboard navigation and filtering.Dialog and AlertDialog get their exit animations in sibling PRs.
🤖 Generated with Claude Code
Summary by cubic
Animates the Combobox popover open and close instead of a hard cut. The controller now owns every close path (trigger, outside click, Escape, radio selection) and waits for the exit animation before hiding the popover.
New Features
popover="manual"so a single close path can wait for the exit animation.data-stateenter/exit classes,duration-100, anddata-sideslide-in for parity with shadcn/ui.role="popover"and adds window-level outside-click and Escape handlers, guarded for comboboxes without a popover.Written for commit 2b26f6b. Summary will update on new commits.