fix: update combobox to handle duplicate items#5950
fix: update combobox to handle duplicate items#5950preethialuru wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 6a27163 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
|
The issue described is an accessibility pattern violation. each item should have a unique value AND label for the drop down. There should never be an instance where two items render the same label with different underlying values. |
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
I have concerns about the accessibility and usability of items with identical text. Can you provide an example where this is necessary and is accessible.
| export const duplicateItemText = (): TemplateResult => { | ||
| const optionsWithDuplicates: ComboboxOption[] = [ | ||
| { value: 'val1', itemText: 'Same Display Text' }, | ||
| { value: 'val2', itemText: 'Same Display Text' }, | ||
| { value: 'val3', itemText: 'Unique Text' }, | ||
| ]; |
There was a problem hiding this comment.
This example would not be accessible as there is no way to differentiate options to the user. Is there an accessible use case you can provide for this PR instead?
There was a problem hiding this comment.
I've updated the example to be more accessible.
| protected override handleChange(): void { | ||
| this.dispatchEvent( | ||
| new CustomEvent('change', { | ||
| bubbles: true, | ||
| composed: true, | ||
| detail: { | ||
| value: this.itemValue, | ||
| itemText: this.value, | ||
| }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The combobox documentation should be updated to reflect this.
There was a problem hiding this comment.
Overriding handleChange will suppress base class behaviour too. Do we want this?
This is risky. The base class is doing important housekeeping (state updates, validation, other events). My approach would be to use super.handleChange() unless you intentionally want to replace that logic.
There was a problem hiding this comment.
Texfield is already dispatching change event at @change handler. This is duplicate semantics.
Check here:
There was a problem hiding this comment.
I want to dispatch change event with detail like text and value, which is not added in Textfield. These details are needed for the user to handle UI on their side.
Suppose, we have a requirement to store the itemText selected in the text box and pass it to API on a CTA click.
There was a problem hiding this comment.
The combobox documentation should be updated to reflect this.
Can you please let me know where do we need to update the docs?
There was a problem hiding this comment.
The combobox documentation should be updated to reflect this.
Can you please let me know where do we need to update the docs?
@preethialuru 1st-gen/packages/combobox/README.md
Add a section just above the accessibility section, something like this:
### Behaviors
#### Change Event
When the selection changes, a `change` event is fired. The event detail contains `value` and `itemText` of the selected option.
```html demo
<sp-combobox id="employee" label="Employee">
<sp-menu-item value="emp-1042">Alex Johnson (Engineering)</sp-menu-item>
<sp-menu-item value="emp-2187">Alex Johnson (Marketing)</sp-menu-item>
<sp-menu-item value="emp-3301">Sam Chen (Design)</sp-menu-item>
<sp-menu-item value="emp-4455">Jordan Lee (Sales)</sp-menu-item>
</sp-combobox>
<script>
document.getElementById('employee').addEventListener('change', (event) => {
alert(`${event.detail.value}: ${event.detail.itemText}`);
});
</script>
```
There was a problem hiding this comment.
@preethialuru Your use case is valid but I see ther eis big problem changing the event contract. It effects all change events not just menu selection. If you need itemValue we can expose it as a public reactive property rather than changing the event contract. You can add a public @Property for the selected option's value:
@property({ type: String, attribute: 'selected-value' })
public selectedValue = '';Then you can capture it on your app side by
combobox.addEventListener('change', () => {
const itemText = combobox.value; // display text
const value = combobox.selectedValue; // underlying ID
api.save({ id: value, name: itemText });
});This would be non-breaking and will work for all scenarios.
|
We also need to make sure this PR includes a changeset. |
| protected override handleChange(): void { | ||
| this.dispatchEvent( | ||
| new CustomEvent('change', { | ||
| bubbles: true, | ||
| composed: true, | ||
| detail: { | ||
| value: this.itemValue, | ||
| itemText: this.value, | ||
| }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Overriding handleChange will suppress base class behaviour too. Do we want this?
This is risky. The base class is doing important housekeeping (state updates, validation, other events). My approach would be to use super.handleChange() unless you intentionally want to replace that logic.
| this.updateComplete.then(() => { | ||
| this._menuSelectedValue = ''; | ||
| }); |
There was a problem hiding this comment.
You are setting _menuSelectedValue when selecting a menu item and clear it both in shouldUpdate (synchronously) and via this.updateComplete.then(() => { this._menuSelectedValue = ''; }).
This is redundant and can mask timing issues. Remove updateComplete.then(...) cleanup and keep clearing in shouldUpdate right after you use it. Much clearer and ties it to the render/update cycle
There was a problem hiding this comment.
Removed updateComplete.then(...)
| protected override handleChange(): void { | ||
| this.dispatchEvent( | ||
| new CustomEvent('change', { | ||
| bubbles: true, | ||
| composed: true, | ||
| detail: { | ||
| value: this.itemValue, | ||
| itemText: this.value, | ||
| }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Texfield is already dispatching change event at @change handler. This is duplicate semantics.
Check here:
| this._menuSelectedValue = ''; | ||
| } else { | ||
| this.itemValue = | ||
| this.availableOptions.find( |
There was a problem hiding this comment.
If availableOptions is stale or lazily updated, you might pick a wrong match. Please ensure filterAvailableOptions() is deterministic and up-to-date before shouldUpdate runs.
There was a problem hiding this comment.
I've updated PR to use all the options instead of availableOptions.
Hi @nikkimk, we've a requirement where itemText can be duplicate. We cannot rely on just the itemText to handle change event. Please refer to this requirement thread in SWC channel https://cclight.slack.com/archives/CESK60MQD/p1764083843191409 |
pfulton
added
the
Status:Ready for re-review
Rajdeepc
left a comment
Rajdeepc
left a comment
There was a problem hiding this comment.
Good thinking here but I want to be careful in how to expose if safely. Kindly go through my suggestions and let me know what you think.
Once these are addressed we can revisit the changeset and JSDoc which also needs to be updated.
| this.itemValue = selected?.value || ''; | ||
| this._menuSelectedValue = selected?.value || ''; | ||
| this.value = selected?.itemText || ''; | ||
| this.handleChange(); |
There was a problem hiding this comment.
The sp-menu fires its own change event with bubbles: true, composed: true. That event propagates up through the shadow DOM and reaches consumers listening on <sp-combobox>. event.preventDefault() does not stop propagation -- it only prevents the browser's default action.
Then this.handleChange() dispatches a second CustomEvent('change', ...). Consumers will receive two change events per selection: the menu's original plain Event (no detail) and the new CustomEvent (with detail).
Please call event.stopPropagation() to prevent the menu's change from leaking out.
| protected override handleChange(): void { | ||
| this.dispatchEvent( | ||
| new CustomEvent('change', { | ||
| bubbles: true, | ||
| composed: true, | ||
| detail: { | ||
| value: this.itemValue, | ||
| itemText: this.value, | ||
| }, | ||
| }) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
@preethialuru Your use case is valid but I see ther eis big problem changing the event contract. It effects all change events not just menu selection. If you need itemValue we can expose it as a public reactive property rather than changing the event contract. You can add a public @Property for the selected option's value:
@property({ type: String, attribute: 'selected-value' })
public selectedValue = '';Then you can capture it on your app side by
combobox.addEventListener('change', () => {
const itemText = combobox.value; // display text
const value = combobox.selectedValue; // underlying ID
api.save({ id: value, name: itemText });
});This would be non-breaking and will work for all scenarios.
| * This ensures we preserve the exact selected value even when multiple | ||
| * options have the same itemText. | ||
| */ | ||
| private _menuSelectedValue: string = ''; |
There was a problem hiding this comment.
You dont' need this. You can replace this with a boolean. You are already setting this.itemValue in line 313.. Instead add a flag here
private _valueSetByMenu = false;| (item) => item.value === target?.value | ||
| ); | ||
| this.itemValue = selected?.value || ''; | ||
| this._menuSelectedValue = selected?.value || ''; |
There was a problem hiding this comment.
No need of creating a shadow copy here. Raise the guard so shouldUpdate doesn't overwrite it via itemText lookup.
this.itemValue = selected?.value || '';
this._valueSetByMenu = true;| if (this._menuSelectedValue) { | ||
| this.itemValue = this._menuSelectedValue; | ||
| this._menuSelectedValue = ''; | ||
| } else { | ||
| const allOptions = this.options || this.optionEls; | ||
| this.itemValue = | ||
| allOptions.find((option) => option.itemText === this.value) | ||
| ?.value ?? ''; | ||
| } |
There was a problem hiding this comment.
Check the boolean guard instead of reading a shadow copy of the value
if (this._valueSetByMenu) {
// itemValue was already set by handleMenuChange; just lower the guard.
this._valueSetByMenu = false;
} else {
// Value changed from typing or programmatic set -- resolve itemValue
// by looking up the matching option.
const allOptions = this.options || this.optionEls;
this.itemValue =
allOptions.find((option) => option.itemText === this.value)
?.value ?? '';
}
|
Hi @preethialuru , Just to let you know that this PR has been moved to a feature request rather than a bug fix. I have added some feedback on the implementation. Once you go through it let me know we can discuss and after talking to the a11y team this is where we stand. I think we could approve the PR under these three conditions:
|
5 participants
Description
This PR fixes an issue in Combobox component where duplicate itemText values would cause incorrect behavior. When multiple options had the same display text but different underlying values, selecting one would always resolve to the first match.
Motivation and context
When there is a list of items, which has itemText and value. Currently, the combobox only supports the itemText to be sent to change event. So any handling needs to be done using itemText.
But is the list of items has duplicate items with same itemText and different value (uniqueId, in our case its projectId). In this case, we would need both itemText and value to be sent to the change event. So the handling can be done based on both the itemText and value, even if the itemText is same, we can use value as differentiator.
So the request here is to, expose an explicit selectedValue property would eliminate reliance on itemText uniqueness and provide a more reliable contract for downstream consumers.
#5910
Related issue(s)
Screenshots (if appropriate)
Screen.Recording.2025-12-16.at.10.54.40.PM.mov
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review