fix: popover opened after click twice#115
Conversation
- Changed event listeners from `@click` to `@pointerup` in DropdownMenu and PillButtonWrapper components for improved responsiveness. - Added `max-height` style to suggestion popover for better layout control.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe event handlers for user interactions in dropdown menus, suggestion pills, and popovers were updated from Changes
Poem
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/components/src/suggestion-popover/index.vue (1)
283-284: Redundantheight+max-height– consider making height flexible
height: v-bind('toCssUnit(props.popoverHeight)')forces a fixed box even when the content is shorter, defeating the purpose of the newmax-height: 100dvh;.
If the goal is “cap, but don’t stretch”, drop the fixed height or move it tomin-heightinstead.packages/components/src/suggestion-pills/components/PillButtonWrapper.vue (1)
23-37: Pointer event choice is fine; tiny typing nit
PointerEventextendsMouseEvent, so passing it to theemit('click', ev, item)signature is valid.
No action required, but consider renaming the emitted event ('pointerup'vs'click') for semantic clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/src/dropdown-menu/index.vue(1 hunks)packages/components/src/suggestion-pills/components/PillButtonWrapper.vue(2 hunks)packages/components/src/suggestion-popover/index.vue(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/components/src/suggestion-popover/index.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:131-133
Timestamp: 2025-05-27T03:35:11.008Z
Learning: In the SuggestionPopover component (packages/components/src/suggestion-popover/index.vue), the click handler can be bound unconditionally because the `show` computed property has a custom setter that prevents state mutations when `props.trigger === 'manual'`. This design centralizes trigger mode logic in the computed property rather than requiring conditional checks in event handlers.
| @pointerup="handleToggleShow" | ||
| > |
There was a problem hiding this comment.
pointerup opening event conflicts with click-based outside-close logic & drops keyboard accessibility
- The popover now opens on
pointerup, butonClickOutside(L150) still listens for the defaultclickevent.
– On many touch devices a synthetic click fires ~300 ms afterpointerup; the outside-click handler will therefore close the popover right after it opens → visible flicker / impossible to interact. - Replacing
clickalso removes the implicit keyboard fallback (Enter/Spacegenerate a click but not a pointer event).
// keep the trigger semantics but listen to the same event type + add KB fallback
-if (props.trigger === 'click') {
- onClickOutside(popoverRef, handleOutside, /* default => click */)
+if (props.trigger === 'click') {
+ onClickOutside(
+ popoverRef,
+ handleOutside,
+ { event: 'pointerup' } // stay in the same event stream
+ )
}
...
<div
ref="popoverTriggerRef"
- @pointerup="handleToggleShow"
+ @pointerup="handleToggleShow"
+ @keyup.enter.space="handleToggleShow"
>Failing to align the two streams is a breaking UX bug on iOS/Android; please patch before merge.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pointerup="handleToggleShow" | |
| > | |
| // keep the trigger semantics but listen to the same event type + add KB fallback | |
| if (props.trigger === 'click') { | |
| onClickOutside( | |
| popoverRef, | |
| handleOutside, | |
| { event: 'pointerup' } // stay in the same event stream | |
| ) | |
| } | |
| // … | |
| <template> | |
| <div | |
| ref="popoverTriggerRef" | |
| @pointerup="handleToggleShow" | |
| @keyup.enter.space="handleToggleShow" | |
| > | |
| <!-- popover trigger content --> | |
| </div> | |
| </template> |
🤖 Prompt for AI Agents
In packages/components/src/suggestion-popover/index.vue around lines 215-216,
the popover opens on the pointerup event but the outside-close logic listens for
click events, causing a conflict and flicker on touch devices. To fix this,
align the outside-close event listener to also use pointerup instead of click,
ensuring consistent behavior across devices and preserving keyboard
accessibility by handling keyboard events separately or ensuring they still
trigger the close logic appropriately.
| @pointerup="handleToggleShow" | ||
| > |
There was a problem hiding this comment.
Same pointer/click mismatch as above – dropdown closes immediately after opening
@pointerup opens the menu, but onClickOutside (L34) still reacts to click.
Touch devices will emit the delayed click → menu closes at once.
Apply the same fix as for the popover:
-onClickOutside(dropdownMenuRef, handleOutside)
+onClickOutside(dropdownMenuRef, handleOutside, { event: 'pointerup' })
...
-@pointerup="handleToggleShow"
+@pointerup="handleToggleShow"
+@keyup.enter.space="handleToggleShow"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pointerup="handleToggleShow" | |
| > | |
| @pointerup="handleToggleShow" | |
| @keyup.enter.space="handleToggleShow" | |
| > |
🤖 Prompt for AI Agents
In packages/components/src/dropdown-menu/index.vue around lines 64 to 65, the
dropdown menu uses @pointerup to open but listens for click events on outside
clicks, causing the menu to close immediately on touch devices due to delayed
click events. To fix this, change the outside click listener on line 34 to
listen for pointerup events instead of click, matching the open event and
preventing immediate closure on touch devices.
gene9831
changed the title
…uggestion components - Introduced a `manual` trigger option for DropdownMenu and SuggestionPopover to allow external control of visibility. - Implemented `@click-outside` event to close the dropdown and popover when clicking outside. - Updated documentation to include new properties and event details for both components.
1 participant
TODO 后面移动端要移除当hover才显示的可交互元素
临时方案：
@clickto@pointerupin DropdownMenu and PillButtonWrapper components for improved responsiveness.max-heightstyle to suggestion popover for better layout control.在 iOS 移动端，:hover 的确会干扰首次点击事件，导致用户需要点击两次才能触发 click —— 第一次触发 :hover，第二次才触发事件。这是因为：
iOS Safari 模拟了 :hover 行为，即使是在触屏设备上。
Summary by CodeRabbit
New Features
Style
Refactor