Fix MultiSelect not working in label elements#1922
Fix MultiSelect not working in label elements#1922shinokada merged 2 commits intothemesberg:mainfrom
Conversation
|
@aarondoet is attempting to deploy a commit to the Bergside Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe MultiSelect component in Svelte was enhanced with a new public Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/lib/forms/select/MultiSelect.svelte (1)
122-132:elsebranch intoggleDropdownis dead code (pre-existing)Since
toggleDropdownis bound asonclickonmultiSelectContainer, every click event that reaches it has necessarily bubbled from within (or on) the container, somultiSelectContainer.contains(event.target as Node)is alwaystrue. Theelse { show = false }branch is unreachable, and its comment "Close if clicked outside" is misleading — theonMountglobal listener (lines 194–203) already covers that case.🧹 Optional cleanup
const toggleDropdown = (event: MouseEvent) => { if (disabled) return; - // Prevent immediate closing if the click originated from within the component itself - // This is useful if the click triggers a re-render and focus is lost momentarily. - if (multiSelectContainer && multiSelectContainer.contains(event.target as Node)) { - show = !show; - event.preventDefault(); - } else { - show = false; // Close if clicked outside - } + show = !show; + event.preventDefault(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/forms/select/MultiSelect.svelte` around lines 122 - 132, The else branch in the toggleDropdown function is unreachable because toggleDropdown is only bound to clicks on multiSelectContainer; remove the dead else { show = false } branch and its misleading comment, leaving toggleDropdown to only toggle show (and call event.preventDefault()) when multiSelectContainer.contains(event.target as Node) is true; rely on the existing onMount global click listener to handle outside clicks. Update references inside the toggleDropdown function (multiSelectContainer, show, event) accordingly and remove the misleading comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/forms/select/MultiSelect.svelte`:
- Line 27: The id prop was pulled out and only applied to the hidden <select>,
breaking root targeting and label associations; restore the user-provided id on
the root <div> (the component root in MultiSelect.svelte) and apply a derived
select id for the hidden native select (e.g., const selectId = id ?
`${id}-select` : generatedInternalId) so the hidden <select> uses selectId while
the root <div> retains the original id; update any references to use selectId
for <label for> and ensure the hidden select id is unique when id is absent.
- Around line 219-223: Remove the `hidden` attribute from the <select> in
MultiSelect.svelte and replace it with an accessible, interactive hiding
technique so label activation works across browsers; locate the <select {id}
{name} {form} {required} {autocomplete} {value} multiple {onchange}> element in
the MultiSelect component and remove `hidden`, then apply a CSS class (or inline
styles) that hides visually but keeps the control in the accessibility and
layout tree (e.g., visibility: hidden; position: absolute; width: 0; height: 0;
or opacity: 0; position: absolute;). Ensure the new style preserves
interactivity so <label for={id}> triggers the select reliably across
Chrome/Firefox/Safari.
---
Nitpick comments:
In `@src/lib/forms/select/MultiSelect.svelte`:
- Around line 122-132: The else branch in the toggleDropdown function is
unreachable because toggleDropdown is only bound to clicks on
multiSelectContainer; remove the dead else { show = false } branch and its
misleading comment, leaving toggleDropdown to only toggle show (and call
event.preventDefault()) when multiSelectContainer.contains(event.target as Node)
is true; rely on the existing onMount global click listener to handle outside
clicks. Update references inside the toggleDropdown function
(multiSelectContainer, show, event) accordingly and remove the misleading
comment.
| class: className, | ||
| classes, | ||
| // Extract select-specific props | ||
| id, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find MultiSelectProps type definition
fd -e ts -e svelte . -path "*/forms/select/*" | head -20Repository: themesberg/flowbite-svelte
Length of output: 238
🏁 Script executed:
# Search for MultiSelectProps definition
rg -n "interface MultiSelectProps|type MultiSelectProps" --type ts --type svelte -A 30Repository: themesberg/flowbite-svelte
Length of output: 100
🏁 Script executed:
# Check the current MultiSelect.svelte file structure around line 27 and 211
fd -e svelte . -path "*MultiSelect.svelte" -exec cat -n {} \; | head -250Repository: themesberg/flowbite-svelte
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Find MultiSelectProps type definition - search in ts files only
rg "interface MultiSelectProps|type MultiSelectProps" -A 30Repository: themesberg/flowbite-svelte
Length of output: 1703
🏁 Script executed:
#!/bin/bash
# Find and read the MultiSelect.svelte file
find . -name "MultiSelect.svelte" -type f | head -1Repository: themesberg/flowbite-svelte
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Read the MultiSelect.svelte file to check line 27 and surrounding context
cat -n src/lib/forms/select/MultiSelect.svelte | head -150Repository: themesberg/flowbite-svelte
Length of output: 5477
🏁 Script executed:
#!/bin/bash
# Verify the full context around line 211 and 219 where id is used
sed -n '200,230p' src/lib/forms/select/MultiSelect.svelte | cat -nRepository: themesberg/flowbite-svelte
Length of output: 1291
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or documentation about id usage
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "MultiSelect" 2>/dev/null | head -5
rg "MultiSelect.*id" --type=svelte -B 2 -A 2Repository: themesberg/flowbite-svelte
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Read more context around lines 122-132 to verify the toggleDropdown function
sed -n '120,135p' src/lib/forms/select/MultiSelect.svelte | cat -nRepository: themesberg/flowbite-svelte
Length of output: 788
🏁 Script executed:
#!/bin/bash
# Check git history or any CHANGELOG to see if this was a documented breaking change
find . -name "CHANGELOG*" -o -name "RELEASE*" -o -name "HISTORY*" | head -3
rg "id.*breaking|breaking.*id" -iRepository: themesberg/flowbite-svelte
Length of output: 572
🏁 Script executed:
#!/bin/bash
# Check if there are example usages or tests that show how id was previously used
rg "MultiSelect" -A 10 -B 2 | grep -A 10 -B 2 "id=" | head -40Repository: themesberg/flowbite-svelte
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG to see if there are any mentions of this PR or MultiSelect changes
cat CHANGELOG.md | head -100Repository: themesberg/flowbite-svelte
Length of output: 2488
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about how id should be used with MultiSelect
find . -name "*.md" | xargs grep -l "MultiSelect" | head -3Repository: themesberg/flowbite-svelte
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check the forms/select documentation for MultiSelect usage examples
cat src/routes/docs/forms/select.md | head -200Repository: themesberg/flowbite-svelte
Length of output: 4231
🏁 Script executed:
#!/bin/bash
# Check for any examples with MultiSelect and id attribute
find src/routes -name "*.svelte" | xargs grep -l "MultiSelect" | head -5Repository: themesberg/flowbite-svelte
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Look for actual example files
find src/routes/docs -name "Multi*.svelte" | head -10Repository: themesberg/flowbite-svelte
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check MultiSelect examples for id usage
cat src/routes/docs-examples/forms/select/Multi.svelteRepository: themesberg/flowbite-svelte
Length of output: 467
id prop moved to hidden select breaks root element targeting — breaking change
The id prop was previously implicitly part of MultiSelectProps via HTMLAttributes<HTMLDivElement> and flowed through restProps to the root <div>. After extracting id explicitly (line 27) and applying it only to the hidden <select> (line 219), the root element no longer receives the user-provided id.
This breaks:
- CSS selectors:
#my-select { ... }targets the hidden select, not the component root - DOM APIs:
document.getElementById('my-select')returns adisplay:noneelement - Accessibility:
aria-labelledbyoraria-describedbyreferencing the component id will fail
Additionally, the hidden <select> has the hidden attribute, which removes it from the accessibility tree. This breaks <label for> associations since labels require a focusable, semantically present form control.
Restore the id to the root <div> and use a derived id (e.g., ${id}-select) for the hidden select, or use $props.id() to generate a separate internal id for the select if no external id is provided.
Proposed fix
let {
children,
items = [],
value = $bindable(),
size = "md",
dropdownClass = "",
placeholder = "",
disabled = false,
onchange,
onblur,
class: className,
classes,
// Extract select-specific props
id,
name,
form,
required,
autocomplete,
...restProps
}: MultiSelectProps<T> = $props();
+ let internalSelectId = $derived(id ? `${id}-select` : undefined);- <select {id} {name} {form} {required} {autocomplete} {value} hidden multiple {onchange}>
+ <select id={internalSelectId} {name} {form} {required} {autocomplete} {value} hidden multiple {onchange}> <div
bind:this={multiSelectContainer}
- {...restProps}
+ id={id}
+ {...restProps}
onclick={toggleDropdown}
onblur={handleBlur}
onkeydown={handleKeyDown}
tabindex="0"
role="listbox"
class={base({ size, class: clsx(theme?.base, className) })}
>Document that for <label for> associations, consumers should target the derived selectId prop or use for="${id}-select".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/forms/select/MultiSelect.svelte` at line 27, The id prop was pulled
out and only applied to the hidden <select>, breaking root targeting and label
associations; restore the user-provided id on the root <div> (the component root
in MultiSelect.svelte) and apply a derived select id for the hidden native
select (e.g., const selectId = id ? `${id}-select` : generatedInternalId) so the
hidden <select> uses selectId while the root <div> retains the original id;
update any references to use selectId for <label for> and ensure the hidden
select id is unique when id is absent.
There was a problem hiding this comment.
I don't think the id being on the select instead of the div breaks anything. Library users should never style the element via the id, rather via classes or snippets. The library itself does not use the id. The only reason for using an id that I see is exactly what I fixed here, labeling the element, and for that the id needs to be on the select.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
HTML label for attribute with hidden form control accessibility
💡 Result:
- If the control is truly hidden (e.g.,
<input type="hidden">or hidden viadisplay:none/hidden), don’t use<label for="…">for accessibility—it won’t be a “labelable” control, isn’t focusable, and is effectively not exposed to assistive tech. Hidden inputs specifically are not labelable andinput.labelsbecomesnullwhentype="hidden". [1], [2], [3], [4] - If you mean “visually hidden but still accessible” (screen-reader-only), keep it labelable (e.g.,
type="text",type="checkbox", etc.), hide it with “visually-hidden” CSS (notdisplay:none), and a normal<label for="id">…</label>works. (The key is: it must remain rendered/available to the accessibility tree.) [4] - If you have no visible label you can associate, give the control an accessible name with
aria-labelledby(preferred when label text exists in the DOM) oraria-label. [5]
@aarondoet, I understand your reasoning about styling practices, and you're right that the library doesn't internally reference the id. However, there's an accessibility concern with placing the id on the hidden select:
According to accessibility specifications, hidden form controls (using the hidden attribute or display:none) are not labelable elements and are removed from the accessibility tree. This means:
- Screen readers won't announce the label association - while a
<label for="my-id">pointing to the hidden select may trigger click behavior visually, assistive technology won't expose this relationship to users - The hidden select is not focusable or accessible - it's effectively invisible to AT users
- The root
<div>withrole="listbox"is the actual accessible control - this is what screen readers interact with
For proper accessibility, the id should remain on the root <div> so that:
aria-labelledbyoraria-describedbycan reference it- Screen readers can properly announce the component when focused
- The accessible control (the listbox) has the identity, not the hidden implementation detail
The visual label-click behavior you're fixing works due to the preventDefault() logic you added, which is great! But the accessibility association requires the id on the accessible element (the root div), not the hidden select.
Would you be open to applying the id to the root div and using a derived id (like ${id}-select) for the hidden select if needed for form submission purposes?
🧠 Learnings used
Learnt from: jjagielka
Repo: themesberg/flowbite-svelte PR: 1822
File: src/lib/command-palette/CommandPalette.svelte:30-31
Timestamp: 2025-11-04T14:50:28.736Z
Learning: In Svelte 5.20.0+, `$props.id()` is a valid rune that generates a unique ID for the current component instance. The syntax `let gid = $props.id();` is correct and commonly used for creating unique IDs for accessibility attributes like `aria-controls`, `aria-labelledby`, etc.
Learnt from: LahTeuto
Repo: themesberg/flowbite-svelte PR: 1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
| <select {id} {name} {form} {required} {autocomplete} {value} hidden multiple {onchange}> | ||
| {#each items as item (item.value)} | ||
| <option value={item.value} disabled={item.disabled}>{item.name}</option> | ||
| {/each} | ||
| </select> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does clicking label for dispatch click to hidden form element Chrome Firefox Safari HTML behavior
💡 Result:
- Yes—usually. In HTML, a
<label>can be associated with a labelable form control (viafor/idor by nesting). When the label is activated, browsers typically perform label activation by firing a separateclickat the associated control (it’s not “bubbling” the same click from the label to the input). The exact platform behavior is described as matching the platform’s label behavior, but “fire a click event at the input” is explicitly called out as the typical case. [1] - But not for truly “hidden” controls:
<input type="hidden">is not labelable, so a label can’t activate it. (In the spec,labelsisnullfor inputs whosetypeis in the Hidden state.) [1]- Elements hidden with the
hiddenattribute also shouldn’t be referenced bylabel for=...(authoring requirement). [2]
- If it’s only “hidden” via CSS (
display:none, etc.) the control is still the labeled control (because it’s still labelable), but browser behavior can differ for some control types, especially file inputs: Safari has long had cases where a label won’t open a file picker if the associated<input type="file">isdisplay:none, while other browsers may allow it. [3] - Practical implication for events: if you’re testing “does it dispatch click to the hidden form element,” attach a
click(or better, for checkboxes/radios,change) handler to the control itself—don’t expect the label’s click to haveevent.target === input. [1][4]
Sources: HTML Standard (label element) [1], HTML Standard (hidden attribute + label for restriction) [2], Safari + hidden file input via label (Stack Overflow) [3], MDN <label> reference [4].
Remove hidden attribute from <select> — violates HTML spec and breaks cross-browser label activation
The HTML specification explicitly discourages using <label for> with elements that have the hidden attribute. Additionally, web testing confirms that when a hidden form control (via display:none) is targeted by <label for>, the browser's synthetic click dispatch is unreliable across Chrome, Firefox, and Safari—Safari in particular is known to silently fail to activate hidden form controls when clicked via their associated label.
Replace the hidden attribute with one of these accessible alternatives:
visibility: hidden; position: absolute; width: 0; height: 0;— keeps element in layout tree, can receive focus/clicks programmatically, hidden from viewopacity: 0; position: absolute;— similar effect, element remains interactive
This ensures the <label for> interaction works reliably across all browsers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/forms/select/MultiSelect.svelte` around lines 219 - 223, Remove the
`hidden` attribute from the <select> in MultiSelect.svelte and replace it with
an accessible, interactive hiding technique so label activation works across
browsers; locate the <select {id} {name} {form} {required} {autocomplete}
{value} multiple {onchange}> element in the MultiSelect component and remove
`hidden`, then apply a CSS class (or inline styles) that hides visually but
keeps the control in the accessibility and layout tree (e.g., visibility:
hidden; position: absolute; width: 0; height: 0; or opacity: 0; position:
absolute;). Ensure the new style preserves interactivity so <label for={id}>
triggers the select reliably across Chrome/Firefox/Safari.
There was a problem hiding this comment.
I have tested the behavior in Firefox where the select was clicked via the label despite being hidden.
There was a problem hiding this comment.
@aarondoet, thank you for testing! Firefox is good news. My concern was primarily about Safari, which the web research flagged as sometimes failing to activate hidden form controls via label clicks (though that was specifically documented for file inputs, not select elements).
Have you had a chance to test in Safari as well? If it works there too, then your real-world testing trumps the theoretical concern, and the current implementation is fine.
🧠 Learnings used
Learnt from: mrh1997
Repo: themesberg/flowbite-svelte PR: 1442
File: src/lib/utils/Popper.svelte:144-145
Timestamp: 2024-11-22T08:37:27.996Z
Learning: In `src/lib/utils/Popper.svelte`, inconsistencies in event handling (e.g., using both `mouseenter`/`mouseleave` and `pointerenter`/`pointerleave`) may be intentional to minimize modifications and reduce the risk of new problems.
Learnt from: LahTeuto
Repo: themesberg/flowbite-svelte PR: 1484
File: src/lib/navbar/NavLi.svelte:34-34
Timestamp: 2024-11-12T10:36:34.807Z
Learning: When enhancing ARIA roles in Svelte components, avoid setting `role="presentation"` on elements that should retain the `link` role, as it can diminish ARIA role coverage and negatively affect accessibility.
|
If the accessibility concerns raised by code rabbit are true (as said, I'm testing on Firefox, and without screen reader, for me everything works as intended), then everything except for the |
2 participants
📑 Description
Currently the MultiSelect dropdown does not show when it is inside of a label. Or rather, it gets shown and immediately hidden again. Calling
preventDefaulton the onclick event fixes this.🔍 PR Type
🚦 PR Status
✅ Checklist
pnpm lint && pnpm check && pnpm test:e2eand all tests passmainbranch (not the published npm version)I don't think anything breaks. If anything, then that the MultiSelect
idprop is added to theselectand not the wrappingdivanymore.ℹ️ Additional Information
I have tested a few things.
Cases 1 and 3 work with the current version of flowbite-svelte, but case 3 does not open the dropdown when the label is clicked.
With only
event.preventDefault()all cases work, case 2 and 3 behave the same: the dropdown does not get shown when the label itself is clicked.const toggleDropdown = (event: MouseEvent) => { if (disabled) return; // Prevent immediate closing if the click originated from within the component itself // This is useful if the click triggers a re-render and focus is lost momentarily. if (multiSelectContainer && multiSelectContainer.contains(event.target as Node)) { show = !show; + event.preventDefault(); } else { show = false; // Close if clicked outside } };When also moving the hidden select element inside on the MultiSelect div wrapper element, case 2 works and it opens the dropdown when the label is clicked. Case 3 however stays unaffected.
If we additionally add the
restProps.idproperty to the hidden select, so that clicking the label is a click on the select again, then all cases work and in both case 2 and 3 the dropdown shows when clicking the label.<script> [...] let { children, items = [], value = $bindable(), [...] // Extract select-specific props + id, name, form, required, autocomplete, ...restProps }: MultiSelectProps<T> = $props(); [...] </script> [...] <div bind:this={multiSelectContainer} {...restProps} onclick={toggleDropdown} onblur={handleBlur} onkeydown={handleKeyDown} tabindex="0" role="listbox" class={base({ size, class: clsx(theme?.base, className) })} > - <select {name} {form} {required} {autocomplete} {value} hidden multiple {onchange}> + <select {id} {name} {form} {required} {autocomplete} {value} hidden multiple {onchange}> {#each items as item (item.value)} <option value={item.value} disabled={item.disabled}>{item.name}</option> {/each} </select> {#if !selectItems.length} <span class={placeholderSpan({ class: clsx(classes?.placeholder) })}>{placeholder}</span> {/if}Summary by CodeRabbit
Release Notes
New Features
idprop.Bug Fixes