Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion gem/lib/ruby_ui/combobox/combobox.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ def default_attrs
controller: "ruby-ui--combobox",
ruby_ui__combobox_term_value: @term,
ruby_ui__combobox_placement_value: @placement,
action: "turbo:morph@window->ruby-ui--combobox#updateTriggerContent"
action: %w[
turbo:morph@window->ruby-ui--combobox#updateTriggerContent
click@window->ruby-ui--combobox#handleOutsideClick
keydown.esc@window->ruby-ui--combobox#handleEscape
]
}
}
end
Expand Down
89 changes: 85 additions & 4 deletions gem/lib/ruby_ui/combobox/combobox_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,36 @@ export default class extends Controller {
}

disconnect() {
if (this.cleanup) { this.cleanup() }
this.stopAutoUpdate()
// Nothing is left to wait for the exit animation, so apply the pending hide now.
if (this.hasPopoverTarget) this.settleExit(this.popoverTarget)
}

handlePopoverToggle(event) {
// Keep ariaExpanded in sync with the actual popover state
this.triggerTarget.ariaExpanded = event.newState === 'open' ? 'true' : 'false'
}

// Still true while the exit animation runs; the window handlers also reach a combobox rendered without a popover.
get popoverShowing() {
return this.hasPopoverTarget && this.popoverTarget.matches(":popover-open")
}

handleOutsideClick(event) {
if (!this.popoverShowing) return
if (this.popoverTarget.contains(event.target)) return
if (this.triggerTarget.contains(event.target)) return

this.closePopover()
}

handleEscape(event) {
if (!this.popoverShowing) return

event.preventDefault()
this.closePopover()
}

