fix(radio): [radio] fix using aria-hidden="true" on focusable elements #2265
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/radio/radio-events.spec.ts (1)
13-13: Consistent improvement, but consider removing.first()The change here is consistent with the previous one, which is good for maintainability. However, the use of
.first()raises a potential concern:
- It suggests there might be multiple elements matching the filter.
- Relying on the order of elements can make tests less reliable if the UI structure changes.
Consider making the selector more specific to avoid needing
.first(). For example, you could include a parent element's selector if these radio buttons are in different sections.If possible, try to make the selector unique without relying on
.first(). For example:await demo.locator('specific-parent-selector label').filter({ hasText: '月度' }).click()This would make the test more robust against potential UI changes.
examples/sites/demos/pc/app/radio/group-options.spec.ts (1)
7-10: Refined radio button and label locators.The updates to use the
demolocator for selecting radio buttons and labels are consistent and improve test specificity. This change maintains existing functionality while reducing the risk of selecting unintended elements.Consider using more specific selectors (e.g., data-testid attributes) instead of relying on element order with
nth(). This could make the tests more resilient to changes in the component's structure.Example:
const radio1 = demo.locator('[data-testid="radio-1"]') const radio2 = demo.locator('[data-testid="radio-2"]')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/sites/demos/pc/app/radio/default-slot.spec.ts (1 hunks)
- examples/sites/demos/pc/app/radio/group-options.spec.ts (1 hunks)
- examples/sites/demos/pc/app/radio/radio-events.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
examples/sites/demos/pc/app/radio/radio-events.spec.ts (1)
8-9: Improved radio button selection methodThe change from
getByRoletolocator('label').filter({ hasText: '选项二' })is a good improvement. This new approach:
- Is more specific, reducing the risk of selecting the wrong element.
- Better mimics user behavior by targeting the label, which is what users typically click.
- Is less dependent on the exact role implementation, making the test more robust.
examples/sites/demos/pc/app/radio/group-options.spec.ts (3)
6-6: Improved test specificity with demo locator.The introduction of the
demolocator targeting#group-optionsis a good practice. It enhances the test's robustness by scoping subsequent actions to a specific demo area, reducing the risk of interacting with unintended elements if the page structure changes.
14-14: Improved radio button selection method.The updated method for clicking the radio button labeled '一般' is a significant improvement. Using
demo.locator('label').filter({ hasText: '一般' }).click()is more robust and less prone to breaking if the order of elements changes. This approach directly targets the desired element based on its text content, which is closer to how a user would interact with the interface.
6-14: Overall improvement in test reliability and maintainability.The changes made to this test file consistently enhance its specificity and robustness. By introducing the
demolocator and refining the selection methods for radio buttons and labels, the test becomes more resilient to potential changes in the page structure. These improvements align well with best practices in test automation, making the test more reliable and easier to maintain in the long run.
| const radio1 = demo.locator('label').filter({ hasText: '内容一：选项描述' }) | ||
| const radio2 = demo.locator('label').filter({ hasText: '内容二：选项描述' }) |
There was a problem hiding this comment.
🛠️ 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:
- 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. - It provides more specific selection of elements, potentially reducing false positives in tests.
Potential drawbacks:
- 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.
- 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:
- If possible, use data attributes for test selection to make the tests more robust and less dependent on text content.
- Document the reason for this change in a comment to provide context for future maintainers.
- 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.vuepackages/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
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
aria-hidden="true"from the radio input, enhancing accessibility for assistive technologies.