fix(web-components): preserve radio state after deferred upgrade#36243
fix(web-components): preserve radio state after deferred upgrade#36243KirtiRamchandani wants to merge 1 commit 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.
3 participants
Previous Behavior
When a
fluent-radio-groupupgraded before its slottedfluent-radiochildren, the group collected tag-name matches before the radio elements had upgraded.radiosChangedthen wrotechecked,name, anddisableddirectly onto bareHTMLElementinstances, creating own properties that shadowed the eventual radio accessors.New Behavior
RadioGroup now defers descendant collection, only writes to radio elements after they have upgraded, and refreshes the collection once matching pending radio tags finish defining. This preserves the radio
checkedaccessor so state, ARIA, and form participation stay in sync.A regression test composes temporary radio/radio-group tags and forces the group to upgrade before the radios, verifying the selected radio is checked without an own
checkedshadow property.Tests run
corepack yarn nx run tokens:buildcorepack yarn e2e src/radio-group/radio-group.spec.ts --project=chromium -g "preserve checked state"corepack yarn e2e src/radio-group/radio-group.spec.ts --project=chromium --workers=1(passed with one harness setup retry)corepack yarn type-checkfrompackages/web-componentscorepack yarn eslint src/radio-group/radio-group.base.ts src/radio-group/radio-group.spec.tsfrompackages/web-componentscorepack yarn prettier --check packages/web-components/src/radio-group/radio-group.base.ts packages/web-components/src/radio-group/radio-group.spec.tscorepack yarn check:changegit diff --checkRelated Issue(s)
checkedState #36239