From ba2f49b8a460520d20ac198db800ea2d9e5b015f Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 31 Jul 2023 10:07:44 -0400 Subject: [PATCH] fix(radio): radios can be focused and are announced with group (#27817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue number: resolves #27438 --------- ## What is the current behavior? There are a few issues with the modern radio syntax: 1. The native radio is inside the Shadow DOM. As a result, radios are not announced with their parent group with screen readers (i.e. "1 of 3") 2. The native radio cannot be focused inside of `ion-select-popover` on Firefox. 3. The `ionFocus` and `ionBlur` events do not fire. I also discovered an issue with item: 1. Items inside of a Radio Group have a role of `listitem` which prevent radios from being grouped correctly in some browsers. According to https://bugzilla.mozilla.org/show_bug.cgi?id=1840916, browsers are behaving correctly here. The `listitem` role should not be present when an item is used in a radio group (even if the radio group itself is inside a list). ## What is the new behavior? Most of the changes are test-related, but I broke it down per commit to make this easier to review: https://github.com/ionic-team/ionic-framework/pull/27817/commits/ae77002afd7465c2684c882e1cc769808929fbdd - Item no longer has `role="listitem"` when used inside of a radio group. - Added spec tests to verify the role behavior https://github.com/ionic-team/ionic-framework/pull/27817/commits/0a9b7fb91d5972dc3f20178ab3ee3360bdde1854 - I discovered that some the legacy basic test were accidentally using the modern syntax. I corrected this by adding `legacy="true"` to the radios. https://github.com/ionic-team/ionic-framework/pull/27817/commits/a8a90e53b2464c3bd6f58ad672e741aa3af23477, https://github.com/ionic-team/ionic-framework/pull/27817/commits/412d1d54e7e2bd3af6cd61a37a3a84da89e0d305, and https://github.com/ionic-team/ionic-framework/pull/27817/commits/1d1179b69ae0c707220a5142f8aa05217d4fa49b - The current radio group tests only tested the legacy radio syntax, and not the modern syntax. - I created a `legacy` directory to house the legacy syntax tests. - I created new tests in the root test directory for the modern syntax. - I also deleted the screenshots for the modern tests here because the tests for `ion-radio` already take screenshots of the radio (even in an item). https://github.com/ionic-team/ionic-framework/pull/27817/commits/e2c966e68bf7268eef02c765686cb7e7bf3cfb77 - Moved radio roles to the host. This allows Firefox to focus radios and for screen readers to announce the radios as part of a group. - I also added focus/blur listeners so ionFocus and ionBlur fire https://github.com/ionic-team/ionic-framework/pull/27817/commits/f10eff47a5a84b211c9a50c258b35921c95f6985 - I cleaned up the tests here to use a common radio fixture ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information I tested this with the following setups. ✅ indicates the screen reader announces the group count (i.e. "1 of 4"). ❌ indicates the screen reader does not announce the group count. **Radio in Radio Group:** - iOS + VoiceOver: ✅ - Android + TalkBack: ✅ - macOS + VoiceOver + Safari: ✅ - macOS + VoiceOver + Firefox: ✅ - macOS + VoiceOver + Chrome: ✅ - Windows + NVDA + Chrome: ✅ - Windows + NVDA + Firefox: ✅ **Radio in Item in Radio Group :** - iOS + VoiceOver: ✅ - Android + TalkBack: ❌ (https://bugs.chromium.org/p/chromium/issues/detail?id=1459006) - macOS + VoiceOver + Safari: ✅ - macOS + VoiceOver + Firefox: ✅ - macOS + VoiceOver + Chrome: ❌ (https://bugs.chromium.org/p/chromium/issues/detail?id=1459003) - Windows + NVDA + Chrome: ✅ - Windows + NVDA + Firefox: ✅ --- core/src/components/item/item.tsx | 2 +- .../components/item/test/a11y/item.spec.ts | 51 ++++++ .../radio-group/test/basic/index.html | 12 +- .../radio-group/test/basic/radio-group.e2e.ts | 66 +------ .../components/radio-group/test/fixtures.ts | 39 +++++ .../radio-group/test/form/index.html | 21 +-- .../radio-group/test/legacy/basic/index.html | 56 ++++++ .../test/legacy/basic/radio-group.e2e.ts | 163 ++++++++++++++++++ ...group-diff-ios-ltr-Mobile-Chrome-linux.png | Bin ...roup-diff-ios-ltr-Mobile-Firefox-linux.png | Bin ...group-diff-ios-ltr-Mobile-Safari-linux.png | Bin ...group-diff-ios-rtl-Mobile-Chrome-linux.png | Bin ...roup-diff-ios-rtl-Mobile-Firefox-linux.png | Bin ...group-diff-ios-rtl-Mobile-Safari-linux.png | Bin ...-group-diff-md-ltr-Mobile-Chrome-linux.png | Bin ...group-diff-md-ltr-Mobile-Firefox-linux.png | Bin ...-group-diff-md-ltr-Mobile-Safari-linux.png | Bin ...-group-diff-md-rtl-Mobile-Chrome-linux.png | Bin ...group-diff-md-rtl-Mobile-Firefox-linux.png | Bin ...-group-diff-md-rtl-Mobile-Safari-linux.png | Bin .../radio-group/test/legacy/form/index.html | 114 ++++++++++++ .../test/legacy/form/radio-group.e2e.ts | 35 ++++ .../radio-group/test/legacy/search/index.html | 82 +++++++++ .../test/legacy/search/radio-group.e2e.ts | 42 +++++ .../radio-group/test/search/index.html | 3 +- .../test/search/radio-group.e2e.ts | 49 +++--- core/src/components/radio/radio.tsx | 74 ++++---- .../components/radio/test/a11y/radio.e2e.ts | 5 +- .../radio/test/legacy/basic/radio.e2e.ts | 4 +- 29 files changed, 664 insertions(+), 154 deletions(-) create mode 100644 core/src/components/item/test/a11y/item.spec.ts create mode 100644 core/src/components/radio-group/test/fixtures.ts create mode 100644 core/src/components/radio-group/test/legacy/basic/index.html create mode 100644 core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Chrome-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Firefox-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Safari-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Chrome-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Firefox-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Safari-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Chrome-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Firefox-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Safari-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Chrome-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Firefox-linux.png (100%) rename core/src/components/radio-group/test/{ => legacy}/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Safari-linux.png (100%) create mode 100644 core/src/components/radio-group/test/legacy/form/index.html create mode 100644 core/src/components/radio-group/test/legacy/form/radio-group.e2e.ts create mode 100644 core/src/components/radio-group/test/legacy/search/index.html create mode 100644 core/src/components/radio-group/test/legacy/search/radio-group.e2e.ts diff --git a/core/src/components/item/item.tsx b/core/src/components/item/item.tsx index 4e06b0ec52..6e2d48186d 100644 --- a/core/src/components/item/item.tsx +++ b/core/src/components/item/item.tsx @@ -398,7 +398,7 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac }); const ariaDisabled = disabled || childStyles['item-interactive-disabled'] ? 'true' : null; const fillValue = fill || 'none'; - const inList = hostContext('ion-list', this.el); + const inList = hostContext('ion-list', this.el) && !hostContext('ion-radio-group', this.el); return ( { + it('should not have a role when used without list', async () => { + const page = await newSpecPage({ + components: [Item], + html: `Hello World`, + }); + + const item = page.body.querySelector('ion-item'); + expect(item.getAttribute('role')).toBe(null); + }); + + it('should have a listitem role when used inside list', async () => { + const page = await newSpecPage({ + components: [Item, List], + html: ` + + + Hello World + + + `, + }); + + const item = page.body.querySelector('ion-item'); + expect(item.getAttribute('role')).toBe('listitem'); + }); + + it('should not have a role when used inside radio group and list', async () => { + const page = await newSpecPage({ + components: [Radio, RadioGroup, Item, List], + html: ` + + + + + + + + `, + }); + + const item = page.body.querySelector('ion-item'); + expect(item.getAttribute('role')).toBe(null); + }); +}); diff --git a/core/src/components/radio-group/test/basic/index.html b/core/src/components/radio-group/test/basic/index.html index 5b690307e3..650695cd2b 100644 --- a/core/src/components/radio-group/test/basic/index.html +++ b/core/src/components/radio-group/test/basic/index.html @@ -30,23 +30,19 @@ - Item 1 - + Item 1 - Item 2 - + Item 2 - Item 3 - + Item 3 - Item 4 - + Item 4 diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts b/core/src/components/radio-group/test/basic/radio-group.e2e.ts index 06d6dbad14..96b7b5d955 100644 --- a/core/src/components/radio-group/test/basic/radio-group.e2e.ts +++ b/core/src/components/radio-group/test/basic/radio-group.e2e.ts @@ -1,19 +1,7 @@ import { expect } from '@playwright/test'; -import type { Locator } from '@playwright/test'; import { configs, test } from '@utils/test/playwright'; -import type { E2EPage } from '@utils/test/playwright'; -configs().forEach(({ title, screenshot, config }) => { - test.describe(title('radio-group: basic'), () => { - test('should not have visual regressions', async ({ page }) => { - await page.goto(`/src/components/radio-group/test/basic`, config); - - const list = page.locator('ion-list'); - - await expect(list).toHaveScreenshot(screenshot(`radio-group-diff`)); - }); - }); -}); +import { RadioFixture } from '../fixtures'; /** * This behavior does not vary across modes/directions. @@ -31,8 +19,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => ` - One - + One `, @@ -48,8 +35,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => ` - One - + One `, @@ -65,8 +51,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => ` - One - + One `, @@ -82,8 +67,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => ` - One - + One `, @@ -99,18 +83,15 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => ` - Item 1 - + Item 1 - Item 2 - + Item 2 - Item 3 - + Item 3 `, @@ -130,34 +111,3 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => }); }); }); - -class RadioFixture { - readonly page: E2EPage; - - private radio!: Locator; - - constructor(page: E2EPage) { - this.page = page; - } - - async checkRadio(method: 'keyboard' | 'mouse', selector = 'ion-radio') { - const { page } = this; - const radio = (this.radio = page.locator(selector)); - - if (method === 'keyboard') { - await radio.focus(); - await page.keyboard.press('Space'); - } else { - await radio.click(); - } - - await page.waitForChanges(); - - return radio; - } - - async expectChecked(state: boolean) { - const { radio } = this; - await expect(radio.locator('input')).toHaveJSProperty('checked', state); - } -} diff --git a/core/src/components/radio-group/test/fixtures.ts b/core/src/components/radio-group/test/fixtures.ts new file mode 100644 index 0000000000..21722126d7 --- /dev/null +++ b/core/src/components/radio-group/test/fixtures.ts @@ -0,0 +1,39 @@ +import type { Locator } from '@playwright/test'; +import { expect } from '@playwright/test'; +import type { E2EPage } from '@utils/test/playwright'; + +export class RadioFixture { + readonly page: E2EPage; + + private radio!: Locator; + + constructor(page: E2EPage) { + this.page = page; + } + + async checkRadio(method: 'keyboard' | 'mouse', selector = 'ion-radio') { + const { page } = this; + const radio = (this.radio = page.locator(selector)); + + if (method === 'keyboard') { + await radio.focus(); + await page.keyboard.press('Space'); + } else { + await radio.click(); + } + + await page.waitForChanges(); + + return radio; + } + + async expectChecked(state: boolean) { + const { radio } = this; + + if (state) { + await expect(radio).toHaveClass(/radio-checked/); + } else { + await expect(radio).not.toHaveClass(/radio-checked/); + } + } +} diff --git a/core/src/components/radio-group/test/form/index.html b/core/src/components/radio-group/test/form/index.html index 159f8c6249..6cbe354b4c 100644 --- a/core/src/components/radio-group/test/form/index.html +++ b/core/src/components/radio-group/test/form/index.html @@ -31,32 +31,19 @@ - Biff - - - + Biff - Griff - - - + Griff - Buford - - - + Buford - George - + George diff --git a/core/src/components/radio-group/test/legacy/basic/index.html b/core/src/components/radio-group/test/legacy/basic/index.html new file mode 100644 index 0000000000..5b690307e3 --- /dev/null +++ b/core/src/components/radio-group/test/legacy/basic/index.html @@ -0,0 +1,56 @@ + + + + + Radio Group - Basic + + + + + + + + + + + + + Radio Group - Basic + + + + + + + + Radio Group Header + + + + Item 1 + + + + + Item 2 + + + + + Item 3 + + + + + Item 4 + + + + + + + + diff --git a/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts new file mode 100644 index 0000000000..470ce52bcc --- /dev/null +++ b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts @@ -0,0 +1,163 @@ +import { expect } from '@playwright/test'; +import type { Locator } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; +import type { E2EPage } from '@utils/test/playwright'; + +configs().forEach(({ title, screenshot, config }) => { + test.describe(title('radio-group: basic'), () => { + test('should not have visual regressions', async ({ page }) => { + await page.goto(`/src/components/radio-group/test/legacy/basic`, config); + + const list = page.locator('ion-list'); + + await expect(list).toHaveScreenshot(screenshot(`radio-group-diff`)); + }); + }); +}); + +/** + * This behavior does not vary across modes/directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('radio-group: interaction'), () => { + let radioFixture: RadioFixture; + + test.beforeEach(({ page }) => { + radioFixture = new RadioFixture(page); + }); + + test('spacebar should not deselect without allowEmptySelection', async ({ page }) => { + await page.setContent( + ` + + + One + + + + `, + config + ); + + await radioFixture.checkRadio('keyboard'); + await radioFixture.expectChecked(true); + }); + + test('spacebar should deselect with allowEmptySelection', async ({ page }) => { + await page.setContent( + ` + + + One + + + + `, + config + ); + + await radioFixture.checkRadio('keyboard'); + await radioFixture.expectChecked(false); + }); + + test('click should not deselect without allowEmptySelection', async ({ page }) => { + await page.setContent( + ` + + + One + + + + `, + config + ); + + await radioFixture.checkRadio('mouse'); + await radioFixture.expectChecked(true); + }); + + test('click should deselect with allowEmptySelection', async ({ page }) => { + await page.setContent( + ` + + + One + + + + `, + config + ); + + await radioFixture.checkRadio('mouse'); + await radioFixture.expectChecked(false); + }); + + test('programmatically assigning a value should update the checked radio', async ({ page }) => { + await page.setContent( + ` + + + Item 1 + + + + + Item 2 + + + + + Item 3 + + + + `, + config + ); + + const radioGroup = page.locator('ion-radio-group'); + const radioOne = page.locator('ion-radio[value="1"]'); + const radioTwo = page.locator('ion-radio[value="2"]'); + + await radioGroup.evaluate((el: HTMLIonRadioGroupElement) => (el.value = '2')); + + await page.waitForChanges(); + + await expect(radioOne).not.toHaveClass(/radio-checked/); + await expect(radioTwo).toHaveClass(/radio-checked/); + }); + }); +}); + +class RadioFixture { + readonly page: E2EPage; + + private radio!: Locator; + + constructor(page: E2EPage) { + this.page = page; + } + + async checkRadio(method: 'keyboard' | 'mouse', selector = 'ion-radio') { + const { page } = this; + const radio = (this.radio = page.locator(selector)); + + if (method === 'keyboard') { + await radio.focus(); + await page.keyboard.press('Space'); + } else { + await radio.click(); + } + + await page.waitForChanges(); + + return radio; + } + + async expectChecked(state: boolean) { + const { radio } = this; + await expect(radio.locator('input')).toHaveJSProperty('checked', state); + } +} diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Chrome-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Chrome-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Chrome-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Chrome-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Firefox-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Firefox-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Firefox-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Firefox-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Safari-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Safari-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Safari-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-ltr-Mobile-Safari-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Chrome-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Chrome-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Chrome-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Chrome-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Firefox-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Firefox-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Firefox-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Firefox-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Safari-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Safari-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Safari-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-ios-rtl-Mobile-Safari-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Chrome-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Chrome-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Chrome-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Chrome-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Firefox-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Firefox-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Firefox-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Firefox-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Safari-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Safari-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Safari-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-ltr-Mobile-Safari-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Chrome-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Chrome-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Chrome-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Chrome-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Firefox-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Firefox-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Firefox-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Firefox-linux.png diff --git a/core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Safari-linux.png b/core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Safari-linux.png similarity index 100% rename from core/src/components/radio-group/test/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Safari-linux.png rename to core/src/components/radio-group/test/legacy/basic/radio-group.e2e.ts-snapshots/radio-group-diff-md-rtl-Mobile-Safari-linux.png diff --git a/core/src/components/radio-group/test/legacy/form/index.html b/core/src/components/radio-group/test/legacy/form/index.html new file mode 100644 index 0000000000..159f8c6249 --- /dev/null +++ b/core/src/components/radio-group/test/legacy/form/index.html @@ -0,0 +1,114 @@ + + + + + Radio Group - Form + + + + + + + + + + + + + Radio Group - Form + + + + +
+ + + + Luckiest Man On Earth + + + + Biff + + + + + + + Griff + + + + + + + Buford + + + + + + + George + + + + + Submit + + + +
+ +

+ Value: + +

+ +

+ Changes: + 0 +

+ + + + + + + diff --git a/core/src/components/radio-group/test/legacy/form/radio-group.e2e.ts b/core/src/components/radio-group/test/legacy/form/radio-group.e2e.ts new file mode 100644 index 0000000000..72e3811d1c --- /dev/null +++ b/core/src/components/radio-group/test/legacy/form/radio-group.e2e.ts @@ -0,0 +1,35 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('radio-group: form'), () => { + test.beforeEach(async ({ page }) => { + await page.goto('/src/components/radio-group/test/legacy/form', config); + }); + + test('selecting an option should update the value', async ({ page }) => { + const radioGroup = page.locator('ion-radio-group'); + const ionChange = await page.spyOnEvent('ionChange'); + const griffRadio = page.locator('ion-radio[value="griff"]'); + await expect(radioGroup).toHaveAttribute('value', 'biff'); + + await griffRadio.click(); + await page.waitForChanges(); + + await expect(ionChange).toHaveReceivedEventDetail({ value: 'griff', event: { isTrusted: true } }); + }); + + test('selecting a disabled option should not update the value', async ({ page }) => { + const value = page.locator('#value'); + const disabledRadio = page.locator('ion-radio[value="george"]'); + + await expect(value).toHaveText(''); + await expect(disabledRadio).toHaveAttribute('disabled', ''); + + await disabledRadio.click({ force: true }); + await page.waitForChanges(); + + await expect(value).toHaveText(''); + }); + }); +}); diff --git a/core/src/components/radio-group/test/legacy/search/index.html b/core/src/components/radio-group/test/legacy/search/index.html new file mode 100644 index 0000000000..fb0bb926f3 --- /dev/null +++ b/core/src/components/radio-group/test/legacy/search/index.html @@ -0,0 +1,82 @@ + + + + + Radio Group - Search + + + + + + + + + + + + + Radio Group - Form + + + + + + Current value: + + + + + + + + + + + + diff --git a/core/src/components/radio-group/test/legacy/search/radio-group.e2e.ts b/core/src/components/radio-group/test/legacy/search/radio-group.e2e.ts new file mode 100644 index 0000000000..ec40c75605 --- /dev/null +++ b/core/src/components/radio-group/test/legacy/search/radio-group.e2e.ts @@ -0,0 +1,42 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +/** + * This behavior does not var across modes/directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('radio-group'), () => { + test.beforeEach(async ({ page }) => { + await page.goto('/src/components/radio-group/test/legacy/search', config); + }); + + test.describe('radio-group: state', () => { + test('radio should remain checked after being removed/readded to the dom', async ({ page }) => { + const radioGroup = page.locator('ion-radio-group'); + const radio = page.locator('ion-radio[value=two]'); + const searchbarInput = page.locator('ion-searchbar input'); + + // select radio + await radio.click(); + await expect(radio.locator('input')).toHaveJSProperty('checked', true); + + // filter radio so it is not in DOM + await page.fill('ion-searchbar input', 'zero'); + await searchbarInput.evaluate((el) => el.blur()); + await page.waitForChanges(); + await expect(radio).toBeHidden(); + + // ensure radio group has the same value + await expect(radioGroup).toHaveJSProperty('value', 'two'); + + // clear the search so the radio appears + await page.fill('ion-searchbar input', ''); + await searchbarInput.evaluate((el) => el.blur()); + await page.waitForChanges(); + + // ensure that the new radio instance is still checked + await expect(radio.locator('input')).toHaveJSProperty('checked', true); + }); + }); + }); +}); diff --git a/core/src/components/radio-group/test/search/index.html b/core/src/components/radio-group/test/search/index.html index fb0bb926f3..825ac1c81c 100644 --- a/core/src/components/radio-group/test/search/index.html +++ b/core/src/components/radio-group/test/search/index.html @@ -65,8 +65,7 @@ if (d.value.includes(query)) { html += ` - Item ${d.value} - + Item ${d.value} `; } diff --git a/core/src/components/radio-group/test/search/radio-group.e2e.ts b/core/src/components/radio-group/test/search/radio-group.e2e.ts index ef9ac5828d..121acd4cd6 100644 --- a/core/src/components/radio-group/test/search/radio-group.e2e.ts +++ b/core/src/components/radio-group/test/search/radio-group.e2e.ts @@ -1,42 +1,41 @@ import { expect } from '@playwright/test'; import { configs, test } from '@utils/test/playwright'; +import { RadioFixture } from '../fixtures'; + /** * This behavior does not var across modes/directions. */ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { - test.describe(title('radio-group'), () => { - test.beforeEach(async ({ page }) => { + test.describe(title('radio-group: search'), () => { + test('radio should remain checked after being removed/readded to the dom', async ({ page }) => { await page.goto('/src/components/radio-group/test/search', config); - }); - test.describe('radio-group: state', () => { - test('radio should remain checked after being removed/readded to the dom', async ({ page }) => { - const radioGroup = page.locator('ion-radio-group'); - const radio = page.locator('ion-radio[value=two]'); - const searchbarInput = page.locator('ion-searchbar input'); + const radioFixture = new RadioFixture(page); - // select radio - await radio.click(); - await expect(radio.locator('input')).toHaveJSProperty('checked', true); + const radioGroup = page.locator('ion-radio-group'); + const searchbarInput = page.locator('ion-searchbar input'); - // filter radio so it is not in DOM - await page.fill('ion-searchbar input', 'zero'); - await searchbarInput.evaluate((el) => el.blur()); - await page.waitForChanges(); - await expect(radio).toBeHidden(); + // select radio + const radio = await radioFixture.checkRadio('mouse', 'ion-radio[value=two]'); + await radioFixture.expectChecked(true); - // ensure radio group has the same value - await expect(radioGroup).toHaveJSProperty('value', 'two'); + // filter radio so it is not in DOM + await page.fill('ion-searchbar input', 'zero'); + await searchbarInput.evaluate((el) => el.blur()); + await page.waitForChanges(); + await expect(radio).toBeHidden(); - // clear the search so the radio appears - await page.fill('ion-searchbar input', ''); - await searchbarInput.evaluate((el) => el.blur()); - await page.waitForChanges(); + // ensure radio group has the same value + await expect(radioGroup).toHaveJSProperty('value', 'two'); - // ensure that the new radio instance is still checked - await expect(radio.locator('input')).toHaveJSProperty('checked', true); - }); + // clear the search so the radio appears + await page.fill('ion-searchbar input', ''); + await searchbarInput.evaluate((el) => el.blur()); + await page.waitForChanges(); + + // ensure that the new radio instance is still checked + await radioFixture.expectChecked(true); }); }); }); diff --git a/core/src/components/radio/radio.tsx b/core/src/components/radio/radio.tsx index 14ae1f4d68..d6618dcffe 100644 --- a/core/src/components/radio/radio.tsx +++ b/core/src/components/radio/radio.tsx @@ -2,8 +2,7 @@ import type { ComponentInterface, EventEmitter } from '@stencil/core'; import { Component, Element, Event, Host, Method, Prop, State, Watch, h } from '@stencil/core'; import type { LegacyFormController } from '@utils/forms'; import { createLegacyFormController } from '@utils/forms'; -import type { Attributes } from '@utils/helpers'; -import { addEventListener, getAriaLabel, inheritAriaAttributes, removeEventListener } from '@utils/helpers'; +import { addEventListener, getAriaLabel, removeEventListener } from '@utils/helpers'; import { printIonWarning } from '@utils/logging'; import { createColorClasses, hostContext } from '@utils/theme'; @@ -31,7 +30,6 @@ export class Radio implements ComponentInterface { private radioGroup: HTMLIonRadioGroupElement | null = null; private nativeInput!: HTMLInputElement; private legacyFormController!: LegacyFormController; - private inheritedAttributes: Attributes = {}; // This flag ensures we log the deprecation warning at most once. private hasLoggedDeprecationWarning = false; @@ -135,8 +133,7 @@ export class Radio implements ComponentInterface { ev.stopPropagation(); ev.preventDefault(); - const element = this.legacyFormController.hasLegacyControl() ? this.el : this.nativeInput; - element.focus(); + this.el.focus(); } /** @internal */ @@ -167,12 +164,6 @@ export class Radio implements ComponentInterface { componentWillLoad() { this.emitStyle(); - - if (!this.legacyFormController.hasLegacyControl()) { - this.inheritedAttributes = { - ...inheritAriaAttributes(this.el), - }; - } } @Watch('checked') @@ -201,7 +192,34 @@ export class Radio implements ComponentInterface { }; private onClick = () => { - this.checked = this.nativeInput.checked; + const { radioGroup, checked } = this; + + /** + * The legacy control uses a native input inside + * of the radio host, so we can set this.checked + * to the state of the nativeInput. RadioGroup + * will prevent the native input from checking if + * allowEmptySelection="false" by calling ev.preventDefault(). + */ + if (this.legacyFormController.hasLegacyControl()) { + this.checked = this.nativeInput.checked; + return; + } + + /** + * The modern control does not use a native input + * inside of the radio host, so we cannot rely on the + * ev.preventDefault() behavior above. If the radio + * is checked and the parent radio group allows for empty + * selection, then we can set the checked state to false. + * Otherwise, the checked state should always be set + * to true because the checked state cannot be toggled. + */ + if (checked && radioGroup?.allowEmptySelection) { + this.checked = false; + } else { + this.checked = true; + } }; private onFocus = () => { @@ -232,23 +250,14 @@ export class Radio implements ComponentInterface { } private renderRadio() { - const { - checked, - disabled, - inputId, - color, - el, - justify, - labelPlacement, - inheritedAttributes, - hasLabel, - buttonTabindex, - } = this; + const { checked, disabled, color, el, justify, labelPlacement, hasLabel, buttonTabindex } = this; const mode = getIonMode(this); const inItem = hostContext('ion-item', el); return (