From 30b548b167883f0a657b0410d3bcf76dbb6895e0 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Mon, 17 Apr 2023 16:30:19 -0400 Subject: [PATCH] fix(many): dynamic label support for modern form controls (#27156) ## What is the current behavior? Developers that are using Ionic v7 are experiencing an issue where implementations that are intended to use the modern control syntax will render with the legacy syntax and a warning will be displayed. The issue is most easily reproduced by not assigning a label to the control and then asynchronously assigning a label after a duration. Angular example: ```html ``` ```ts @Component({ ... }) export class MyComponent { @Input() label?: string; // initially unset ngOnInit() { setTimeout(() => this.label = 'Hello world', 500); } } ``` Issue URL: resolves #27085 ## What is the new behavior? - Form controls that do not have a decorative label or `aria-label`/`aria-labelledby` assigned, will default render as modern controls. - Legacy form implementations that render an `` within the item, will continue to render with the legacy template and a warning will be displayed in the console. - Modern form syntax supports dynamically set labels ## Does this introduce a breaking change? - [ ] Yes - [x] No Legacy implementations that do not have a decorative label and do not specify `aria-label` on the control, will be upgraded to the modern syntax. For example: ```html ``` Developers that do not want to update to the modern syntax yet should add the `legacy="true"` attribute to their form control. ## Other information Dev-build: `7.0.2-dev.11681157690.1060bc7f` When migrating the range tests to modern syntax, I observed a visual clipping issue. This is being addressed in: https://github.com/ionic-team/ionic-framework/pull/27188. This PR simply adds the legacy flag so that screenshots are the same as `main`. --- .../components/input/test/item/input.e2e.ts | 23 +++++++++++++++++++ .../components/range/test/a11y/range.e2e.ts | 4 ++-- .../components/range/test/label/range.e2e.ts | 4 ++-- .../range/test/legacy/a11y/range.e2e.ts | 4 ++-- .../range/test/legacy/basic/index.html | 2 +- .../range/test/legacy/basic/range.e2e.ts | 4 ++-- core/src/utils/forms/form-controller.ts | 8 +++++-- 7 files changed, 38 insertions(+), 11 deletions(-) diff --git a/core/src/components/input/test/item/input.e2e.ts b/core/src/components/input/test/item/input.e2e.ts index d853f01c0f..6e87a31e89 100644 --- a/core/src/components/input/test/item/input.e2e.ts +++ b/core/src/components/input/test/item/input.e2e.ts @@ -36,4 +36,27 @@ test.describe('input: item', () => { const list = page.locator('ion-list'); expect(await list.screenshot()).toMatchSnapshot(`input-inset-list-no-fill-${page.getSnapshotSettings()}.png`); }); + test('input renders as modern when label is set asynchronously', async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/27085', + }); + + await page.setContent( + ` + + + ` + ); + const input = page.locator('ion-input'); + // Initial template should be modern + await expect(input).not.toHaveClass(/legacy-input/); + + // Update the input label + await input.evaluate((el: HTMLIonInputElement) => (el.label = 'New label')); + await page.waitForChanges(); + + // Template should still be modern + await expect(input).not.toHaveClass(/legacy-input/); + }); }); diff --git a/core/src/components/range/test/a11y/range.e2e.ts b/core/src/components/range/test/a11y/range.e2e.ts index 444f8b590d..4005c7dead 100644 --- a/core/src/components/range/test/a11y/range.e2e.ts +++ b/core/src/components/range/test/a11y/range.e2e.ts @@ -25,7 +25,7 @@ test.describe('range: a11y', () => { await page.setContent( ` - + ` @@ -57,7 +57,7 @@ test.describe('range: a11y', () => { await page.setContent( ` - + ` diff --git a/core/src/components/range/test/label/range.e2e.ts b/core/src/components/range/test/label/range.e2e.ts index 9c128b1c2d..d773cce0ec 100644 --- a/core/src/components/range/test/label/range.e2e.ts +++ b/core/src/components/range/test/label/range.e2e.ts @@ -5,7 +5,7 @@ test.describe('range: label', () => { test.describe('range: no start or end items', () => { test('should render a range with no visible label', async ({ page }) => { await page.setContent(` - + `); const range = page.locator('ion-range'); @@ -50,7 +50,7 @@ test.describe('range: label', () => { test.describe('range: start and end items', () => { test('should render a range with no visible label', async ({ page }) => { await page.setContent(` - + diff --git a/core/src/components/range/test/legacy/a11y/range.e2e.ts b/core/src/components/range/test/legacy/a11y/range.e2e.ts index bc2941bf51..d60877daf1 100644 --- a/core/src/components/range/test/legacy/a11y/range.e2e.ts +++ b/core/src/components/range/test/legacy/a11y/range.e2e.ts @@ -9,7 +9,7 @@ test.describe('range: a11y', () => { await page.setContent( ` - + ` @@ -41,7 +41,7 @@ test.describe('range: a11y', () => { await page.setContent( ` - + ` diff --git a/core/src/components/range/test/legacy/basic/index.html b/core/src/components/range/test/legacy/basic/index.html index 58b6a31350..a9b05c86d3 100644 --- a/core/src/components/range/test/legacy/basic/index.html +++ b/core/src/components/range/test/legacy/basic/index.html @@ -249,7 +249,7 @@ Custom pin label - + diff --git a/core/src/components/range/test/legacy/basic/range.e2e.ts b/core/src/components/range/test/legacy/basic/range.e2e.ts index 4fbebb9410..be8cfcc5ac 100644 --- a/core/src/components/range/test/legacy/basic/range.e2e.ts +++ b/core/src/components/range/test/legacy/basic/range.e2e.ts @@ -15,7 +15,7 @@ test.describe('range: basic', () => { * TODO FW-2873 */ test.fixme('should emit start/end events', async ({ page }, testInfo) => { - await page.setContent(``); + await page.setContent(``); const rangeStart = await page.spyOnEvent('ionKnobMoveStart'); const rangeEnd = await page.spyOnEvent('ionKnobMoveEnd'); @@ -43,7 +43,7 @@ test.describe('range: basic', () => { }); test('should emit start/end events, keyboard', async ({ page }) => { - await page.setContent(``); + await page.setContent(``); const rangeStart = await page.spyOnEvent('ionKnobMoveStart'); const rangeEnd = await page.spyOnEvent('ionKnobMoveEnd'); diff --git a/core/src/utils/forms/form-controller.ts b/core/src/utils/forms/form-controller.ts index a43a2d2491..0a7c628be0 100644 --- a/core/src/utils/forms/form-controller.ts +++ b/core/src/utils/forms/form-controller.ts @@ -1,3 +1,5 @@ +import { findItemLabel } from '@utils/helpers'; + type HTMLLegacyFormControlElement = HTMLElement & { label?: string; legacy?: boolean }; /** @@ -21,17 +23,19 @@ export const createLegacyFormController = (el: HTMLLegacyFormControlElement): Le * can check to see if the component has slotted text * in the light DOM. */ - const hasLabelProp = (controlEl as any).label !== undefined || hasLabelSlot(controlEl); + const hasLabelProp = controlEl.label !== undefined || hasLabelSlot(controlEl); const hasAriaLabelAttribute = controlEl.hasAttribute('aria-label') || // Shadow DOM form controls cannot use aria-labelledby (controlEl.hasAttribute('aria-labelledby') && controlEl.shadowRoot === null); + const legacyItemLabel = findItemLabel(controlEl); /** * Developers can manually opt-out of the modern form markup * by setting `legacy="true"` on components. */ - legacyControl = controlEl.legacy === true || (!hasLabelProp && !hasAriaLabelAttribute); + legacyControl = + controlEl.legacy === true || (!hasLabelProp && !hasAriaLabelAttribute && legacyItemLabel !== null); } return legacyControl; };