feat(core): added LiveSelectionController#6486
Conversation
🦋 Changeset detectedLatest commit: 5ca3b69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
nikkimk
changed the title
nikkimk
changed the title
nikkimk
changed the title
Coverage Report for CI Build 29357924290Coverage remained the same at 96.257%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…trum-web-components into nikkimk/self-owning-selection
📚 Branch Preview Links🔍 Gen1 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 |
nikkimk
added
Status:Ready for review
miwha-adobe
left a comment
miwha-adobe
left a comment
There was a problem hiding this comment.
Nice work! A few questions
| changedProperties.get('allowMultiple') === true && | ||
| !this.allowMultiple | ||
| ) { | ||
| this.selectionController.refresh(); |
There was a problem hiding this comment.
Quick question on intent: is initial-state enforcement meant to be out of scope here? Since the controller is event-driven and only refresh()es on the transitions in this file (same as the reference pattern in the docs), a single-mode accordion authored with two open items renders with both open, and nothing reconciles it until the first user toggle. Same goes for a direct .open = true, since no toggle event fires. I confirmed it on the branch (openCount=2 of 2).
Totally fine if that's intended! Just wondering whether it's worth a refresh() on first render / slotchange, or if a valid initial state is the author's responsibility (either way, might be nice to note in the docs). Test below in case it's useful.
export const InitialSingleSelectEnforcedTest: Story = {
render: () => html`
<swc-accordion accessible-label="Initial single-select">
<swc-accordion-item open><span slot="label">One</span><p>1</p></swc-accordion-item>
<swc-accordion-item open><span slot="label">Two</span><p>2</p></swc-accordion-item>
</swc-accordion>
`,
play: async ({ canvasElement }) => {
const accordion = await getComponent<Accordion>(canvasElement, 'swc-accordion');
await accordion.updateComplete;
const items = Array.from(
canvasElement.querySelectorAll('swc-accordion-item')
) as AccordionItem[];
await Promise.all(items.map((i) => i.updateComplete));
expect(items.filter((i) => i.open).length).toBe(1); // fails today: 2
},
};There was a problem hiding this comment.
Good catch. The controller is event-driven, so it never saw those initial open attributes. Fixed by adding a firstUpdated() override in AccordionBase that calls selectionController.refresh() after the first render, at which point the slot is populated and assignedItems()returns all items. Added the test you suggested; it now passes. I also updated the documentation accordingly.
| if (!this.options.readSelected(source as T)) { | ||
| return; | ||
| } | ||
| for (const item of this.options.getItems()) { |
There was a problem hiding this comment.
You might want to double-check this section for nested accordions. It looks like an inner item's toggle (bubbles/composed) also reaches the outer controller, and since the inner item isn't in the outer's getItems(), it closes the outer's open panels, including the one wrapping the nested accordion. So opening a nested panel collapses its outer panel (outerItem1.open flips true to false on the branch). Doesn't look tested yet. Was nesting in scope?
export const NestedAccordionIsolationTest: Story = {
render: () => html`
<swc-accordion accessible-label="Outer">
<swc-accordion-item open>
<span slot="label">Outer 1</span>
<swc-accordion accessible-label="Inner">
<swc-accordion-item><span slot="label">Inner 1</span><p>inner</p></swc-accordion-item>
</swc-accordion>
</swc-accordion-item>
<swc-accordion-item><span slot="label">Outer 2</span><p>o2</p></swc-accordion-item>
</swc-accordion>
`,
play: async ({ canvasElement, step }) => {
const outer = canvasElement.querySelector('swc-accordion') as Accordion;
await outer.updateComplete;
const outerItem1 = canvasElement.querySelector('swc-accordion-item') as AccordionItem;
const innerItem1 = outerItem1.querySelector('swc-accordion-item') as AccordionItem;
await Promise.all([outerItem1.updateComplete, innerItem1.updateComplete]);
await step('opening a nested item does not collapse the outer item', async () => {
(innerItem1.shadowRoot?.querySelector('#header') as HTMLElement).click();
await new Promise((r) => setTimeout(r, 0));
await outer.updateComplete;
expect(innerItem1.open).toBe(true);
expect(outerItem1.open).toBe(true); // fails today: false
});
},
};There was a problem hiding this comment.
Thanks for catching this. Added an items.includes(source) check. The source element is retrieved via composedPath()[0]; if it isn't one of the outer accordion's own items, the controller exits early. Inner accordion items bubble their swc-accordion-item-toggle event all the way to the outer controller, but are not in the outer group's getItems() list, so the outer items are unaffected. Added your test to verifies this.
Rajdeepc
left a comment
Rajdeepc
left a comment
There was a problem hiding this comment.
Nice work! I like the shape of these controllers. Two small things below neither blocking.
| if (this.currentMode !== 'single') { | ||
| return; | ||
| } | ||
| const source = event.target; |
There was a problem hiding this comment.
This works fine for Accordion since items are direct light-DOM children of the host, but event.target can get retargeted at shadow boundaries. If we ever point this at menu/picker where items might sit behind an extra shadow root the could resolve a wrong element.
event.composedPath()[0] would be preferable similar what you have done in SelectionController
There was a problem hiding this comment.
Resolved as you suggested.
| } | ||
|
|
||
| private readonly handleEvent = (event: Event): void => { | ||
| if (this.currentMode !== 'single') { |
There was a problem hiding this comment.
Though likely rare but if you can validate that would be great.
You are checkingcurrentMode once synchronously here, but you are not re-checking inside the deferred microtask. If mode (as a getter) flips from single to multiple in the same tick between the event firing and the microtask running, the deferred close still executes under single-mode assumptions.
There was a problem hiding this comment.
Fixed. The synchronous check before queueMicrotask stays as a fast exit (avoids queuing a microtask at all in multiple mode). Added a second currentMode check at the top of the callback to handle the race condition where allowMultiple might be set between the event and the microtask execution.
nikkimk
added
the
Status:Ready for re-review
…trum-web-components into nikkimk/self-owning-selection
4 participants
Description
Motivation and context
This controller could handle selection for accordions, menus, and pickers.
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Live selection controller
Accordion
Is the creation and implementation of this controller useful for tabs, accordions, radio groups, swatch groups with selectable swatches, button groups that function as radio groups, menus, and pickers? Or is it easier just to let each custom element handle its own selection management?
Device review