fix(picker): preserve selection when menu-items are added after value is set #5972
Conversation
🦋 Changeset detectedLatest commit: 82fdcb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 78 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 |
rubencarvalho
added
the
Status: Ready for review
📚 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 |
rubencarvalho
changed the title
| if (this.menuItems.length > 0) { | ||
| this.value = ''; | ||
| this.selectedItem = undefined; | ||
| } |
There was a problem hiding this comment.
Currently with this change we are assuming items exist and items are meaningful, which might not always be true coz menu-item can exist in DOM in incomplete states before upgrade with no value or value not populated yet during async render. We should only clear only when items exist and at least one item has a real value and none of those values match the picker's value. Thoughts?
There was a problem hiding this comment.
Nice! I updated the code + tests for this! 👍
| const hasItemsWithValues = this.menuItems.some( | ||
| (item) => item.value && item.value.trim() !== '' | ||
| ); |
There was a problem hiding this comment.
This is good but it’s still unsafe in a custom-elements lifecycle. This assumes item.value is always present and authoritative. <sp-menu-item> can exist in the DOM before it is upgraded. In that state, item.value is undefined even if the element already has a value attribute. This will therefore return false for items that do have meaningful values at the DOM level. We should check both the property and the attribute, and only care about existence.
const hasItemsWithValues = this.menuItems.some(
(item) =>
(item as any).value != null ||
item.getAttribute?.('value') != null
);
Description
This PR fixes an issue where a Picker component with a value set before menu-items are rendered doesn't display the label when menu-items are added later (e.g., when using Lit's
when()directive for conditional rendering or lazy loading).The fix preserves the Picker's value when
manageSelection()runs before menu-items are available, allowing the value to be matched and the label to display correctly once menu-items are added.Motivation and context
When a Picker has a
valueattribute set but its menu-items are conditionally rendered or loaded asynchronously, the component'smanageSelection()method runs before menu-items exist. Previously, this would clear the value immediately, preventing the label from displaying even after menu-items were added.This is a common pattern in modern web applications where:
The fix ensures the Picker maintains the value until menu-items are available, then correctly matches and displays the label.
Related issue(s)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Verify StackBlitz isolated example now works