From 48780927122633f8ee84cb0548cd9cd968c45a29 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Fri, 2 Sep 2022 16:31:46 +0200 Subject: [PATCH] Improve accessibility when announcing `Listbox.Option` and `Combobox.Option` components (#1812) * ensure that `aria-selected` is explicitly set to `false` The WAI-ARIA Best Practices don't recommend this and prefer `aria-selected: true` or undefined (aka not existing when it is "false"). However in practice, both MacOS VoiceOver and NVDA experience strange issues if you don't do this (e.g.: everything before the selected item is also selected) * update tests to ensure we are checking for `aria-selected=false` * update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + .../src/components/combobox/combobox.test.tsx | 18 ++++++------- .../src/components/combobox/combobox.tsx | 2 +- .../src/components/listbox/listbox.test.tsx | 18 ++++++------- .../src/components/listbox/listbox.tsx | 2 +- .../test-utils/accessibility-assertions.ts | 26 +++---------------- packages/@headlessui-vue/CHANGELOG.md | 1 + .../src/components/combobox/combobox.test.ts | 18 ++++++------- .../src/components/combobox/combobox.ts | 2 +- .../src/components/listbox/listbox.test.tsx | 18 ++++++------- .../src/components/listbox/listbox.ts | 2 +- .../test-utils/accessibility-assertions.ts | 26 +++---------------- 12 files changed, 50 insertions(+), 84 deletions(-) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index 3f86739..5ca11a8 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Don't scroll when wrapping around in focus trap ([#1789](https://github.com/tailwindlabs/headlessui/pull/1789)) - Fix `Transition` component's incorrect cleanup and order of events ([#1803](https://github.com/tailwindlabs/headlessui/pull/1803)) - Ensure enter transitions work when using `unmount={false}` ([#1811](https://github.com/tailwindlabs/headlessui/pull/1811)) +- Improve accessibility when announcing `Listbox.Option` and `Combobox.Option` components ([#1812](https://github.com/tailwindlabs/headlessui/pull/1812)) ## [1.6.6] - 2022-07-07 diff --git a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx index 45f1f9a..e9d4f7f 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.test.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.test.tsx @@ -295,23 +295,23 @@ describe('Rendering', () => { await click(getComboboxButton()) let [alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') await click(getComboboxOptions()[2]) await click(getComboboxButton()) ;[alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') - expect(bob).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') + expect(bob).toHaveAttribute('aria-selected', 'false') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getComboboxOptions()[1]) await click(getComboboxButton()) ;[alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) @@ -339,15 +339,15 @@ describe('Rendering', () => { await click(getComboboxOptions()[2]) let [alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getComboboxOptions()[2]) ;[alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) }) diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx index 2ba11f2..525cea7 100644 --- a/packages/@headlessui-react/src/components/combobox/combobox.tsx +++ b/packages/@headlessui-react/src/components/combobox/combobox.tsx @@ -1155,7 +1155,7 @@ let Option = forwardRefWithAs(function Option< // According to the WAI-ARIA best practices, we should use aria-checked for // multi-select,but Voice-Over disagrees. So we use aria-checked instead for // both single and multi-select. - 'aria-selected': selected === true ? true : undefined, + 'aria-selected': selected, disabled: undefined, // Never forward the `disabled` prop onClick: handleClick, onFocus: handleFocus, diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index d12469e..0b6af6e 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx @@ -286,23 +286,23 @@ describe('Rendering', () => { await click(getListboxButton()) let [alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') await click(getListboxOptions()[2]) await click(getListboxButton()) ;[alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') - expect(bob).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') + expect(bob).toHaveAttribute('aria-selected', 'false') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getListboxOptions()[1]) await click(getListboxButton()) ;[alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) @@ -330,15 +330,15 @@ describe('Rendering', () => { await click(getListboxOptions()[2]) let [alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getListboxOptions()[2]) ;[alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) }) diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index cb0b38e..b8a5798 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/packages/@headlessui-react/src/components/listbox/listbox.tsx @@ -875,7 +875,7 @@ let Option = forwardRefWithAs(function Option< // According to the WAI-ARIA best practices, we should use aria-checked for // multi-select,but Voice-Over disagrees. So we use aria-checked instead for // both single and multi-select. - 'aria-selected': selected === true ? true : undefined, + 'aria-selected': selected, disabled: undefined, // Never forward the `disabled` prop onClick: handleClick, onFocus: handleFocus, diff --git a/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts b/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts index 37db4db..6ba9263 100644 --- a/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts +++ b/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts @@ -618,7 +618,7 @@ export function assertNoActiveComboboxOption(combobox = getComboboxInput()) { export function assertNoSelectedComboboxOption(items = getComboboxOptions()) { try { - for (let item of items) expect(item).not.toHaveAttribute('aria-selected') + for (let item of items) expect(item).toHaveAttribute('aria-selected', 'false') } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertNoSelectedComboboxOption) throw err @@ -656,16 +656,7 @@ export function assertComboboxOption( } if (options.selected != null) { - switch (options.selected) { - case true: - return expect(item).toHaveAttribute('aria-selected', 'true') - - case false: - return expect(item).not.toHaveAttribute('aria-selected') - - default: - assertNever(options.selected) - } + return expect(item).toHaveAttribute('aria-selected', options.selected ? 'true' : 'false') } } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertComboboxOption) @@ -948,7 +939,7 @@ export function assertNoActiveListboxOption(listbox = getListbox()) { export function assertNoSelectedListboxOption(items = getListboxOptions()) { try { - for (let item of items) expect(item).not.toHaveAttribute('aria-selected') + for (let item of items) expect(item).toHaveAttribute('aria-selected', 'false') } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertNoSelectedListboxOption) throw err @@ -986,16 +977,7 @@ export function assertListboxOption( } if (options.selected != null) { - switch (options.selected) { - case true: - return expect(item).toHaveAttribute('aria-selected', 'true') - - case false: - return expect(item).not.toHaveAttribute('aria-selected') - - default: - assertNever(options.selected) - } + return expect(item).toHaveAttribute('aria-selected', options.selected ? 'true' : 'false') } } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertListboxOption) diff --git a/packages/@headlessui-vue/CHANGELOG.md b/packages/@headlessui-vue/CHANGELOG.md index 937116a..42da3b2 100644 --- a/packages/@headlessui-vue/CHANGELOG.md +++ b/packages/@headlessui-vue/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Only select the active option when using "singular" mode when pressing `` in the `Combobox` component ([#1750](https://github.com/tailwindlabs/headlessui/pull/1750)) - Only restore focus to the `MenuButton` if necessary when activating a `MenuOption` ([#1782](https://github.com/tailwindlabs/headlessui/pull/1782)) - Don't scroll when wrapping around in focus trap ([#1789](https://github.com/tailwindlabs/headlessui/pull/1789)) +- Improve accessibility when announcing `ListboxOption` and `ComboboxOption` components ([#1812](https://github.com/tailwindlabs/headlessui/pull/1812)) ## [1.6.7] - 2022-07-12 diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts index 2c1585e..1c8b34c 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.test.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.test.ts @@ -331,23 +331,23 @@ describe('Rendering', () => { await click(getComboboxButton()) let [alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') await click(getComboboxOptions()[2]) await click(getComboboxButton()) ;[alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') - expect(bob).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') + expect(bob).toHaveAttribute('aria-selected', 'false') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getComboboxOptions()[1]) await click(getComboboxButton()) ;[alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) @@ -375,15 +375,15 @@ describe('Rendering', () => { await click(getComboboxOptions()[2]) let [alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getComboboxOptions()[2]) ;[alice, bob, charlie] = getComboboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) }) diff --git a/packages/@headlessui-vue/src/components/combobox/combobox.ts b/packages/@headlessui-vue/src/components/combobox/combobox.ts index 8cfa1ce..b228309 100644 --- a/packages/@headlessui-vue/src/components/combobox/combobox.ts +++ b/packages/@headlessui-vue/src/components/combobox/combobox.ts @@ -943,7 +943,7 @@ export let ComboboxOption = defineComponent({ // According to the WAI-ARIA best practices, we should use aria-checked for // multi-select,but Voice-Over disagrees. So we use aria-checked instead for // both single and multi-select. - 'aria-selected': selected.value === true ? selected.value : undefined, + 'aria-selected': selected.value, disabled: undefined, // Never forward the `disabled` prop onClick: handleClick, onFocus: handleFocus, diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx index 5db764f..ee6b062 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx +++ b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx @@ -311,23 +311,23 @@ describe('Rendering', () => { await click(getListboxButton()) let [alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') await click(getListboxOptions()[2]) await click(getListboxButton()) ;[alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') - expect(bob).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') + expect(bob).toHaveAttribute('aria-selected', 'false') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getListboxOptions()[1]) await click(getListboxButton()) ;[alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) @@ -355,15 +355,15 @@ describe('Rendering', () => { await click(getListboxOptions()[2]) let [alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') expect(charlie).toHaveAttribute('aria-selected', 'true') await click(getListboxOptions()[2]) ;[alice, bob, charlie] = getListboxOptions() - expect(alice).not.toHaveAttribute('aria-selected') + expect(alice).toHaveAttribute('aria-selected', 'false') expect(bob).toHaveAttribute('aria-selected', 'true') - expect(charlie).not.toHaveAttribute('aria-selected') + expect(charlie).toHaveAttribute('aria-selected', 'false') }) ) }) diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.ts b/packages/@headlessui-vue/src/components/listbox/listbox.ts index f3dc397..7a4e790 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.ts +++ b/packages/@headlessui-vue/src/components/listbox/listbox.ts @@ -757,7 +757,7 @@ export let ListboxOption = defineComponent({ // According to the WAI-ARIA best practices, we should use aria-checked for // multi-select,but Voice-Over disagrees. So we use aria-checked instead for // both single and multi-select. - 'aria-selected': selected.value === true ? selected.value : undefined, + 'aria-selected': selected.value, disabled: undefined, // Never forward the `disabled` prop onClick: handleClick, onFocus: handleFocus, diff --git a/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts b/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts index 37db4db..6ba9263 100644 --- a/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts +++ b/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts @@ -618,7 +618,7 @@ export function assertNoActiveComboboxOption(combobox = getComboboxInput()) { export function assertNoSelectedComboboxOption(items = getComboboxOptions()) { try { - for (let item of items) expect(item).not.toHaveAttribute('aria-selected') + for (let item of items) expect(item).toHaveAttribute('aria-selected', 'false') } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertNoSelectedComboboxOption) throw err @@ -656,16 +656,7 @@ export function assertComboboxOption( } if (options.selected != null) { - switch (options.selected) { - case true: - return expect(item).toHaveAttribute('aria-selected', 'true') - - case false: - return expect(item).not.toHaveAttribute('aria-selected') - - default: - assertNever(options.selected) - } + return expect(item).toHaveAttribute('aria-selected', options.selected ? 'true' : 'false') } } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertComboboxOption) @@ -948,7 +939,7 @@ export function assertNoActiveListboxOption(listbox = getListbox()) { export function assertNoSelectedListboxOption(items = getListboxOptions()) { try { - for (let item of items) expect(item).not.toHaveAttribute('aria-selected') + for (let item of items) expect(item).toHaveAttribute('aria-selected', 'false') } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertNoSelectedListboxOption) throw err @@ -986,16 +977,7 @@ export function assertListboxOption( } if (options.selected != null) { - switch (options.selected) { - case true: - return expect(item).toHaveAttribute('aria-selected', 'true') - - case false: - return expect(item).not.toHaveAttribute('aria-selected') - - default: - assertNever(options.selected) - } + return expect(item).toHaveAttribute('aria-selected', options.selected ? 'true' : 'false') } } catch (err) { if (err instanceof Error) Error.captureStackTrace(err, assertListboxOption)