fix(web-components): preserve radio state after deferred upgrade#36243
fix(web-components): preserve radio state after deferred upgrade#36243KirtiRamchandani wants to merge 3 commits into
Conversation
|
Tagging @radium-v as a required reviewer. |
radium-v
left a comment
radium-v
left a comment
There was a problem hiding this comment.
While I think this is pointing out a few important scenarios (tag names can be a fragile identifier; ancestral relationships between components all have additional structural concerns), I'd prefer if we could address the issue all-up. We have similar problems with Accordion/AccordionItem, Dropdown/Listbox/DropdownOption, MenuButton/Menu/MenuList/MenuItem, Tree/TreeItem, and others.
| await expect(radios.nth(2)).toHaveJSProperty('checked', false); | ||
| }); | ||
|
|
||
| test('should preserve checked state when radios upgrade after the group', async ({ page }) => { |
There was a problem hiding this comment.
This test needs to be written using fastPage, but may need to be its own test fixture entrypoint. Doing it this way may allow us to reuse this for other components that have similar ancestor/descendant patterns.
| private static isUpgradedRadio(element: Element): element is Radio { | ||
| return element.localName.endsWith('-radio') && '$fastController' in element; | ||
| } |
There was a problem hiding this comment.
radio.options.ts already provides an isRadio predicate function that handles the tagname checking, and the $fastController check can be done in-place.
| this.radios = [...this.querySelectorAll('*')].filter(x => isRadio(x)) as Radio[]; | ||
| Updates.enqueue(() => { | ||
| const elements = [...this.querySelectorAll('*')]; | ||
| this.radios = this.getRadioDescendants(); |
There was a problem hiding this comment.
Why get elements first and then get radio descendants separately? They both end up querying for all elements.
| const elements = [...this.querySelectorAll('*')]; | ||
| this.radios = this.getRadioDescendants(); | ||
|
|
||
| for (const tagName of new Set( |
There was a problem hiding this comment.
This is a weird pattern since Set provides iterator methods
| elements | ||
| .filter(x => x.localName.endsWith('-radio') && !BaseRadioGroup.isUpgradedRadio(x)) | ||
| .map(x => x.localName), | ||
| )) { | ||
| customElements.whenDefined(tagName).then(() => { | ||
| if (this.isConnected) { | ||
| this.radios = this.getRadioDescendants(); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
There's potential for a lot of repetitive and unnecessary DOM querying going on here
| import type { RadioGroup } from './radio-group.js'; | ||
| import { tagName } from './radio-group.options.js'; | ||
|
|
||
| const sourceBaseUrl = `/@fs${new URL('../', import.meta.url).pathname}`; |
There was a problem hiding this comment.
I'd rather we didn't rely on internal vite-specific constructs, or try to work outside of the bounds of the environment. I see custom pathing like this as a fragile workaround for something that can probably be handled in a better and more purposeful way. For instance, the new test can likely be its own test entrypoint.
|
Thanks for the review. I pushed a follow-up that narrows the implementation and removes the fragile test setup:
Checks run:
|
|
Thanks for the direction here. I pushed a follow-up that moves this from the radio-only path to a shared upgraded-child collection helper and wires it into the parent/child paths called out in the review:
The helper only returns matching child custom elements after FAST has upgraded them, then refreshes the parent collection once matching pending tag definitions resolve. I also added parent-before-child upgrade-order coverage for RadioGroup, Listbox, and Tree so the regression checks both the collection refresh and the absence of own shadowing properties. Validation run:
|
3 participants
Previous Behavior
Parent components could collect matching child tags before those child custom elements had upgraded. When the parent then wrote child state, those writes could create own properties on bare
HTMLElementinstances and shadow the eventual component accessors.The reported RadioGroup case created an own
checkedproperty on pending radios, but the same parent/child collection pattern exists across Accordion/AccordionItem, Listbox/Option, MenuList/MenuItem, and Tree/TreeItem.New Behavior
Parent collection now goes through a shared upgraded-custom-element helper. Matching child tags are ignored until FAST has upgraded them, and the parent refreshes its collection once any matching pending tag definitions resolve.
This is wired into RadioGroup, Accordion, Listbox, MenuList, Tree, and TreeItem. Listbox also reapplies its current
multiplestate when options are collected after upgrade.The regression fixture forces parent tags to upgrade before child tags and verifies:
checkedshadow property.multipleto options after they upgrade without an ownmultipleshadow property.sizeto tree items after they upgrade without an ownsizeshadow property.Tests run
corepack yarn type-checkfrompackages/web-componentscorepack yarn eslint src/accordion/accordion.ts src/listbox/listbox.ts src/listbox/listbox.spec.ts src/menu-list/menu-list.base.ts src/radio-group/radio-group.base.ts src/tree/tree.base.ts src/tree/tree.spec.ts src/tree-item/tree-item.base.ts src/utils/custom-elements.ts test/src/parent-child-upgrade-order.tsfrompackages/web-componentscorepack yarn e2e src/radio-group/radio-group.spec.ts src/listbox/listbox.spec.ts src/tree/tree.spec.ts --project=chromium -g "upgrade order"frompackages/web-componentscorepack yarn e2e src/accordion/accordion.spec.ts src/listbox/listbox.spec.ts src/menu-list/menu-list.spec.ts src/radio-group/radio-group.spec.ts src/tree/tree.spec.ts --project=chromium --workers=1frompackages/web-componentscorepack yarn prettier --check packages/web-components/src/accordion/accordion.ts packages/web-components/src/listbox/listbox.ts packages/web-components/src/listbox/listbox.spec.ts packages/web-components/src/menu-list/menu-list.base.ts packages/web-components/src/radio-group/radio-group.base.ts packages/web-components/src/tree/tree.base.ts packages/web-components/src/tree/tree.spec.ts packages/web-components/src/tree-item/tree-item.base.ts packages/web-components/src/utils/custom-elements.ts packages/web-components/test/parent-child-upgrade-order.html packages/web-components/test/src/parent-child-upgrade-order.tscorepack yarn check:changegit diff --checkRelated Issue(s)
checkedState #36239