Skip to content

fix(radio): [radio] fix using aria-hidden="true" on focusable elements #2265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kagol merged 2 commits into from
Oct 16, 2024
Merged

fix(radio): [radio] fix using aria-hidden="true" on focusable elements #2265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04557da
fix(radio): [radio] fix using aria-hidden="true" on focusable elements
zzcr Oct 15, 2024
f66850a
fix: fix e2e error
zzcr Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/sites/demos/pc/app/radio/default-slot.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ test('默认插槽', async ({ page }) => {
await page.goto('radio#default-slot')

const demo = page.locator('#default-slot')
const radio1 = demo.getByRole('radio', { name: '内容一：选项描述' })
const radio2 = demo.getByRole('radio', { name: '内容二：选项描述' })
const radio1 = demo.locator('label').filter({ hasText: '内容一：选项描述' })
const radio2 = demo.locator('label').filter({ hasText: '内容二：选项描述' })
Comment on lines +8 to +9
Copy link

@coderabbitai coderabbitai bot Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider the trade-offs of the new selector approach

The change from using getByRole (assumed from the PR context) to locator().filter() for selecting radio buttons has both advantages and potential drawbacks:

Advantages:

  1. This approach is less dependent on ARIA roles, which aligns with the PR's objective of fixing issues related to aria-hidden="true" on focusable elements.
  2. It provides more specific selection of elements, potentially reducing false positives in tests.

Potential drawbacks:

  1. The test might become more brittle if the exact text of the labels changes frequently, as it now depends on the specific content of the labels.
  2. This approach might be less accessible-friendly as it doesn't rely on ARIA roles, which are important for accessibility testing.

Consider the following suggestions:

  1. If possible, use data attributes for test selection to make the tests more robust and less dependent on text content.
  2. Document the reason for this change in a comment to provide context for future maintainers.
  3. Ensure that this change doesn't negatively impact the accessibility testing capabilities of your test suite.

Here's a potential improvement using data attributes:

const radio1 = demo.locator('[data-testid="radio-option-1"]')
const radio2 = demo.locator('[data-testid="radio-option-2"]')

This approach would require adding data-testid attributes to your radio button elements in the component, but it would make the tests more robust and easier to maintain.

💡 Codebase verification

Issues Found: aria-hidden="true" is still present in the radio component files

  • packages/vue/src/radio/src/mobile-first.vue
  • packages/vue/src/radio/src/mobile.vue

These elements remain hidden from assistive technologies despite the PR's objective to address aria-hidden="true" usage on focusable elements.

🔗 Analysis chain

Please provide more context on the relationship to the PR objective

While these changes appear to be related to the PR's objective of fixing issues with aria-hidden="true" on focusable elements, the connection is not immediately clear from this file alone. Could you please provide more context on how these test changes relate to the accessibility improvements mentioned in the PR title?

This additional information would help reviewers better understand the full scope and impact of the changes across the codebase.

To gain more context, let's examine the changes in the radio component:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for changes in the radio component related to aria-hidden

# Test: Search for changes related to aria-hidden in the radio component
rg --type vue 'aria-hidden' packages/vue/src/radio

Length of output: 78

Script:

#!/bin/bash
# Description: Re-attempt to search for changes related to aria-hidden in the radio component within .vue files.

# Search for 'aria-hidden' in .vue files within packages/vue/src/radio
rg 'aria-hidden' packages/vue/src/radio --glob '*.vue'

Length of output: 190

await expect(radio1).toBeVisible()
await expect(radio2).toBeVisible()
})
11 changes: 6 additions & 5 deletions examples/sites/demos/pc/app/radio/group-options.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import { test, expect } from '@playwright/test'
test('配置式单选组', async ({ page }) => {
page.on('pageerror', (exception) => expect(exception).toBeNull())
await page.goto('radio#group-options')
const radio1Label = page.locator('.tiny-radio__label').nth(0)
const radio2Label = page.locator('.tiny-radio__label').nth(1)
const radio1 = page.locator('.tiny-radio').nth(0)
const radio2 = page.locator('.tiny-radio').nth(1)
const demo = page.locator('#group-options')
const radio1Label = demo.locator('.tiny-radio__label').nth(0)
const radio2Label = demo.locator('.tiny-radio__label').nth(1)
const radio1 = demo.locator('.tiny-radio').nth(0)
const radio2 = demo.locator('.tiny-radio').nth(1)
await expect(radio1).toHaveClass('tiny-radio is-checked')
await expect(radio1Label).toHaveText('很好')
await expect(radio2Label).toHaveText('一般')
await page.getByRole('radio', { name: '一般' }).click()
await demo.locator('label').filter({ hasText: '一般' }).click()
await expect(radio2).toHaveClass('tiny-radio is-focus is-checked')
})
5 changes: 3 additions & 2 deletions examples/sites/demos/pc/app/radio/radio-events.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ test('单选框事件', async ({ page }) => {
await page.goto('radio#radio-events')

const demo = page.locator('#radio-events')
await demo.getByRole('radio', { name: '选项二' }).click()

await demo.locator('label').filter({ hasText: '选项二' }).click()
const radio1 = page.locator('div').filter({ hasText: 'change 事件，选中的 Radio label 值为：2' }).nth(1)
await expect(radio1).toBeVisible()

await demo.getByRole('radio', { name: '月度' }).first().click()
await demo.locator('label').filter({ hasText: '月度' }).first().click()
const radio2 = page.locator('div').filter({ hasText: 'change 事件，选中的 Radio label 值为：2' }).nth(1)
await expect(radio2).toBeVisible()
})
1 change: 0 additions & 1 deletion packages/vue/src/radio/src/pc.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
class="tiny-radio__original"
:value="label"
type="radio"
aria-hidden="true"
v-model="state.model"
@focus="state.focus = true"
@blur="state.focus = false"
Expand Down
Loading