-
-
Notifications
You must be signed in to change notification settings - Fork 224
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.
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
fix: a11y rule "interactive-supports-focus" fails with a warning #4159
base: beta
Are you sure you want to change the base?
Conversation
…supports-focus" warning The changes include adding tabIndex={0} to elements with role="tab" to ensure they are focusable for keyboard navigation, addressing the accessibility warning (jsx-a11y/interactive-supports-focus). Additionally, an onKeyDown handler was introduced to allow keyboard interaction, enabling users to select tabs using the Enter or Space keys. These changes improve both accessibility and user experience by making the tabs keyboard-accessible. fixes open-sauced#4138
👷 Deploy request for oss-insights pending review.Visit the deploys page to approve it
|
✅ Deploy Preview for design-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Co-authored-by: Nick Taylor <[email protected]>
Co-authored-by: Nick Taylor <[email protected]>
Hi @Bashamega, just seeing if you're blocked on anything. No rush on this, just checking in. |
I don't quite understand the required changes |
I'm just looking at the PR again, and it looks like the additional feedback I made wasn't saved. I'll post it shortly. |
key={index} | ||
className={`tool-list-item border-b-2 transition-all ease-in-out ${ | ||
className={clsx( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing the pull request, this really is just links in a navigation element that look like tabs, but they really are just. links.
The role="tablist"
can be removed from the <nav />
and the role="tab"
on the <div />
can be removed.
@@ -77,6 +77,11 @@ export const SubTabsList = ({ tabList, pageId, selectedTab, label, textSize, onS | |||
key={index} | |||
className={clsx(isSelected && "bg-white shadow", "rounded py-1 px-2", !isSelected && "text-light-slate-11")} | |||
onClick={onSelect ? () => onSelect(tab) : undefined} | |||
onKeyDown={(e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the SubTabsList, it's not a tab list if there is no onSelect
, it's just links. But if the onSelect
is present, it means it is a tab list. We can rework the SubTabsList component to this.
export const SubTabsList = ({ tabList, pageId, selectedTab, label, textSize, onSelect }: SubTabsListProps) => {
const ContainerComponent = onSelect ? "div" : "nav";
const extraProps = onSelect ? { role: "tablist" } : {};
return (
<ContainerComponent
{...extraProps}
aria-label={label}
className={clsx(
"flex items-center w-max overflow-x-auto overflow-y-hidden gap-2 bg-light-slate-3 p-1 rounded",
textSize === "small" && "text-sm"
)}
>
{tabList.map((tab, index) => {
const isSelected = selectedTab === tab.name.toLowerCase();
return (
<div
key={index}
className={clsx(isSelected && "bg-white shadow", "rounded py-1 px-2", !isSelected && "text-light-slate-11")}
>
{onSelect ? (
<button role="tab" onClick={() => onSelect(tab)}>
tab.name
</button>
) : (
<Link href={`${pageId ? `${pageId}/` : ""}${tab.path}`}>{tab.name}</Link>
)}
</div>
);
})}
</ContainerComponent>
);
};
This could be improved, but I don't want to introduced too much refactoring for the moment as the goal of the issue was to sort out some accessibility issues.
Note that because we've introduced a button when the onSelect
exists, i.e.
{onSelect ? (
<button role="tab" onClick={() => onSelect(tab)}>
tab.name
</button>
) : (
<Link href={`${pageId ? `${pageId}/` : ""}${tab.path}`}>{tab.name}</Link>
)}
we no longer need the onKeyDown
to handle ENTER as this is handled natively with a button.
Description
The changes include adding
tabIndex={0}
to elements withrole="tab"
to ensure they are focusable forkeyboard navigation, addressing the accessibility warning (jsx-a11y/interactive-supports-focus).
Additionally, an
onKeyDown
handler was introduced to allow keyboard interaction, enabling users toselect tabs using the Enter or Space keys. These changes improve both accessibility and user
experience by making the tabs keyboard-accessible.
Related Tickets & Documents
fixes #4138
Mobile & Desktop Screenshots/Recordings
lint output:
Steps to QA
Tier (staff will fill in)
[optional] What gif best describes this PR or how it makes you feel?