Skip to content

Commit

Permalink
fix(radio): radios can be focused and are announced with group (#27817)
Browse files Browse the repository at this point in the history
Issue number: resolves #27438

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

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?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

Most of the changes are test-related, but I broke it down per commit to
make this easier to review:


ae77002

- Item no longer has `role="listitem"` when used inside of a radio
group.
- Added spec tests to verify the role behavior


0a9b7fb

- I discovered that some the legacy basic test were accidentally using
the modern syntax. I corrected this by adding `legacy="true"` to the
radios.


a8a90e5,
412d1d5,
and
1d1179b

- 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).


e2c966e
- 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


f10eff4

- I cleaned up the tests here to use a common radio fixture

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

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: ✅
  • Loading branch information
liamdebeasi authored Jul 31, 2023
1 parent a08a589 commit ba2f49b
Show file tree
Hide file tree
Showing 29 changed files with 669 additions and 159 deletions.
2 changes: 1 addition & 1 deletion core/src/components/item/item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<Host
Expand Down
51 changes: 51 additions & 0 deletions core/src/components/item/test/a11y/item.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { Radio } from '../../../radio/radio.tsx';
import { RadioGroup } from '../../../radio-group/radio-group.tsx';
import { Item } from '../../item.tsx';
import { List } from '../../../list/list.tsx';
import { newSpecPage } from '@stencil/core/testing';

describe('ion-item', () => {
it('should not have a role when used without list', async () => {
const page = await newSpecPage({
components: [Item],
html: `<ion-item>Hello World</ion-item>`,
});

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: `
<ion-list>
<ion-item>
Hello World
</ion-item>
</ion-list>
`,
});

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: `
<ion-list>
<ion-radio-group value="a">
<ion-item>
<ion-radio value="other-value" aria-label="my radio"></ion-radio>
</ion-item>
</ion-radio-group>
</ion-list>
`,
});

const item = page.body.querySelector('ion-item');
expect(item.getAttribute('role')).toBe(null);
});
});
12 changes: 4 additions & 8 deletions core/src/components/radio-group/test/basic/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,19 @@
</ion-list-header>

<ion-item>
<ion-label>Item 1</ion-label>
<ion-radio value="1" slot="start"></ion-radio>
<ion-radio value="1">Item 1</ion-radio>
</ion-item>

<ion-item>
<ion-label>Item 2</ion-label>
<ion-radio value="2" slot="start"></ion-radio>
<ion-radio value="2">Item 2</ion-radio>
</ion-item>

<ion-item>
<ion-label>Item 3</ion-label>
<ion-radio value="3" slot="start"></ion-radio>
<ion-radio value="3">Item 3</ion-radio>
</ion-item>

<ion-item>
<ion-label>Item 4</ion-label>
<ion-radio value="4" slot="start"></ion-radio>
<ion-radio value="4">Item 4</ion-radio>
</ion-item>
</ion-radio-group>
</ion-list>
Expand Down
66 changes: 8 additions & 58 deletions core/src/components/radio-group/test/basic/radio-group.e2e.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -31,8 +19,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
`
<ion-radio-group value="one" allow-empty-selection="false">
<ion-item>
<ion-label>One</ion-label>
<ion-radio id="one" value="one"></ion-radio>
<ion-radio id="one" value="one">One</ion-radio>
</ion-item>
</ion-radio-group>
`,
Expand All @@ -48,8 +35,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
`
<ion-radio-group value="one" allow-empty-selection="true">
<ion-item>
<ion-label>One</ion-label>
<ion-radio id="one" value="one"></ion-radio>
<ion-radio id="one" value="one">One</ion-radio>
</ion-item>
</ion-radio-group>
`,
Expand All @@ -65,8 +51,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
`
<ion-radio-group value="one" allow-empty-selection="false">
<ion-item>
<ion-label>One</ion-label>
<ion-radio id="one" value="one"></ion-radio>
<ion-radio id="one" value="one">One</ion-radio>
</ion-item>
</ion-radio-group>
`,
Expand All @@ -82,8 +67,7 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
`
<ion-radio-group value="one" allow-empty-selection="true">
<ion-item>
<ion-label>One</ion-label>
<ion-radio id="one" value="one"></ion-radio>
<ion-radio id="one" value="one">One</ion-radio>
</ion-item>
</ion-radio-group>
`,
Expand All @@ -99,18 +83,15 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) =>
`
<ion-radio-group value="1">
<ion-item>
<ion-label>Item 1</ion-label>
<ion-radio value="1" slot="start"></ion-radio>
<ion-radio value="1">Item 1</ion-radio>
</ion-item>
<ion-item>
<ion-label>Item 2</ion-label>
<ion-radio value="2" slot="start"></ion-radio>
<ion-radio value="2">Item 2</ion-radio>
</ion-item>
<ion-item>
<ion-label>Item 3</ion-label>
<ion-radio value="3" slot="start"></ion-radio>
<ion-radio value="3">Item 3</ion-radio>
</ion-item>
</ion-radio-group>
`,
Expand All @@ -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);
}
}
39 changes: 39 additions & 0 deletions core/src/components/radio-group/test/fixtures.ts
Original file line number Diff line number Diff line change
@@ -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/);
}
}
}
21 changes: 4 additions & 17 deletions core/src/components/radio-group/test/form/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,32 +31,19 @@
</ion-list-header>

<ion-item>
<ion-label
>Biff
<span id="biff"></span>
</ion-label>
<ion-radio value="biff" slot="start"></ion-radio>
<ion-radio value="biff">Biff</ion-radio>
</ion-item>

<ion-item>
<ion-label
>Griff
<span id="griff"></span>
</ion-label>
<ion-radio value="griff" slot="start"></ion-radio>
<ion-radio value="griff">Griff</ion-radio>
</ion-item>

<ion-item>
<ion-label
>Buford
<span id="buford"></span>
</ion-label>
<ion-radio value="buford" slot="start"></ion-radio>
<ion-radio value="buford">Buford</ion-radio>
</ion-item>

<ion-item>
<ion-label>George</ion-label>
<ion-radio value="george" disabled slot="start"></ion-radio>
<ion-radio value="george" disabled>George</ion-radio>
</ion-item>

<ion-item>
Expand Down
56 changes: 56 additions & 0 deletions core/src/components/radio-group/test/legacy/basic/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
<meta charset="UTF-8" />
<title>Radio Group - Basic</title>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no"
/>
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" />
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" />
<script src="../../../../../scripts/testing/scripts.js"></script>
<script nomodule src="../../../../../dist/ionic/ionic.js"></script>
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
</head>

<body>
<ion-app>
<ion-header>
<ion-toolbar>
<ion-title>Radio Group - Basic</ion-title>
</ion-toolbar>
</ion-header>

<ion-content>
<ion-list>
<ion-radio-group name="items" id="group" value="1">
<ion-list-header>
<ion-label>Radio Group Header</ion-label>
</ion-list-header>

<ion-item>
<ion-label>Item 1</ion-label>
<ion-radio value="1" slot="start"></ion-radio>
</ion-item>

<ion-item>
<ion-label>Item 2</ion-label>
<ion-radio value="2" slot="start"></ion-radio>
</ion-item>

<ion-item>
<ion-label>Item 3</ion-label>
<ion-radio value="3" slot="start"></ion-radio>
</ion-item>

<ion-item>
<ion-label>Item 4</ion-label>
<ion-radio value="4" slot="start"></ion-radio>
</ion-item>
</ion-radio-group>
</ion-list>
</ion-content>
</ion-app>
</body>
</html>
Loading

0 comments on commit ba2f49b

Please sign in to comment.