a11y(tools): replace div+onClick with semantic button elements (#5223)#5350
Open
armorbreak001 wants to merge 1 commit intoasyncapi:masterfrom
Open
a11y(tools): replace div+onClick with semantic button elements (#5223)#5350armorbreak001 wants to merge 1 commit intoasyncapi:masterfrom
armorbreak001 wants to merge 1 commit intoasyncapi:masterfrom
Conversation
…api#5223) Problem: Filter and Jump to Category controls on /tools page are implemented as clickable div elements instead of semantic buttons. This causes: - Inconsistent keyboard focus/activation behavior - Screen reader announcement issues - Invalid markup (button wrapping interactive children) Changes: - ToolsDashboard.tsx: - Filter trigger: <div onClick> → <button type='button'> - Filter popup wrapper: <button> → <div role='menu'> (non-interactive) - Jump to Category: <div onClick> → <button type='button'> - Inner text wrappers: <div> → <span> - Filters.tsx: - Undo Changes: <div onClick> → <button type='button'> All changes preserve existing styling and behavior while adding proper keyboard accessibility and ARIA semantics.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
We require all PRs to follow Conventional Commits specification. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Contributor
📝 WalkthroughWalkthroughThe changes replace non-semantic clickable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5350 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 830 830
Branches 159 159
=========================================
Hits 830 830 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5350--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/tools/ToolsDashboard.tsx (1)
233-237:⚠️ Potential issue | 🟠 MajorUse a semantic button for Clear Filters too.
This remains a clickable
div, so keyboard users cannot focus or activate it when filters are active.♿ Proposed fix
{isFiltered && ( - <div className='mt-4 flex cursor-pointer items-center text-gray-600 hover:text-black' onClick={clearFilters}> + <button + type='button' + className='mt-4 flex cursor-pointer items-center bg-transparent p-0 text-gray-600 hover:text-black' + onClick={clearFilters} + > <Cross /> <span className='ml-3'>Clear Filters</span> - </div> + </button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tools/ToolsDashboard.tsx` around lines 233 - 237, Replace the clickable div used when isFiltered is true with a semantic <button> element (keeping the same visual classes and the Cross component) so keyboard users can focus and activate it; ensure the button has type="button", retains the onClick handler clearFilters, and include an accessible name (either visible text "Clear Filters" or aria-label) so screen readers can announce its purpose. Reference the isFiltered conditional and clearFilters handler and update the element that currently renders the Cross icon to be a button to provide native keyboard and accessibility behavior.components/tools/Filters.tsx (1)
147-163:⚠️ Potential issue | 🟠 MajorFinish replacing the remaining clickable
<div>controls in this panel.Keyboard users can open the filter panel, but the pricing chips, dropdown headers, and Apply Filters wrapper still depend on
div onClick. Convert them to semantic buttons and addaria-pressed/aria-expandedwhere stateful.♿ Example direction
- <div + <button + type='button' className={twMerge( `bg-gray-200 px-4 py-2 flex gap-1 rounded-md hover:bg-secondary-100 border hover:border-secondary-500 cursor-pointer ${checkPaid === 'free' ? 'bg-secondary-100 border-secondary-500' : ''}` )} + aria-pressed={checkPaid === 'free'} onClick={() => (checkPaid === 'free' ? setCheckPaid('all') : setCheckPaid('free'))} > - <div className='text-sm'>Open Source</div> + <span className='text-sm'>Open Source</span> <img src='/img/illustrations/icons/FreeIcon.svg' alt='Free' /> - </div> + </button>- <div + <button + type='button' className={twMerge( `px-4 py-2 flex justify-between rounded-lg border border-gray-400 w-full bg-gray-200 text-gray-700 shadow text-sm cursor-pointer ${openedFiltersDropown === OpenedFiltersDropdownType.LANGUAGE ? 'rounded-b-none' : ''}` )} + aria-expanded={openedFiltersDropown === OpenedFiltersDropdownType.LANGUAGE} onClick={() => toggleDropdown(OpenedFiltersDropdownType.LANGUAGE)} >Also applies to: 195-200, 237-242, 279-284, 310-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/tools/Filters.tsx` around lines 147 - 163, Replace the interactive <div>s used for the pricing chips and other filter controls with semantic <button type="button"> elements (e.g., the Open Source/Commercial chips that read checkPaid and call setCheckPaid) while preserving existing className, click handlers, and data-testid ('Applied-filters'); add aria-pressed to toggle buttons that reflect checkPaid state and aria-expanded to any dropdown header buttons that open/close panels, and ensure keyboard behavior is preserved (focusable, no implicit submit). Also convert the Apply Filters wrapper to a button with appropriate aria attributes and role if it opens a panel, keeping visual styles and onClick handlers unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/tools/ToolsDashboard.tsx`:
- Around line 179-191: The container for the filter panel in ToolsDashboard
currently uses role='menu' which is incorrect because Filters renders form
controls, so remove role='menu' from the panel and instead connect the trigger
button and panel with accessible state: add aria-expanded={openFilter} to the
Filter button (the onClick that toggles openFilter) and give the panel a stable
id (e.g., filters-panel) then add aria-controls="filters-panel" on the button;
keep the panel as a plain div (no menu/menuitem roles) and ensure Filters
receives setOpenFilter so closing logic remains intact.
- Around line 197-207: The button toggle for the category dropdown currently
uses openCategory and setopenCategory but does not expose state to assistive
tech; update the button element rendered in ToolsDashboard (the toggle that
calls setopenCategory and uses openCategory) to include aria-expanded tied to
openCategory (aria-expanded={openCategory}) and add aria-controls pointing to
the popup container's id, and give the popup div a matching unique id and an
appropriate landmark/role (e.g., role="menu" or role="region") so screen readers
can associate the button with the popup; ensure the popup div that renders when
openCategory is true (the div with className 'absolute right-52 top-16 z-20')
receives that id.
---
Outside diff comments:
In `@components/tools/Filters.tsx`:
- Around line 147-163: Replace the interactive <div>s used for the pricing chips
and other filter controls with semantic <button type="button"> elements (e.g.,
the Open Source/Commercial chips that read checkPaid and call setCheckPaid)
while preserving existing className, click handlers, and data-testid
('Applied-filters'); add aria-pressed to toggle buttons that reflect checkPaid
state and aria-expanded to any dropdown header buttons that open/close panels,
and ensure keyboard behavior is preserved (focusable, no implicit submit). Also
convert the Apply Filters wrapper to a button with appropriate aria attributes
and role if it opens a panel, keeping visual styles and onClick handlers
unchanged.
In `@components/tools/ToolsDashboard.tsx`:
- Around line 233-237: Replace the clickable div used when isFiltered is true
with a semantic <button> element (keeping the same visual classes and the Cross
component) so keyboard users can focus and activate it; ensure the button has
type="button", retains the onClick handler clearFilters, and include an
accessible name (either visible text "Clear Filters" or aria-label) so screen
readers can announce its purpose. Reference the isFiltered conditional and
clearFilters handler and update the element that currently renders the Cross
icon to be a button to provide native keyboard and accessibility behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71cdf94f-6ee8-4037-8f63-96ed70392ff9
📒 Files selected for processing (2)
components/tools/Filters.tsxcomponents/tools/ToolsDashboard.tsx
Comment on lines
+179
to
+191
| <button | ||
| type='button' | ||
| className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md' | ||
| onClick={() => setOpenFilter(!openFilter)} | ||
| data-testid='ToolsDashboard-Filters-Click' | ||
| > | ||
| <FilterIcon /> | ||
| <div>Filter</div> | ||
| </div> | ||
| <span>Filter</span> | ||
| </button> | ||
| {openFilter && ( | ||
| <button className='absolute top-16 z-20 min-w-[20rem]'> | ||
| <div className='absolute top-16 z-20 min-w-[20rem]' role='menu'> | ||
| <Filters setOpenFilter={setOpenFilter} /> | ||
| </button> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Don’t expose the filter panel as an ARIA menu.
Line 189 adds role='menu', but Filters renders form-like controls rather than menuitem descendants. Use a plain container and wire it to the trigger with expanded/control state.
♿ Proposed fix
<button
type='button'
+ aria-expanded={openFilter}
+ aria-controls='tools-filter-panel'
className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md'
onClick={() => setOpenFilter(!openFilter)}
data-testid='ToolsDashboard-Filters-Click'
>
@@
</button>
{openFilter && (
- <div className='absolute top-16 z-20 min-w-[20rem]' role='menu'>
+ <div id='tools-filter-panel' className='absolute top-16 z-20 min-w-[20rem]'>
<Filters setOpenFilter={setOpenFilter} />
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| type='button' | |
| className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md' | |
| onClick={() => setOpenFilter(!openFilter)} | |
| data-testid='ToolsDashboard-Filters-Click' | |
| > | |
| <FilterIcon /> | |
| <div>Filter</div> | |
| </div> | |
| <span>Filter</span> | |
| </button> | |
| {openFilter && ( | |
| <button className='absolute top-16 z-20 min-w-[20rem]'> | |
| <div className='absolute top-16 z-20 min-w-[20rem]' role='menu'> | |
| <Filters setOpenFilter={setOpenFilter} /> | |
| </button> | |
| </div> | |
| <button | |
| type='button' | |
| aria-expanded={openFilter} | |
| aria-controls='tools-filter-panel' | |
| className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md' | |
| onClick={() => setOpenFilter(!openFilter)} | |
| data-testid='ToolsDashboard-Filters-Click' | |
| > | |
| <FilterIcon /> | |
| <span>Filter</span> | |
| </button> | |
| {openFilter && ( | |
| <div id='tools-filter-panel' className='absolute top-16 z-20 min-w-[20rem]'> | |
| <Filters setOpenFilter={setOpenFilter} /> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/tools/ToolsDashboard.tsx` around lines 179 - 191, The container
for the filter panel in ToolsDashboard currently uses role='menu' which is
incorrect because Filters renders form controls, so remove role='menu' from the
panel and instead connect the trigger button and panel with accessible state:
add aria-expanded={openFilter} to the Filter button (the onClick that toggles
openFilter) and give the panel a stable id (e.g., filters-panel) then add
aria-controls="filters-panel" on the button; keep the panel as a plain div (no
menu/menuitem roles) and ensure Filters receives setOpenFilter so closing logic
remains intact.
Comment on lines
+197
to
207
| <button | ||
| type='button' | ||
| className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md' | ||
| onClick={() => setopenCategory(!openCategory)} | ||
| data-testid='ToolsDashboard-category' | ||
| > | ||
| <div>Jump to Category</div> | ||
| <span>Jump to Category</span> | ||
| <ArrowDown className={`my-auto ${openCategory ? 'rotate-180' : ''}`} /> | ||
| </div> | ||
| </button> | ||
| {openCategory && ( | ||
| <div className='absolute right-52 top-16 z-20'> |
Contributor
There was a problem hiding this comment.
Expose the Jump to Category expanded state.
The trigger is now keyboard-operable, but screen readers still won’t know whether the dropdown is open. Add aria-expanded and connect it to the popup.
♿ Proposed fix
<button
type='button'
+ aria-expanded={openCategory}
+ aria-controls='tools-category-dropdown'
className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md'
onClick={() => setopenCategory(!openCategory)}
data-testid='ToolsDashboard-category'
>
@@
</button>
{openCategory && (
- <div className='absolute right-52 top-16 z-20'>
+ <div id='tools-category-dropdown' className='absolute right-52 top-16 z-20'>
<CategoryDropdown setopenCategory={setopenCategory} />
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button | |
| type='button' | |
| className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md' | |
| onClick={() => setopenCategory(!openCategory)} | |
| data-testid='ToolsDashboard-category' | |
| > | |
| <div>Jump to Category</div> | |
| <span>Jump to Category</span> | |
| <ArrowDown className={`my-auto ${openCategory ? 'rotate-180' : ''}`} /> | |
| </div> | |
| </button> | |
| {openCategory && ( | |
| <div className='absolute right-52 top-16 z-20'> | |
| <button | |
| type='button' | |
| aria-expanded={openCategory} | |
| aria-controls='tools-category-dropdown' | |
| className='flex h-14 w-full cursor-pointer items-center justify-center gap-2 rounded-lg border border-gray-300 px-4 py-1 text-sm text-gray-700 shadow hover:border-gray-600 hover:shadow-md' | |
| onClick={() => setopenCategory(!openCategory)} | |
| data-testid='ToolsDashboard-category' | |
| > | |
| <span>Jump to Category</span> | |
| <ArrowDown className={`my-auto ${openCategory ? 'rotate-180' : ''}`} /> | |
| </button> | |
| {openCategory && ( | |
| <div id='tools-category-dropdown' className='absolute right-52 top-16 z-20'> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/tools/ToolsDashboard.tsx` around lines 197 - 207, The button
toggle for the category dropdown currently uses openCategory and setopenCategory
but does not expose state to assistive tech; update the button element rendered
in ToolsDashboard (the toggle that calls setopenCategory and uses openCategory)
to include aria-expanded tied to openCategory (aria-expanded={openCategory}) and
add aria-controls pointing to the popup container's id, and give the popup div a
matching unique id and an appropriate landmark/role (e.g., role="menu" or
role="region") so screen readers can associate the button with the popup; ensure
the popup div that renders when openCategory is true (the div with className
'absolute right-52 top-16 z-20') receives that id.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #5223
The
/toolspage Filter and Jump to Category controls use<div onClick>instead of<button>, causing:<button>containing other interactive elementsChanges
components/tools/ToolsDashboard.tsx<div className=... onClick={...}><button type=button ...><button><Filters/></button><div role=menu>(non-interactive)<div className=... onClick={...}><button type=button ...><div>Filter</div><span>Filter</span>components/tools/Filters.tsx<div className=... onClick={undoChanges}><button type=button ...>Accessibility Impact
Summary by CodeRabbit