inputChanged(e) {
this.updateTriggerContent()

Expand Down Expand Up @@ -87,12 +109,60 @@ export default class extends Controller {
this.triggerTarget.ariaExpanded = "true"
this.selectedItemIndex = null
this.itemTargets.forEach(item => item.ariaCurrent = "false")
this.popoverTarget.showPopover()
this.popoverTarget.dataset.state = "open"
// Reopened mid-exit: it is still showing, and the state flip alone brings it back in.
if (!this.popoverShowing) this.popoverTarget.showPopover()
}

closePopover() {
if (this.popoverTarget.dataset.state === "closed") return

this.triggerTarget.ariaExpanded = "false"
this.popoverTarget.hidePopover()
this.popoverTarget.dataset.state = "closed"
this.hideAfterExitAnimation(this.popoverTarget)
}

// Positioning keeps running until the popover is hidden, so it does not drift while it fades.
afterExit(popover) {
popover.hidePopover()
this.stopAutoUpdate()
}

// Overlay exit — the same block in every overlay controller, so keep them in sync.
exitAnimationNames = new WeakMap();

hideAfterExitAnimation(animated) {
const exitAnimations = animated
.getAnimations()
.filter((animation) => animation instanceof CSSAnimation);

// No exit animation, or no box to run it in: animationend would never fire.
if (exitAnimations.length === 0) {
this.settleExit(animated);
return;
}

this.exitAnimationNames.set(animated, exitAnimations.map((animation) => animation.animationName));
animated.addEventListener("animationend", this.handleExitAnimationEnd);
animated.addEventListener("animationcancel", this.handleExitAnimationEnd);
}

handleExitAnimationEnd = (event) => {
// animationend bubbles — an animated child must not hide its container.
if (event.target !== event.currentTarget) return;
// Closing mid-open cancels the enter animation; only the exit run settles this.
if (!this.exitAnimationNames.get(event.currentTarget)?.includes(event.animationName)) return;

this.settleExit(event.currentTarget);
};

settleExit(animated) {
animated.removeEventListener("animationend", this.handleExitAnimationEnd);
animated.removeEventListener("animationcancel", this.handleExitAnimationEnd);
// Reopened mid-exit: it is on its way back in, leave it visible.
if (animated.dataset.state !== "closed") return;

this.afterExit(animated);
}

filterItems(e) {
Expand Down Expand Up @@ -174,19 +244,30 @@ export default class extends Controller {
}

updatePopoverPosition() {
this.stopAutoUpdate()

this.cleanup = autoUpdate(this.triggerTarget, this.popoverTarget, () => {
computePosition(this.triggerTarget, this.popoverTarget, {
placement: this.placementValue,
middleware: [offset(4), flip()],
}).then(({ x, y }) => {
}).then(({ x, y, placement }) => {
Object.assign(this.popoverTarget.style, {
left: `${x}px`,
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]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

});
});
}

stopAutoUpdate() {
if (!this.cleanup) return

this.cleanup()
this.cleanup = null
}

updatePopoverWidth() {
const width = Math.max(this.triggerTarget.offsetWidth, this.minPopoverWidthValue)
this.popoverTarget.style.width = `${width}px`
Expand Down
13 changes: 9 additions & 4 deletions gem/lib/ruby_ui/combobox/combobox_popover.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,23 @@ def view_template(&)

def default_attrs
{
class: "inset-auto m-0 absolute border bg-background shadow-lg rounded-lg",
role: "popover",
class: [
"inset-auto m-0 absolute border bg-background shadow-lg rounded-lg",
"data-[state=open]:animate-in data-[state=open]:fade-in-0 data-[state=open]:zoom-in-95",
"data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95",
"data-[state=closed]:fill-mode-forwards duration-100",
"data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2",
"data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2"
],
autofocus: true,
popover: true,
popover: "manual",
data: {
ruby_ui__combobox_target: "popover",
action: %w[
toggle->ruby-ui--combobox#handlePopoverToggle
keydown.down->ruby-ui--combobox#keyDownPressed
keydown.up->ruby-ui--combobox#keyUpPressed
keydown.enter->ruby-ui--combobox#keyEnterPressed
keydown.esc->ruby-ui--combobox#closePopover:prevent
resize@window->ruby-ui--combobox#updatePopoverWidth
]
}
Expand Down
58 changes: 57 additions & 1 deletion gem/test/ruby_ui/combobox_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ def test_combobox_keyboard_actions_on_controller
assert_match(/keydown\.down/, output)
assert_match(/keydown\.up/, output)
assert_match(/keydown\.enter/, output)
assert_match(/keydown\.esc/, output)
end

def test_combobox_input_trigger_focusin_action
Expand All @@ -249,4 +248,61 @@ def test_combobox_popover_has_autofocus
output = phlex { RubyUI.ComboboxPopover { "" } }
assert_match(/autofocus/, output)
end

# The browser's light dismiss hides an auto popover before any exit animation can run.
def test_combobox_popover_is_a_manual_popover
output = phlex { RubyUI.ComboboxPopover { "options" } }

assert_match(/popover="manual"/, output)
refute_match(/role="popover"/, output)
end

# data-state is set by the controller on open; rendering it closed would start an exit run.
def test_combobox_popover_renders_without_state
output = phlex { RubyUI.ComboboxPopover { "options" } }

refute_match(/data-state/, output)
refute_match(/data-side/, output)
end

def test_combobox_popover_animates_open_and_closed
output = phlex { RubyUI.ComboboxPopover { "options" } }

assert_match(/data-\[state=open\]:animate-in/, output)
assert_match(/data-\[state=open\]:fade-in-0/, output)
assert_match(/data-\[state=open\]:zoom-in-95/, output)
assert_match(/data-\[state=closed\]:animate-out/, output)
assert_match(/data-\[state=closed\]:fade-out-0/, output)
assert_match(/data-\[state=closed\]:zoom-out-95/, output)
assert_match(/\bduration-100\b/, output)
end

# hidePopover() lands a frame after the animation ends; without a forwards fill mode that frame flashes.
def test_combobox_popover_holds_the_last_frame_of_the_exit_animation
output = phlex { RubyUI.ComboboxPopover { "options" } }

assert_match(/data-\[state=closed\]:fill-mode-forwards/, output)
end

def test_combobox_popover_slides_in_from_the_trigger_side
output = phlex { RubyUI.ComboboxPopover { "options" } }

assert_match(/data-\[side=bottom\]:slide-in-from-top-2/, output)
assert_match(/data-\[side=top\]:slide-in-from-bottom-2/, output)
assert_match(/data-\[side=left\]:slide-in-from-right-2/, output)
assert_match(/data-\[side=right\]:slide-in-from-left-2/, output)
end

def test_combobox_popover_keeps_positioning_classes
output = phlex { RubyUI.ComboboxPopover { "options" } }

assert_match(/inset-auto m-0 absolute/, output)
end

def test_combobox_owns_outside_click_and_escape
output = phlex { RubyUI.Combobox { "" } }

assert_match(/click@window->ruby-ui--combobox#handleOutsideClick/, output)
assert_match(/keydown\.esc@window->ruby-ui--combobox#handleEscape/, output)
end
end
Loading