From a717c0e41511f5ece6d7c45ffc47e7573fa8f0d0 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Tue, 14 Feb 2023 14:09:22 -0500 Subject: [PATCH 01/10] fix(select): emit single ionChange event for popover option selection --- core/src/components/select-popover/select-popover.tsx | 1 - core/src/components/select/test/basic/select.e2e.ts | 10 +++++++--- core/src/utils/test/playwright/index.ts | 1 + 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/src/components/select-popover/select-popover.tsx b/core/src/components/select-popover/select-popover.tsx index 56da639a704..09f0970b26d 100644 --- a/core/src/components/select-popover/select-popover.tsx +++ b/core/src/components/select-popover/select-popover.tsx @@ -49,7 +49,6 @@ export class SelectPopover implements ComponentInterface { @Listen('ionChange') onSelect(ev: any) { this.setChecked(ev); - this.callOptionHandler(ev); } private findOptionFromEvent(ev: any) { diff --git a/core/src/components/select/test/basic/select.e2e.ts b/core/src/components/select/test/basic/select.e2e.ts index 995b7bea314..c327d30bd96 100644 --- a/core/src/components/select/test/basic/select.e2e.ts +++ b/core/src/components/select/test/basic/select.e2e.ts @@ -1,5 +1,6 @@ import { expect } from '@playwright/test'; import { test } from '@utils/test/playwright'; +import type { E2ELocator } from '@utils/test/playwright'; test.describe('select: basic', () => { test.beforeEach(async ({ page }) => { @@ -130,6 +131,7 @@ test.describe('select: ionChange', () => { await ionChange.next(); expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' }); + expect(ionChange).toHaveReceivedEventTimes(1); }); test('should fire ionChange when confirming a value from a popover', async ({ page }) => { @@ -141,8 +143,8 @@ test.describe('select: ionChange', () => { `); const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent'); - const ionChange = await page.spyOnEvent('ionChange'); - const select = page.locator('ion-select'); + const select = page.locator('ion-select') as E2ELocator; + const ionChange = await select.spyOnEvent('ionChange'); await select.click(); await ionPopoverDidPresent.next(); @@ -153,7 +155,8 @@ test.describe('select: ionChange', () => { await radioButtons.nth(0).click(); await ionChange.next(); - expect(ionChange).toHaveReceivedEventDetail({ value: 'apple', event: { isTrusted: true } }); + expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' }); + expect(ionChange).toHaveReceivedEventTimes(1); }); test('should fire ionChange when confirming a value from an action sheet', async ({ page }) => { @@ -178,6 +181,7 @@ test.describe('select: ionChange', () => { await ionChange.next(); expect(ionChange).toHaveReceivedEventDetail({ value: 'apple' }); + expect(ionChange).toHaveReceivedEventTimes(1); }); test('should not fire when programmatically setting a valid value', async ({ page }) => { diff --git a/core/src/utils/test/playwright/index.ts b/core/src/utils/test/playwright/index.ts index 0b139e0fed1..6aeb5d7f235 100644 --- a/core/src/utils/test/playwright/index.ts +++ b/core/src/utils/test/playwright/index.ts @@ -1,6 +1,7 @@ export * from './playwright-page'; export * from './playwright-declarations'; export * from './page/event-spy'; +export * from './page/utils'; export * from './drag-element'; export * from './matchers'; export * from './viewports'; From 18c3b367452d4fd3ca2713a6f6bdc63af3119e2a Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Tue, 14 Feb 2023 14:26:38 -0500 Subject: [PATCH 02/10] fix: usage with multiple --- .../select-popover/select-popover.tsx | 11 +++---- .../select/test/basic/select.e2e.ts | 31 +++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/core/src/components/select-popover/select-popover.tsx b/core/src/components/select-popover/select-popover.tsx index 09f0970b26d..101ee222632 100644 --- a/core/src/components/select-popover/select-popover.tsx +++ b/core/src/components/select-popover/select-popover.tsx @@ -1,5 +1,5 @@ import type { ComponentInterface } from '@stencil/core'; -import { Component, Host, Listen, Prop, h } from '@stencil/core'; +import { Component, Host, Prop, h } from '@stencil/core'; import { getIonMode } from '../../global/ionic-global'; import { safeCall } from '../../utils/overlays'; @@ -46,11 +46,6 @@ export class SelectPopover implements ComponentInterface { */ @Prop() options: SelectPopoverOption[] = []; - @Listen('ionChange') - onSelect(ev: any) { - this.setChecked(ev); - } - private findOptionFromEvent(ev: any) { const { options } = this; return options.find((o) => o.value === ev.target.value); @@ -124,6 +119,10 @@ export class SelectPopover implements ComponentInterface { value={option.value} disabled={option.disabled} checked={option.checked} + onIonChange={(ev) => { + this.setChecked(ev); + this.callOptionHandler(ev); + }} > {option.text} diff --git a/core/src/components/select/test/basic/select.e2e.ts b/core/src/components/select/test/basic/select.e2e.ts index c327d30bd96..60ba9591d7f 100644 --- a/core/src/components/select/test/basic/select.e2e.ts +++ b/core/src/components/select/test/basic/select.e2e.ts @@ -159,6 +159,37 @@ test.describe('select: ionChange', () => { expect(ionChange).toHaveReceivedEventTimes(1); }); + test('should fire ionChange when confirming multiple values from a popover', async ({ page }) => { + await page.setContent(` + + Apple + Banana + + `); + + const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent'); + const select = page.locator('ion-select') as E2ELocator; + const ionChange = await select.spyOnEvent('ionChange'); + + await select.click(); + await ionPopoverDidPresent.next(); + + const popover = page.locator('ion-popover'); + const checkboxes = popover.locator('ion-checkbox'); + + await checkboxes.nth(0).click(); + await ionChange.next(); + + expect(ionChange).toHaveReceivedEventDetail({ value: ['apple'] }); + expect(ionChange).toHaveReceivedEventTimes(1); + + await checkboxes.nth(1).click(); + await ionChange.next(); + + expect(ionChange).toHaveReceivedEventDetail({ value: ['apple', 'banana'] }); + expect(ionChange).toHaveReceivedEventTimes(2); + }); + test('should fire ionChange when confirming a value from an action sheet', async ({ page }) => { await page.setContent(` From c49a12bcb4697673d734cf365254727c3c578079 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 15 Feb 2023 13:46:16 -0500 Subject: [PATCH 03/10] fix: select popover dismisses from single selection with keyboard --- .../select-popover/select-popover.tsx | 45 +++++-- .../test/basic/select-popover.e2e.ts | 125 ++++++++++++++++++ .../select-popover/test/fixtures.ts | 81 ++++++++++++ 3 files changed, 237 insertions(+), 14 deletions(-) create mode 100644 core/src/components/select-popover/test/basic/select-popover.e2e.ts create mode 100644 core/src/components/select-popover/test/fixtures.ts diff --git a/core/src/components/select-popover/select-popover.tsx b/core/src/components/select-popover/select-popover.tsx index 101ee222632..aceb26319bf 100644 --- a/core/src/components/select-popover/select-popover.tsx +++ b/core/src/components/select-popover/select-popover.tsx @@ -1,14 +1,14 @@ import type { ComponentInterface } from '@stencil/core'; -import { Component, Host, Prop, h } from '@stencil/core'; +import { Element, Component, Host, Prop, h } from '@stencil/core'; import { getIonMode } from '../../global/ionic-global'; import { safeCall } from '../../utils/overlays'; import { getClassMap } from '../../utils/theme'; +import type { CheckboxCustomEvent } from '../checkbox/checkbox-interface'; +import type { RadioGroupCustomEvent } from '../radio-group/radio-group-interface'; import type { SelectPopoverOption } from './select-popover-interface'; -// TODO(FW-2832): types - /** * @internal */ @@ -21,6 +21,7 @@ import type { SelectPopoverOption } from './select-popover-interface'; scoped: true, }) export class SelectPopover implements ComponentInterface { + @Element() el!: HTMLIonSelectPopoverElement; /** * The header text of the popover */ @@ -46,7 +47,7 @@ export class SelectPopover implements ComponentInterface { */ @Prop() options: SelectPopoverOption[] = []; - private findOptionFromEvent(ev: any) { + private findOptionFromEvent(ev: CheckboxCustomEvent | RadioGroupCustomEvent) { const { options } = this; return options.find((o) => o.value === ev.target.value); } @@ -56,7 +57,7 @@ export class SelectPopover implements ComponentInterface { * of the selected option(s) and return it in the option * handler */ - private callOptionHandler(ev: any) { + private callOptionHandler(ev: CheckboxCustomEvent | RadioGroupCustomEvent) { const option = this.findOptionFromEvent(ev); const values = this.getValues(ev); @@ -66,15 +67,17 @@ export class SelectPopover implements ComponentInterface { } /** - * This is required when selecting a radio that is already - * selected because it will not trigger the ionChange event - * but we still want to close the popover + * Dismisses the host popover that the `ion-select-popover` + * is rendered within. */ - private rbClick(ev: any) { - this.callOptionHandler(ev); + private dismissParentPopover() { + const popover = this.el.closest('ion-popover'); + if (popover) { + popover.dismiss(); + } } - private setChecked(ev: any): void { + private setChecked(ev: CheckboxCustomEvent): void { const { multiple } = this; const option = this.findOptionFromEvent(ev); @@ -85,7 +88,7 @@ export class SelectPopover implements ComponentInterface { } } - private getValues(ev: any): any | any[] | null { + private getValues(ev: CheckboxCustomEvent | RadioGroupCustomEvent): string | string[] | undefined { const { multiple, options } = this; if (multiple) { @@ -133,11 +136,25 @@ export class SelectPopover implements ComponentInterface { const checked = options.filter((o) => o.checked).map((o) => o.value)[0]; return ( - + this.callOptionHandler(ev)}> {options.map((option) => ( {option.text} - this.rbClick(ev)}> + this.dismissParentPopover()} + onKeyUp={(ev) => { + if (ev.key === 'Enter' || ev.key === ' ') { + /** + * Selecting a radio option with keyboard navigation, + * either through the Enter or Space keys, should + * dismiss the popover. + */ + this.dismissParentPopover(); + } + }} + > ))} diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts new file mode 100644 index 00000000000..2094d7299d7 --- /dev/null +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -0,0 +1,125 @@ +import { expect } from '@playwright/test'; +import { test } from '@utils/test/playwright'; + +import type { SelectPopoverOption } from '../../select-popover-interface'; +import { SelectPopoverPage } from '../fixtures'; + +test.describe('select-popover: basic', () => { + test.beforeEach(({ skip }) => { + skip.rtl(); + skip.mode('ios', 'Consistent behavior across modes'); + }); + + const options: SelectPopoverOption[] = [ + { value: 'apple', text: 'Apple', disabled: false, checked: false }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]; + + test.describe('single selection', () => { + let selectPopoverPage: SelectPopoverPage; + + test.beforeEach(async ({ page }) => { + selectPopoverPage = new SelectPopoverPage(page); + await selectPopoverPage.setup(options, false); + }); + + test('clicking an option should dismiss the popover', async () => { + await selectPopoverPage.clickOption('apple'); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + + test('clicking a selected option should dismiss the popover', async () => { + await selectPopoverPage.updateOptions([ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]); + + await selectPopoverPage.clickOption('apple'); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + + test('pressing Enter on an option should dismiss the popover', async () => { + await selectPopoverPage.pressEnterOnOption('apple'); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + + test('pressing Enter on a selected option should dismiss the popover', async () => { + await selectPopoverPage.updateOptions([ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]); + + await selectPopoverPage.pressEnterOnOption('apple'); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + + test('pressing Space on an option should dismiss the popover', async () => { + await selectPopoverPage.pressSpaceOnOption('apple'); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + + test('pressing Space on a selected option should dismiss the popover', async () => { + await selectPopoverPage.updateOptions([ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]); + + await selectPopoverPage.pressSpaceOnOption('apple'); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + }); + + test.describe('multiple selection', () => { + let selectPopoverPage: SelectPopoverPage; + + test.beforeEach(async ({ page }) => { + selectPopoverPage = new SelectPopoverPage(page); + await selectPopoverPage.setup(options, true); + }); + + test('clicking an option should not dismiss the popover', async () => { + await selectPopoverPage.clickOption('apple'); + await expect(selectPopoverPage.popover).toBeVisible(); + }); + + test('clicking a selected option should not dismiss the popover', async () => { + await selectPopoverPage.updateOptions([ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]); + + await selectPopoverPage.clickOption('apple'); + await expect(selectPopoverPage.popover).toBeVisible(); + }); + + test('pressing Enter on an option should not dismiss the popover', async () => { + await selectPopoverPage.pressEnterOnOption('apple'); + await expect(selectPopoverPage.popover).toBeVisible(); + }); + + test('pressing Enter on a selected option should not dismiss the popover', async () => { + await selectPopoverPage.updateOptions([ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]); + + await selectPopoverPage.pressEnterOnOption('apple'); + await expect(selectPopoverPage.popover).toBeVisible(); + }); + + test('pressing Space on an option should not dismiss the popover', async () => { + await selectPopoverPage.pressSpaceOnOption('apple'); + await expect(selectPopoverPage.popover).toBeVisible(); + }); + + test('pressing Space on a selected option should not dismiss the popover', async () => { + await selectPopoverPage.updateOptions([ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, + ]); + + await selectPopoverPage.pressSpaceOnOption('apple'); + await expect(selectPopoverPage.popover).toBeVisible(); + }); + }); +}); diff --git a/core/src/components/select-popover/test/fixtures.ts b/core/src/components/select-popover/test/fixtures.ts new file mode 100644 index 00000000000..40a3a5e190d --- /dev/null +++ b/core/src/components/select-popover/test/fixtures.ts @@ -0,0 +1,81 @@ +import type { E2EPage, E2ELocator } from '@utils/test/playwright'; + +import type { SelectPopoverOption } from '../select-popover-interface'; + +export class SelectPopoverPage { + private page: E2EPage; + private multiple?: boolean; + private options: SelectPopoverOption[] = []; + + popover!: E2ELocator; + selectPopover!: E2ELocator; + + constructor(page: E2EPage) { + this.page = page; + } + + async setup(options: SelectPopoverOption[], multiple = false) { + const { page } = this; + + await page.setContent(` + + + + `); + + const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent'); + + await page.evaluate( + ({ options, multiple }) => { + const selectPopover = document.querySelector('ion-select-popover')!; + selectPopover.options = options; + selectPopover.multiple = multiple; + }, + { options, multiple } + ); + + this.popover = await page.locator('ion-popover'); + this.selectPopover = await page.locator('ion-select-popover'); + this.multiple = multiple; + this.options = options; + + await this.popover.evaluate((popover: HTMLIonPopoverElement) => popover.present()); + + await ionPopoverDidPresent.next(); + } + + async updateOptions(options: SelectPopoverOption[]) { + const { page } = this; + await page.evaluate((options) => { + const selectPopover = document.querySelector('ion-select-popover')!; + selectPopover.options = options; + }, options); + + await page.waitForChanges(); + + this.options = options; + } + + async clickOption(value: string) { + const option = this.getOption(value); + await option.click(); + } + + async pressEnterOnOption(value: string) { + const option = this.getOption(value); + await option.press('Enter'); + } + + async pressSpaceOnOption(value: string) { + const option = this.getOption(value); + await option.press('Space'); + } + + private getOption(value: string) { + const { multiple, selectPopover } = this; + const selector = multiple ? 'ion-checkbox' : 'ion-radio'; + const index = this.options.findIndex((o) => o.value === value); + + return selectPopover.locator(selector).nth(index); + } +} From 4abe30c1d14c2f2c6c6773c6f94277971a555577 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 15 Feb 2023 14:01:46 -0500 Subject: [PATCH 04/10] chore: solve test flakiness --- .../select-popover/test/basic/select-popover.e2e.ts | 6 ++++++ core/src/components/select-popover/test/fixtures.ts | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts index 2094d7299d7..f81868a0427 100644 --- a/core/src/components/select-popover/test/basic/select-popover.e2e.ts +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -25,6 +25,7 @@ test.describe('select-popover: basic', () => { test('clicking an option should dismiss the popover', async () => { await selectPopoverPage.clickOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); @@ -35,11 +36,13 @@ test.describe('select-popover: basic', () => { ]); await selectPopoverPage.clickOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); test('pressing Enter on an option should dismiss the popover', async () => { await selectPopoverPage.pressEnterOnOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); @@ -50,11 +53,13 @@ test.describe('select-popover: basic', () => { ]); await selectPopoverPage.pressEnterOnOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); test('pressing Space on an option should dismiss the popover', async () => { await selectPopoverPage.pressSpaceOnOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); @@ -65,6 +70,7 @@ test.describe('select-popover: basic', () => { ]); await selectPopoverPage.pressSpaceOnOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); }); diff --git a/core/src/components/select-popover/test/fixtures.ts b/core/src/components/select-popover/test/fixtures.ts index 40a3a5e190d..1745dffdb89 100644 --- a/core/src/components/select-popover/test/fixtures.ts +++ b/core/src/components/select-popover/test/fixtures.ts @@ -1,4 +1,4 @@ -import type { E2EPage, E2ELocator } from '@utils/test/playwright'; +import type { E2EPage, E2ELocator, EventSpy } from '@utils/test/playwright'; import type { SelectPopoverOption } from '../select-popover-interface'; @@ -7,9 +7,13 @@ export class SelectPopoverPage { private multiple?: boolean; private options: SelectPopoverOption[] = []; + // Locators popover!: E2ELocator; selectPopover!: E2ELocator; + // Event spies + ionPopoverDidDismiss!: EventSpy; + constructor(page: E2EPage) { this.page = page; } @@ -24,6 +28,7 @@ export class SelectPopoverPage { `); const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent'); + this.ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss'); await page.evaluate( ({ options, multiple }) => { From a014c1f5c84826a722b1bad3701dc4617c732de0 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 15 Feb 2023 16:36:25 -0500 Subject: [PATCH 05/10] chore: skip flaky test --- .../select-popover/test/basic/select-popover.e2e.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts index f81868a0427..141d2580e15 100644 --- a/core/src/components/select-popover/test/basic/select-popover.e2e.ts +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -5,9 +5,10 @@ import type { SelectPopoverOption } from '../../select-popover-interface'; import { SelectPopoverPage } from '../fixtures'; test.describe('select-popover: basic', () => { - test.beforeEach(({ skip }) => { + test.beforeEach(({ skip, browserName }) => { skip.rtl(); skip.mode('ios', 'Consistent behavior across modes'); + test.skip(browserName === 'webkit', 'https://ionic-cloud.atlassian.net/browse/FW-2979'); }); const options: SelectPopoverOption[] = [ @@ -63,7 +64,9 @@ test.describe('select-popover: basic', () => { await expect(selectPopoverPage.popover).not.toBeVisible(); }); - test('pressing Space on a selected option should dismiss the popover', async () => { + test('pressing Space on a selected option should dismiss the popover', async ({ browserName }) => { + test.skip(browserName === 'firefox', 'Same behavior as https://ionic-cloud.atlassian.net/browse/FW-2979'); + await selectPopoverPage.updateOptions([ { value: 'apple', text: 'Apple', disabled: false, checked: true }, { value: 'banana', text: 'Banana', disabled: false, checked: false }, From a753d088ac31565d5697b2cc8453a605c4c698d4 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 15 Feb 2023 16:58:56 -0500 Subject: [PATCH 06/10] chore: remove invalid enter-key tests --- .../test/basic/select-popover.e2e.ts | 32 ------------------- .../select-popover/test/fixtures.ts | 5 --- 2 files changed, 37 deletions(-) diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts index 141d2580e15..ecfc69e7953 100644 --- a/core/src/components/select-popover/test/basic/select-popover.e2e.ts +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -41,23 +41,6 @@ test.describe('select-popover: basic', () => { await expect(selectPopoverPage.popover).not.toBeVisible(); }); - test('pressing Enter on an option should dismiss the popover', async () => { - await selectPopoverPage.pressEnterOnOption('apple'); - await selectPopoverPage.ionPopoverDidDismiss.next(); - await expect(selectPopoverPage.popover).not.toBeVisible(); - }); - - test('pressing Enter on a selected option should dismiss the popover', async () => { - await selectPopoverPage.updateOptions([ - { value: 'apple', text: 'Apple', disabled: false, checked: true }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]); - - await selectPopoverPage.pressEnterOnOption('apple'); - await selectPopoverPage.ionPopoverDidDismiss.next(); - await expect(selectPopoverPage.popover).not.toBeVisible(); - }); - test('pressing Space on an option should dismiss the popover', async () => { await selectPopoverPage.pressSpaceOnOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); @@ -101,21 +84,6 @@ test.describe('select-popover: basic', () => { await expect(selectPopoverPage.popover).toBeVisible(); }); - test('pressing Enter on an option should not dismiss the popover', async () => { - await selectPopoverPage.pressEnterOnOption('apple'); - await expect(selectPopoverPage.popover).toBeVisible(); - }); - - test('pressing Enter on a selected option should not dismiss the popover', async () => { - await selectPopoverPage.updateOptions([ - { value: 'apple', text: 'Apple', disabled: false, checked: true }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]); - - await selectPopoverPage.pressEnterOnOption('apple'); - await expect(selectPopoverPage.popover).toBeVisible(); - }); - test('pressing Space on an option should not dismiss the popover', async () => { await selectPopoverPage.pressSpaceOnOption('apple'); await expect(selectPopoverPage.popover).toBeVisible(); diff --git a/core/src/components/select-popover/test/fixtures.ts b/core/src/components/select-popover/test/fixtures.ts index 1745dffdb89..a01f8b08ef8 100644 --- a/core/src/components/select-popover/test/fixtures.ts +++ b/core/src/components/select-popover/test/fixtures.ts @@ -66,11 +66,6 @@ export class SelectPopoverPage { await option.click(); } - async pressEnterOnOption(value: string) { - const option = this.getOption(value); - await option.press('Enter'); - } - async pressSpaceOnOption(value: string) { const option = this.getOption(value); await option.press('Space'); From 8b86062a40b60f97a6cc0c1c3b11f68594333a43 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 15 Feb 2023 18:23:20 -0500 Subject: [PATCH 07/10] fix: use legacy prop, remove unnecessary tests --- .../select-popover/select-popover.tsx | 2 + .../select-popover/test/basic/index.html | 40 +++++++++++++++++ .../test/basic/select-popover.e2e.ts | 45 ++----------------- .../select-popover/test/fixtures.ts | 2 + 4 files changed, 47 insertions(+), 42 deletions(-) create mode 100644 core/src/components/select-popover/test/basic/index.html diff --git a/core/src/components/select-popover/select-popover.tsx b/core/src/components/select-popover/select-popover.tsx index aceb26319bf..fcd3bed56d2 100644 --- a/core/src/components/select-popover/select-popover.tsx +++ b/core/src/components/select-popover/select-popover.tsx @@ -122,6 +122,7 @@ export class SelectPopover implements ComponentInterface { value={option.value} disabled={option.disabled} checked={option.checked} + legacy={true} onIonChange={(ev) => { this.setChecked(ev); this.callOptionHandler(ev); @@ -143,6 +144,7 @@ export class SelectPopover implements ComponentInterface { this.dismissParentPopover()} onKeyUp={(ev) => { if (ev.key === 'Enter' || ev.key === ' ') { diff --git a/core/src/components/select-popover/test/basic/index.html b/core/src/components/select-popover/test/basic/index.html new file mode 100644 index 00000000000..69b0e78ceba --- /dev/null +++ b/core/src/components/select-popover/test/basic/index.html @@ -0,0 +1,40 @@ + + + + + Select - Popover + + + + + + + + + + + + + Select Popover - Basic + + + + + + + + + + + + + diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts index ecfc69e7953..eb922abcd19 100644 --- a/core/src/components/select-popover/test/basic/select-popover.e2e.ts +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -24,7 +24,7 @@ test.describe('select-popover: basic', () => { await selectPopoverPage.setup(options, false); }); - test('clicking an option should dismiss the popover', async () => { + test('clicking an unselected option should dismiss the popover', async () => { await selectPopoverPage.clickOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); @@ -41,13 +41,13 @@ test.describe('select-popover: basic', () => { await expect(selectPopoverPage.popover).not.toBeVisible(); }); - test('pressing Space on an option should dismiss the popover', async () => { + test('pressing Space on an unselected option should dismiss the popover', async () => { await selectPopoverPage.pressSpaceOnOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); - test('pressing Space on a selected option should dismiss the popover', async ({ browserName }) => { + test('pressing Space on a selected option should dismiss the popover', async ({ page, browserName }) => { test.skip(browserName === 'firefox', 'Same behavior as https://ionic-cloud.atlassian.net/browse/FW-2979'); await selectPopoverPage.updateOptions([ @@ -60,43 +60,4 @@ test.describe('select-popover: basic', () => { await expect(selectPopoverPage.popover).not.toBeVisible(); }); }); - - test.describe('multiple selection', () => { - let selectPopoverPage: SelectPopoverPage; - - test.beforeEach(async ({ page }) => { - selectPopoverPage = new SelectPopoverPage(page); - await selectPopoverPage.setup(options, true); - }); - - test('clicking an option should not dismiss the popover', async () => { - await selectPopoverPage.clickOption('apple'); - await expect(selectPopoverPage.popover).toBeVisible(); - }); - - test('clicking a selected option should not dismiss the popover', async () => { - await selectPopoverPage.updateOptions([ - { value: 'apple', text: 'Apple', disabled: false, checked: true }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]); - - await selectPopoverPage.clickOption('apple'); - await expect(selectPopoverPage.popover).toBeVisible(); - }); - - test('pressing Space on an option should not dismiss the popover', async () => { - await selectPopoverPage.pressSpaceOnOption('apple'); - await expect(selectPopoverPage.popover).toBeVisible(); - }); - - test('pressing Space on a selected option should not dismiss the popover', async () => { - await selectPopoverPage.updateOptions([ - { value: 'apple', text: 'Apple', disabled: false, checked: true }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]); - - await selectPopoverPage.pressSpaceOnOption('apple'); - await expect(selectPopoverPage.popover).toBeVisible(); - }); - }); }); diff --git a/core/src/components/select-popover/test/fixtures.ts b/core/src/components/select-popover/test/fixtures.ts index a01f8b08ef8..92d9711fcff 100644 --- a/core/src/components/select-popover/test/fixtures.ts +++ b/core/src/components/select-popover/test/fixtures.ts @@ -39,6 +39,8 @@ export class SelectPopoverPage { { options, multiple } ); + await page.waitForChanges(); + this.popover = await page.locator('ion-popover'); this.selectPopover = await page.locator('ion-select-popover'); this.multiple = multiple; From 1f4e51a6ec6a2ab158c6f2d50fb4558efa470544 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Thu, 16 Feb 2023 11:49:39 -0500 Subject: [PATCH 08/10] chore: update warning message, remove unused prop --- core/src/components/checkbox/checkbox.tsx | 4 +++- .../select-popover/test/basic/select-popover.e2e.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/core/src/components/checkbox/checkbox.tsx b/core/src/components/checkbox/checkbox.tsx index 0e7e3c3c6a5..e09769a87bc 100644 --- a/core/src/components/checkbox/checkbox.tsx +++ b/core/src/components/checkbox/checkbox.tsx @@ -289,7 +289,9 @@ Example: Label For checkboxes that do not have a visible label, developers should use "aria-label" so screen readers can announce the purpose of the checkbox. -For checkboxes that do not render the label immediately next to the checkbox, developers may continue to use "ion-label" but must manually associate the label with the checkbox by using "aria-labelledby".`, +For checkboxes that do not render the label immediately next to the checkbox, developers may continue to use "ion-label" but must manually associate the label with the checkbox by using "aria-labelledby". + +Developers can use the "legacy" property to continue using the legacy form markup. This property will be removed in an upcoming major release of Ionic where this form control will use the modern form markup.`, this.el ); diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts index eb922abcd19..17669e2d252 100644 --- a/core/src/components/select-popover/test/basic/select-popover.e2e.ts +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -47,7 +47,7 @@ test.describe('select-popover: basic', () => { await expect(selectPopoverPage.popover).not.toBeVisible(); }); - test('pressing Space on a selected option should dismiss the popover', async ({ page, browserName }) => { + test('pressing Space on a selected option should dismiss the popover', async ({ browserName }) => { test.skip(browserName === 'firefox', 'Same behavior as https://ionic-cloud.atlassian.net/browse/FW-2979'); await selectPopoverPage.updateOptions([ From 76ff317b5651256c880fac4a46a91904d113b341 Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Thu, 16 Feb 2023 22:43:36 -0500 Subject: [PATCH 09/10] fix: align keyboard interaction with radio group --- core/src/components/select-popover/select-popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/select-popover/select-popover.tsx b/core/src/components/select-popover/select-popover.tsx index fcd3bed56d2..605269d407d 100644 --- a/core/src/components/select-popover/select-popover.tsx +++ b/core/src/components/select-popover/select-popover.tsx @@ -147,7 +147,7 @@ export class SelectPopover implements ComponentInterface { legacy={true} onClick={() => this.dismissParentPopover()} onKeyUp={(ev) => { - if (ev.key === 'Enter' || ev.key === ' ') { + if (ev.key === ' ') { /** * Selecting a radio option with keyboard navigation, * either through the Enter or Space keys, should From 7fcc1500e0850b9b0a65e8a2cd0a07d156d3b24f Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Thu, 16 Feb 2023 22:43:56 -0500 Subject: [PATCH 10/10] refactor: speed up tests --- .../test/basic/select-popover.e2e.ts | 30 +++++++++-------- .../select-popover/test/fixtures.ts | 32 ++++--------------- 2 files changed, 23 insertions(+), 39 deletions(-) diff --git a/core/src/components/select-popover/test/basic/select-popover.e2e.ts b/core/src/components/select-popover/test/basic/select-popover.e2e.ts index 17669e2d252..6f24b16bb72 100644 --- a/core/src/components/select-popover/test/basic/select-popover.e2e.ts +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -4,6 +4,16 @@ import { test } from '@utils/test/playwright'; import type { SelectPopoverOption } from '../../select-popover-interface'; import { SelectPopoverPage } from '../fixtures'; +const options: SelectPopoverOption[] = [ + { value: 'apple', text: 'Apple', disabled: false, checked: false }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, +]; + +const checkedOptions: SelectPopoverOption[] = [ + { value: 'apple', text: 'Apple', disabled: false, checked: true }, + { value: 'banana', text: 'Banana', disabled: false, checked: false }, +]; + test.describe('select-popover: basic', () => { test.beforeEach(({ skip, browserName }) => { skip.rtl(); @@ -11,30 +21,23 @@ test.describe('select-popover: basic', () => { test.skip(browserName === 'webkit', 'https://ionic-cloud.atlassian.net/browse/FW-2979'); }); - const options: SelectPopoverOption[] = [ - { value: 'apple', text: 'Apple', disabled: false, checked: false }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]; - test.describe('single selection', () => { let selectPopoverPage: SelectPopoverPage; test.beforeEach(async ({ page }) => { selectPopoverPage = new SelectPopoverPage(page); - await selectPopoverPage.setup(options, false); }); test('clicking an unselected option should dismiss the popover', async () => { + await selectPopoverPage.setup(options, false); + await selectPopoverPage.clickOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); }); test('clicking a selected option should dismiss the popover', async () => { - await selectPopoverPage.updateOptions([ - { value: 'apple', text: 'Apple', disabled: false, checked: true }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]); + await selectPopoverPage.setup(checkedOptions, false); await selectPopoverPage.clickOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); @@ -42,6 +45,8 @@ test.describe('select-popover: basic', () => { }); test('pressing Space on an unselected option should dismiss the popover', async () => { + await selectPopoverPage.setup(options, false); + await selectPopoverPage.pressSpaceOnOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); await expect(selectPopoverPage.popover).not.toBeVisible(); @@ -50,10 +55,7 @@ test.describe('select-popover: basic', () => { test('pressing Space on a selected option should dismiss the popover', async ({ browserName }) => { test.skip(browserName === 'firefox', 'Same behavior as https://ionic-cloud.atlassian.net/browse/FW-2979'); - await selectPopoverPage.updateOptions([ - { value: 'apple', text: 'Apple', disabled: false, checked: true }, - { value: 'banana', text: 'Banana', disabled: false, checked: false }, - ]); + await selectPopoverPage.setup(checkedOptions, false); await selectPopoverPage.pressSpaceOnOption('apple'); await selectPopoverPage.ionPopoverDidDismiss.next(); diff --git a/core/src/components/select-popover/test/fixtures.ts b/core/src/components/select-popover/test/fixtures.ts index 92d9711fcff..3d15378129d 100644 --- a/core/src/components/select-popover/test/fixtures.ts +++ b/core/src/components/select-popover/test/fixtures.ts @@ -25,24 +25,18 @@ export class SelectPopoverPage { + `); const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent'); this.ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss'); - await page.evaluate( - ({ options, multiple }) => { - const selectPopover = document.querySelector('ion-select-popover')!; - selectPopover.options = options; - selectPopover.multiple = multiple; - }, - { options, multiple } - ); - - await page.waitForChanges(); - - this.popover = await page.locator('ion-popover'); - this.selectPopover = await page.locator('ion-select-popover'); + this.popover = page.locator('ion-popover'); + this.selectPopover = page.locator('ion-select-popover'); this.multiple = multiple; this.options = options; @@ -51,18 +45,6 @@ export class SelectPopoverPage { await ionPopoverDidPresent.next(); } - async updateOptions(options: SelectPopoverOption[]) { - const { page } = this; - await page.evaluate((options) => { - const selectPopover = document.querySelector('ion-select-popover')!; - selectPopover.options = options; - }, options); - - await page.waitForChanges(); - - this.options = options; - } - async clickOption(value: string) { const option = this.getOption(value); await option.click();