diff --git a/core/src/components/checkbox/checkbox.tsx b/core/src/components/checkbox/checkbox.tsx index f32ebf7a087..9cb1270b13a 100644 --- a/core/src/components/checkbox/checkbox.tsx +++ b/core/src/components/checkbox/checkbox.tsx @@ -288,7 +288,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/select-popover.tsx b/core/src/components/select-popover/select-popover.tsx index 56da639a704..605269d407d 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, Listen, 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,13 +47,7 @@ export class SelectPopover implements ComponentInterface { */ @Prop() options: SelectPopoverOption[] = []; - @Listen('ionChange') - onSelect(ev: any) { - this.setChecked(ev); - this.callOptionHandler(ev); - } - - private findOptionFromEvent(ev: any) { + private findOptionFromEvent(ev: CheckboxCustomEvent | RadioGroupCustomEvent) { const { options } = this; return options.find((o) => o.value === ev.target.value); } @@ -62,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); @@ -72,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); @@ -91,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) { @@ -125,6 +122,11 @@ export class SelectPopover implements ComponentInterface { value={option.value} disabled={option.disabled} checked={option.checked} + legacy={true} + onIonChange={(ev) => { + this.setChecked(ev); + this.callOptionHandler(ev); + }} > {option.text} @@ -135,11 +137,26 @@ 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 === ' ') { + /** + * 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/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 new file mode 100644 index 00000000000..6f24b16bb72 --- /dev/null +++ b/core/src/components/select-popover/test/basic/select-popover.e2e.ts @@ -0,0 +1,65 @@ +import { expect } from '@playwright/test'; +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(); + skip.mode('ios', 'Consistent behavior across modes'); + test.skip(browserName === 'webkit', 'https://ionic-cloud.atlassian.net/browse/FW-2979'); + }); + + test.describe('single selection', () => { + let selectPopoverPage: SelectPopoverPage; + + test.beforeEach(async ({ page }) => { + selectPopoverPage = new SelectPopoverPage(page); + }); + + 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.setup(checkedOptions, false); + + await selectPopoverPage.clickOption('apple'); + await selectPopoverPage.ionPopoverDidDismiss.next(); + await expect(selectPopoverPage.popover).not.toBeVisible(); + }); + + 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(); + }); + + 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.setup(checkedOptions, false); + + 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 new file mode 100644 index 00000000000..3d15378129d --- /dev/null +++ b/core/src/components/select-popover/test/fixtures.ts @@ -0,0 +1,65 @@ +import type { E2EPage, E2ELocator, EventSpy } from '@utils/test/playwright'; + +import type { SelectPopoverOption } from '../select-popover-interface'; + +export class SelectPopoverPage { + private page: E2EPage; + private multiple?: boolean; + private options: SelectPopoverOption[] = []; + + // Locators + popover!: E2ELocator; + selectPopover!: E2ELocator; + + // Event spies + ionPopoverDidDismiss!: EventSpy; + + constructor(page: E2EPage) { + this.page = page; + } + + async setup(options: SelectPopoverOption[], multiple = false) { + const { page } = this; + + await page.setContent(` + + + + + `); + + const ionPopoverDidPresent = await page.spyOnEvent('ionPopoverDidPresent'); + this.ionPopoverDidDismiss = await page.spyOnEvent('ionPopoverDidDismiss'); + + this.popover = page.locator('ion-popover'); + this.selectPopover = page.locator('ion-select-popover'); + this.multiple = multiple; + this.options = options; + + await this.popover.evaluate((popover: HTMLIonPopoverElement) => popover.present()); + + await ionPopoverDidPresent.next(); + } + + async clickOption(value: string) { + const option = this.getOption(value); + await option.click(); + } + + 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); + } +} diff --git a/core/src/components/select/test/basic/select.e2e.ts b/core/src/components/select/test/basic/select.e2e.ts index 995b7bea314..60ba9591d7f 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,39 @@ 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 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 }) => { @@ -178,6 +212,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';