Prefer tag over ARIA role #2977
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces non-semantic ARIA-role wrappers with native semantic HTML elements and updates related test mocks and queries: pagination ellipsis, modal/dialog, dropdown section, MetricsScoreCircle clickable mock, and skeleton loading ARIA attributes. (48 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/__tests__/unit/components/Pagination.test.tsx(1 hunks)frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx(1 hunks)frontend/__tests__/unit/pages/ApiKeysPage.test.tsx(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(1 hunks)frontend/src/components/Pagination.tsx(1 hunks)frontend/src/components/skeletons/Card.tsx(2 hunks)frontend/src/components/skeletons/UserCard.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/pages/Home.test.tsxfrontend/__tests__/unit/pages/ApiKeysPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/Pagination.test.tsx
🔇 Additional comments (5)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
164-173: LGTM! Improved semantic HTML in test mock.The mock now correctly renders a native
buttonelement when the component is clickable, which improves accessibility by providing proper keyboard navigation and screen reader semantics. The fallback to adivfor non-clickable cases is appropriate.frontend/__tests__/unit/pages/Home.test.tsx (1)
46-54: LGTM! Proper use of native dialog element.The mock now uses the native
<dialog>element instead of a div withrole="dialog", which is the correct semantic HTML5 approach. Native dialogs provide built-in accessibility features including focus management and proper ARIA semantics.frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx (1)
78-83: LGTM! Correct semantic HTML for grouping.Using
<fieldset>with<legend>is the semantically correct way to group related form controls or menu items. This native HTML structure provides better accessibility than a div with ARIA attributes, as it's natively understood by all assistive technologies.frontend/__tests__/unit/pages/ApiKeysPage.test.tsx (1)
25-25: LGTM! Native dialog element in test mock.Correctly uses the native
<dialog>element with theopenattribute, providing better semantic HTML and built-in accessibility features in the test mock.frontend/__tests__/unit/components/Pagination.test.tsx (1)
89-93: Test now relies on implementation details.The test now queries ellipses by CSS class selectors (
div.flex.h-10.w-10) and DOM structure rather than accessible labels. This makes the test more brittle and couples it to implementation details (styling).This is a consequence of removing the
aria-labelfrom the ellipsis in the Pagination component. If the accessibility issue infrontend/src/components/Pagination.tsx(line 74-76) is addressed by restoring semantic labeling, this test should revert to querying by accessible role or label.Consider reverting to:
const ellipses = screen.getAllByRole('separator', { name: 'More pages' }) expect(ellipses).toHaveLength(2)once the component accessibility is restored.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Hi @anurag2787 !
Left a few comments under CodeRabbit comments (relevant). Not sure if they are visible as is since you've already resolved them. Sou you might need to unfold them to see the comments.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 could you run make check-test and address found issues?
anurag2787
force-pushed
the
fix-aria-role
branch
from
7d4e468 to
face234
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/Pagination.tsx (1)
74-79: Verify aria-label announcement on plain div without role.Good improvement removing the contradictory
role="presentation"(which would have negated thearia-label). However,aria-labelon a generic<div>without an ARIA role may not be reliably announced by all screen readers, as generic containers aren't guaranteed to be included in the accessibility tree.Consider these alternatives to ensure the label is consistently announced:
Option 1 (Recommended): Add
role="img"since the ellipsis serves as an iconic representation:<div className="flex h-10 w-10 items-center justify-center text-gray-600 dark:text-gray-400" role="img" aria-label="More pages" > <FaEllipsis className="h-5 w-5" aria-hidden="true" /> </div>Option 2: Use visually-hidden text instead of
aria-label:<div className="flex h-10 w-10 items-center justify-center text-gray-600 dark:text-gray-400"> <span className="sr-only">More pages</span> <FaEllipsis className="h-5 w-5" aria-hidden="true" /> </div>(Requires a
.sr-onlyutility class for screen-reader-only text)Note:
role="img"is appropriate here because the ellipsis functions as a visual indicator, similar to an icon, and shouldn't trigger S6819 issues since there's no native HTML element for "pagination ellipsis indicator."Would you like me to help implement either approach, or would you prefer to test the current implementation with actual screen readers first?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/Pagination.tsxfrontend/src/components/skeletons/Card.tsxfrontend/src/components/skeletons/UserCard.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/skeletons/UserCard.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Applied to files:
frontend/src/components/Pagination.tsxfrontend/src/components/skeletons/Card.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/Pagination.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/skeletons/Card.tsx
🔇 Additional comments (1)
frontend/src/components/skeletons/Card.tsx (1)
19-25: LGTM! Correct accessibility implementation for loading state.This implementation properly addresses the skeleton loading state with appropriate ARIA attributes:
role="status"is legitimate here (no semantic HTML alternative exists for loading indicators)aria-live="polite"ensures screen reader announcementsaria-busy="true"signals the loading statearia-label="Loading"provides clear contextIf SonarQube rule S6819 flags this, it's a false positive—the rule targets ARIA roles that have semantic HTML alternatives (e.g.,
role="button"→<button>), butrole="status"has no such alternative. You can suppress false positives with a// NOSONARcomment if needed.
Sorry about this. The issue was caused by my last commit and i will make sure from next time to run make check-test before asking for review |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 thanks for working on this 👌🏼
* Prefer tag over ARIA role * updated aria * Run check command --------- Co-authored-by: anurag2787 <anurag2787@gmail.com>
2 participants
Resolves #2540
Descripiton
Replaced ARIA role–based elements with native semantic HTML tags as suggested by rule typescript:S6819
Checklist
make check-testlocally and all tests passed