Skip to content
Open
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
31 changes: 31 additions & 0 deletions src/components/picker/picker.tsx
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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) { ... }

Original file line number Diff line number Diff line change
Expand Up @@ -547,9 +547,40 @@ export class Picker {
return;
}

// Don't clear if a chip's menu is open - focus moved to the menu's portal
if (this.hasOpenChipMenu()) {
return;
}
Comment on lines +550 to +553
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking at this more carefully, I believe the behavior is consistent and correct:

  1. The picker sets emptyInputOnBlur={false} on the chip-set (line 299)
  2. This means when chip-set exits edit mode on blur, it does NOT clear its visual input (syncEmptyInput() is only called when emptyInputOnBlur is true — see chip-set.tsx lines 519-521)
  3. So both the chip-set's visual input AND the picker's textValue retain 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 textValue matches 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.


this.clearInputField();
}

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) {
// Check the `open` property directly rather than using an attribute
// selector, because Stencil batches attribute reflection and
// the [open] attribute may not be on the DOM yet
const menu = chip.shadowRoot?.querySelector(
'limel-menu'
) as HTMLLimelMenuElement;
if (menu?.open) {
return true;
}
}

return false;
}
Comment on lines +558 to +582
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

# Find the limel-menu component
fd 'menu.tsx' --type f | head -20

Repository: Lundalogik/lime-elements

Length of output: 622


🏁 Script executed:

# Find the limel-chip-set component
fd 'chip-set.tsx' --type f | head -20

Repository: 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 -100

Repository: Lundalogik/lime-elements

Length of output: 4114


🏁 Script executed:

# Check limel-chip component 
fd 'chip.tsx' --type f | grep -v examples | grep -v chip-set

Repository: 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 -80

Repository: 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.tsx

Repository: 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.tsx

Repository: Lundalogik/lime-elements

Length of output: 108


🏁 Script executed:

# Check chip component for menu-related functionality
cat -n src/components/chip/chip.tsx | head -150

Repository: Lundalogik/lime-elements

Length of output: 5914


🏁 Script executed:

# Check if chip has any public methods
rg "@Method()" src/components/chip/chip.tsx

Repository: 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 2

Repository: 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.tsx

Repository: 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 2

Repository: 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 3

Repository: 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.shadowRootlimel-chipchip.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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


/**
* Input handler for the input field
*
Expand Down
Loading