fix(picker): prevent clearing input when chip menu opens#3849
fix(picker): prevent clearing input when chip menu opens#3849adrianschmidt-bot wants to merge 2 commits intoLundalogik:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a private helper that detects when a limel-chip contains an open limel-menu and updates the picker’s blur/stop-edit handler to skip clearing the input while such a chip menu is open, preserving the input during chip menu interaction. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Picker as Picker Component
participant ChipSet as ChipSet (shadow DOM)
participant Chip as limel-chip
participant Menu as limel-menu
User->>Picker: Clicks chip's 3-dot menu
Picker->>Picker: blur event -> handleStopEditAndBlur()
Picker->>ChipSet: call hasOpenChipMenu()
ChipSet->>Chip: iterate child chips (via shadowRoot)
Chip->>Menu: check for `[open]` attribute in chip.shadowRoot
alt menu is open
Chip-->>Picker: returns true
Picker->>Picker: skip clearing input
else menu not open
Chip-->>Picker: returns false
Picker->>Picker: clear input and stop edit
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/picker/picker.tsx`:
- Around line 558-577: The hasOpenChipMenu method currently traverses
chipSet.shadowRoot and each limel-chip.shadowRoot to detect an open limel-menu;
instead add a public method on the limel-chip-set component (e.g.,
limel-chip-set.hasOpenChipMenu()) that encapsulates this logic so picker can
call this.chipSet.hasOpenChipMenu() instead of reaching into shadow DOMs; update
the picker.tsx to call the new limel-chip-set method (and remove the shadowRoot
queries in hasOpenChipMenu), and implement corresponding logic inside the
limel-chip-set to query its children or maintain state about open menus (using
limel-chip/limel-menu internals) so the picker no longer pierces multiple shadow
boundaries.
| private hasOpenChipMenu(): boolean { | ||
| if (!this.chipSet) { | ||
| return false; | ||
| } | ||
|
|
||
| // Query for any open menus inside the chips | ||
| const chips = this.chipSet.shadowRoot?.querySelectorAll('limel-chip'); | ||
| if (!chips) { | ||
| return false; | ||
| } | ||
|
|
||
| for (const chip of chips) { | ||
| const menu = chip.shadowRoot?.querySelector('limel-menu[open]'); | ||
| if (menu) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find the limel-menu component
fd 'menu.tsx' --type f | head -20Repository: Lundalogik/lime-elements
Length of output: 622
🏁 Script executed:
# Find the limel-chip-set component
fd 'chip-set.tsx' --type f | head -20Repository: Lundalogik/lime-elements
Length of output: 150
🏁 Script executed:
# Look at the limel-menu component to check if 'open' is reflected
rg -n "@Prop" --type ts -A 5 | grep -A 5 "open"Repository: Lundalogik/lime-elements
Length of output: 6974
🏁 Script executed:
# Check limel-chip-set component
cat -n src/components/chip-set/chip-set.tsx | head -100Repository: Lundalogik/lime-elements
Length of output: 4114
🏁 Script executed:
# Check limel-chip component
fd 'chip.tsx' --type f | grep -v examples | grep -v chip-setRepository: Lundalogik/lime-elements
Length of output: 96
🏁 Script executed:
# Check if there are any similar shadow DOM traversals in the codebase
rg "shadowRoot\?" --type ts -B 2 -A 2 | head -80Repository: Lundalogik/lime-elements
Length of output: 4474
🏁 Script executed:
# Check if chip-set or chip have any public methods for checking menu state
rg "@Method()" src/components/chip-set/chip-set.tsx src/components/chip/chip.tsxRepository: Lundalogik/lime-elements
Length of output: 220
🏁 Script executed:
# Check the full chip-set to see if there's any method that could help
wc -l src/components/chip-set/chip-set.tsxRepository: Lundalogik/lime-elements
Length of output: 108
🏁 Script executed:
# Check chip component for menu-related functionality
cat -n src/components/chip/chip.tsx | head -150Repository: Lundalogik/lime-elements
Length of output: 5914
🏁 Script executed:
# Check if chip has any public methods
rg "@Method()" src/components/chip/chip.tsxRepository: Lundalogik/lime-elements
Length of output: 50
🏁 Script executed:
# Look at where limel-menu is rendered in limel-chip
rg -n "limel-menu" src/components/chip/chip.tsx -B 2 -A 2Repository: Lundalogik/lime-elements
Length of output: 343
🏁 Script executed:
# Get a sense of the full context - look at the complete chip render to understand the structure
sed -n '200,400p' src/components/chip/chip.tsxRepository: Lundalogik/lime-elements
Length of output: 5953
🏁 Script executed:
# Check if there are any similar patterns checking menu.open state in the codebase
rg "menu\[open\]|\.open" src/components/chip/chip.tsx src/components/picker/picker.tsx -B 2 -A 2Repository: Lundalogik/lime-elements
Length of output: 399
🏁 Script executed:
# Check what the three `@Method` decorators are in chip-set
rg -n "@Method()" src/components/chip-set/chip-set.tsx -A 3Repository: Lundalogik/lime-elements
Length of output: 442
The shadow DOM traversal pattern is used elsewhere in the codebase, but consider refactoring for better encapsulation.
This method reaches into multiple shadow DOM boundaries (chipSet.shadowRoot → limel-chip → chip.shadowRoot). While this pattern appears in other parts of the lime-elements codebase (e.g., src/components/menu/menu.tsx, src/components/select/select.tsx), adding a public method to limel-chip-set (e.g., hasOpenChipMenu()) would better encapsulate the knowledge of its internal DOM structure and improve maintainability.
Note: The open attribute is correctly reflected on limel-menu (defined with @Prop({ mutable: true, reflect: true }) in menu.tsx line 101), so the CSS selector will work as intended.
🤖 Prompt for AI Agents
In `@src/components/picker/picker.tsx` around lines 558 - 577, The hasOpenChipMenu
method currently traverses chipSet.shadowRoot and each limel-chip.shadowRoot to
detect an open limel-menu; instead add a public method on the limel-chip-set
component (e.g., limel-chip-set.hasOpenChipMenu()) that encapsulates this logic
so picker can call this.chipSet.hasOpenChipMenu() instead of reaching into
shadow DOMs; update the picker.tsx to call the new limel-chip-set method (and
remove the shadowRoot queries in hasOpenChipMenu), and implement corresponding
logic inside the limel-chip-set to query its children or maintain state about
open menus (using limel-chip/limel-menu internals) so the picker no longer
pierces multiple shadow boundaries.
There was a problem hiding this comment.
Thanks for the suggestion! You're right that encapsulating this in limel-chip-set would improve separation of concerns. However, this targeted bug fix follows the existing pattern used elsewhere in the codebase (as you noted in menu.tsx, select.tsx, etc.).
Adding a public hasOpenChipMenu() method to chip-set would be a larger refactoring effort that's out of scope for this fix. If the maintainers feel strongly about it, I'm happy to do it as a follow-up PR — but for now I'd prefer to keep this focused on the immediate bug.
adrianschmidt
force-pushed
the
fix/picker-chip-menu-interaction
branch
from
e180849 to
359fc68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/picker/picker.tsx (2)
279-304: 🧹 Nitpick | 🔵 TrivialThe
rendermethod returns an array literal — prefer<Host>.This is pre-existing, but per the coding guidelines for
.tsxfiles using StencilJS: when returning multiple JSX elements fromrender, wrap them in<Host>instead of an array literal.Suggested refactor
- return [ - <limel-chip-set + return ( + <Host> + <limel-chip-set type="input" ... />, {this.renderDropdown()}, - ]; + </Host> + );As per coding guidelines: "When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element."
537-556: 🧹 Nitpick | 🔵 TrivialShadow DOM traversal should be encapsulated in the chip-set component.
Rather than querying through chipSet.shadowRoot → limel-chip → chip.shadowRoot to detect open menus, consider adding a public method to chip-set (e.g.,
hasOpenChipMenu()) that encapsulates this logic. This reduces fragile coupling to internal component structures and makes the implementation more maintainable if the chip-set's DOM hierarchy changes.The fix itself is sound: the menu opens synchronously when clicked, and the deferred
stopEditevent ensures the detection logic runs after the menu'sopenattribute is already set.
🤖 Fix all issues with AI agents
In `@src/components/picker/picker.tsx`:
- Around line 550-553: The early return in handleStopEditAndBlur when
hasOpenChipMenu() is true skips clearInputField(), leaving stale textValue;
subscribe to the chip menu's close event (or the chip set's exit-edit event) and
call clearInputField() there so the previously skipped blur is honored—update
the picker to listen for the chip menu/Chip component "close" (or "editEnd")
event and invoke clearInputField() (or emptyInput() depending on
emptyInputOnBlur) when that event fires; reference hasOpenChipMenu(),
clearInputField(), handleStopEditAndBlur(), handleInputFieldFocus(),
debouncedSearch(), emptyInputOnBlur and emptyInput() to locate where to add the
listener and the call.
| // Don't clear if a chip's menu is open - focus moved to the menu's portal | ||
| if (this.hasOpenChipMenu()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
After the chip menu closes, no mechanism triggers clearInputField for the skipped blur.
When hasOpenChipMenu() returns true, the early return skips clearInputField(). Once the chip menu later closes and focus moves elsewhere, handleStopEditAndBlur won't fire again (the chip-set already exited edit mode). This leaves textValue with stale search text. If the user re-enters the picker, handleInputFieldFocus runs debouncedSearch with the old textValue, and the input field may still display the previous query (since emptyInputOnBlur is false and emptyInput() was never called).
Consider listening for the chip menu's close and calling clearInputField() at that point, or clearing only the search results/items while preserving the guard against the immediate blur.
🤖 Prompt for AI Agents
In `@src/components/picker/picker.tsx` around lines 550 - 553, The early return in
handleStopEditAndBlur when hasOpenChipMenu() is true skips clearInputField(),
leaving stale textValue; subscribe to the chip menu's close event (or the chip
set's exit-edit event) and call clearInputField() there so the previously
skipped blur is honored—update the picker to listen for the chip menu/Chip
component "close" (or "editEnd") event and invoke clearInputField() (or
emptyInput() depending on emptyInputOnBlur) when that event fires; reference
hasOpenChipMenu(), clearInputField(), handleStopEditAndBlur(),
handleInputFieldFocus(), debouncedSearch(), emptyInputOnBlur and emptyInput() to
locate where to add the listener and the call.
There was a problem hiding this comment.
Looking at this more carefully, I believe the behavior is consistent and correct:
- The picker sets
emptyInputOnBlur={false}on the chip-set (line 299) - This means when chip-set exits edit mode on blur, it does NOT clear its visual input (
syncEmptyInput()is only called whenemptyInputOnBluris true — see chip-set.tsx lines 519-521) - So both the chip-set's visual input AND the picker's
textValueretain the search text
When the menu closes and the user later refocuses on the picker:
- The visual input still displays their previous search text (chip-set never cleared it)
- The picker's
textValuematches that text (we didn't clear it either) debouncedSearch(textValue)runs and shows matching results
This is consistent, expected behavior — the user's search is preserved across the menu interaction, which aligns with the emptyInputOnBlur={false} setting.
The concern would be valid if the visual input got cleared but textValue didn't (out-of-sync state), but that can't happen because both are preserved by design.
adrianschmidt
force-pushed
the
fix/picker-chip-menu-interaction
branch
from
359fc68 to
11d0453
Compare
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3849/ |
When a chip inside the picker has a menu (actions), clicking the menu button would cause the picker to lose focus and clear its input field. This happened because focus moved to the menu's portal, which is outside the picker's DOM tree. The fix checks if any chip menu is currently open before deciding to clear the input field on blur. If a menu is open, we assume focus moved to the menu and don't clear. fix: Lundalogik#3676
adrianschmidt-bot
force-pushed
the
fix/picker-chip-menu-interaction
branch
from
11d0453 to
c42e450
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/picker/picker.tsx (1)
279-304: 🧹 Nitpick | 🔵 TrivialRender method returns an array literal — use
<Host>instead.The
render()method wraps its return in[…]. Per project coding guidelines for.tsxfiles, multiple top-level JSX elements should be wrapped in Stencil's<Host>element rather than an array literal. This is pre-existing but worth addressing while you're modifying this file.♻️ Suggested refactor
- return [ - <limel-chip-set + return ( + <Host> + <limel-chip-set type="input" ... - />, + /> {this.renderDropdown()} - ]; + </Host> + );Note: You'll need to import
Hostfrom@stencil/core(it's already available in the import block — just add it to the destructured list on line 11).As per coding guidelines: "When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element."
There was a problem hiding this comment.
In its current state, this PR does not solve the problem. Here is a short screen-recording showing the problem still exists.
limel-picker.with.chips.with.menu.mov
There was a problem hiding this comment.
Good catch! The issue was that I was using an attribute selector (limel-menu[open]) to detect an open menu, but Stencil batches attribute reflection with the render cycle. By the time hasOpenChipMenu() runs, the menu's open property is already true, but the [open] attribute hasn't been reflected to the DOM yet.
Fixed in ⚡ eb4f20e by checking the property directly instead:
const menu = chip.shadowRoot?.querySelector('limel-menu') as HTMLLimelMenuElement;
if (menu?.open) { ... }2 participants
Summary
Fixes an issue where clicking on a chip's action menu (3-dot menu) inside the picker would cause the picker to lose its state and become unresponsive.
Problem
When a chip inside the picker has actions (rendered as a menu), clicking the menu button would:
This left the picker in a weird state where clicking outside wouldn't close the dropdown properly.
Solution
Added a check in
handleStopEditAndBlurto detect if any chip's menu is currently open. If a menu is open, we assume focus moved to that menu and skip clearing the input field.The check queries all chips inside the chip-set and looks for any
limel-menu[open]element.Testing
Fixes #3676
Summary by CodeRabbit