-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(web-components): preserve radio state after deferred upgrade #36243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| import { attr, FASTElement, Observable, observable } from '@microsoft/fast-element'; | ||
| import { attr, FASTElement, Observable, observable, Updates } from '@microsoft/fast-element'; | ||
| import type { Radio } from '../radio/radio.js'; | ||
| import { isRadio } from '../radio/radio.options.js'; | ||
| import { RadioGroupOrientation } from './radio-group.options.js'; | ||
|
|
||
| /** | ||
|
|
@@ -218,14 +217,37 @@ export class BaseRadioGroup extends FASTElement { | |
| @observable | ||
| slottedRadios!: Radio[]; | ||
|
|
||
| private static isUpgradedRadio(element: Element): element is Radio { | ||
| return element.localName.endsWith('-radio') && '$fastController' in element; | ||
| } | ||
|
|
||
| private getRadioDescendants(): Radio[] { | ||
| return [...this.querySelectorAll('*')].filter(BaseRadioGroup.isUpgradedRadio); | ||
| } | ||
|
|
||
| /** | ||
| * Updates the radios collection when the slotted radios change. | ||
| * | ||
| * @param prev - the previous slotted radios | ||
| * @param next - the current slotted radios | ||
| */ | ||
| slottedRadiosChanged(prev: Radio[] | undefined, next: Radio[]): void { | ||
| this.radios = [...this.querySelectorAll('*')].filter(x => isRadio(x)) as Radio[]; | ||
| Updates.enqueue(() => { | ||
| const elements = [...this.querySelectorAll('*')]; | ||
| this.radios = this.getRadioDescendants(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why get |
||
|
|
||
| for (const tagName of new Set( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a weird pattern since |
||
| 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(); | ||
| } | ||
| }); | ||
| } | ||
|
Comment on lines
+240
to
+249
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's potential for a lot of repetitive and unnecessary DOM querying going on here |
||
| }); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ import { tagName as RadioTagName } from '../radio/radio.options.js'; | |
| import type { RadioGroup } from './radio-group.js'; | ||
| import { tagName } from './radio-group.options.js'; | ||
|
|
||
| const sourceBaseUrl = `/@fs${new URL('../', import.meta.url).pathname}`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| test.describe('RadioGroup', () => { | ||
| test.use({ | ||
| tagName, | ||
|
|
@@ -250,6 +252,83 @@ test.describe('RadioGroup', () => { | |
| await expect(radios.nth(2)).toHaveJSProperty('checked', false); | ||
| }); | ||
|
|
||
| test('should preserve checked state when radios upgrade after the group', async ({ page }) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test needs to be written using |
||
| await page.goto('/'); | ||
|
|
||
| const result = await page.evaluate(async sourceBaseUrl => { | ||
| const id = Date.now().toString(36); | ||
| const groupTagName = `upgrade-${id}-radio-group`; | ||
| const radioTagName = `upgrade-${id}-radio`; | ||
|
|
||
| const importModule = (path: string): Promise<Record<string, unknown>> => import(path); | ||
| const [ | ||
| radioGroupModule, | ||
| radioGroupTemplateModule, | ||
| radioGroupStylesModule, | ||
| radioModule, | ||
| radioTemplateModule, | ||
| radioStylesModule, | ||
| ] = await Promise.all([ | ||
| importModule(`${sourceBaseUrl}radio-group/radio-group.ts`), | ||
| importModule(`${sourceBaseUrl}radio-group/radio-group.template.ts`), | ||
| importModule(`${sourceBaseUrl}radio-group/radio-group.styles.ts`), | ||
| importModule(`${sourceBaseUrl}radio/radio.ts`), | ||
| importModule(`${sourceBaseUrl}radio/radio.template.ts`), | ||
| importModule(`${sourceBaseUrl}radio/radio.styles.ts`), | ||
| ]); | ||
|
|
||
| const RadioGroup = radioGroupModule.RadioGroup as { | ||
| compose(options: Record<string, unknown>): { define(registry: CustomElementRegistry): void }; | ||
| }; | ||
| const Radio = radioModule.Radio as { | ||
| compose(options: Record<string, unknown>): { define(registry: CustomElementRegistry): void }; | ||
| }; | ||
|
|
||
| document.body.innerHTML = /* html */ ` | ||
| <${groupTagName} value="bar"> | ||
| <${radioTagName} value="foo"></${radioTagName}> | ||
| <${radioTagName} value="bar"></${radioTagName}> | ||
| <${radioTagName} value="baz"></${radioTagName}> | ||
| </${groupTagName}> | ||
| `; | ||
|
|
||
| RadioGroup.compose({ | ||
| name: groupTagName, | ||
| template: radioGroupTemplateModule.template, | ||
| styles: radioGroupStylesModule.styles, | ||
| }).define(customElements); | ||
|
|
||
| const group = document.querySelector(groupTagName); | ||
| if (!group) { | ||
| throw new Error('Expected radio group to exist.'); | ||
| } | ||
|
|
||
| customElements.upgrade(group); | ||
|
|
||
| Radio.compose({ | ||
| name: radioTagName, | ||
| template: radioTemplateModule.template, | ||
| styles: radioStylesModule.styles, | ||
| }).define(customElements); | ||
|
|
||
| await customElements.whenDefined(groupTagName); | ||
| await customElements.whenDefined(radioTagName); | ||
| await new Promise(requestAnimationFrame); | ||
|
|
||
| const checkedRadio = document.querySelector(`${radioTagName}[value="bar"]`) as HTMLElement & { | ||
| checked: boolean; | ||
| }; | ||
|
|
||
| return { | ||
| checked: checkedRadio.checked, | ||
| hasOwnChecked: Object.prototype.hasOwnProperty.call(checkedRadio, 'checked'), | ||
| }; | ||
| }, sourceBaseUrl); | ||
|
|
||
| expect(result.checked).toBe(true); | ||
| expect(result.hasOwnChecked).toBe(false); | ||
| }); | ||
|
|
||
| test('radio should remain checked after it is set to disabled and uncheck when a new radio is checked', async ({ | ||
| fastPage, | ||
| page, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
radio.options.tsalready provides anisRadiopredicate function that handles the tagname checking, and the$fastControllercheck can be done in-place.