Accessibility bug: FormNavItem links all have tabindex="-1", making none them focusable by TAB/SHIFT+TAB keyboard navigation

ashleemboyer commented 11 months ago

1. Environment

Survey: State of React 2023
Date encountered: Wednesday, November 1st, 2023
Device: MacBook Air M1 2020
OS: Ventura 13.5.1
Browser: Google Chrome Version 117.0.5938.132

2. Steps to Reproduce

Log in to the State of React 2023 survey
Go to any main section of the survey that has the progress <nav> element at the top of the page
After the page has loaded, start hitting the TAB key
Observe how the focus indicator moves through the page after each TAB press

3. Expected Result

I expected the TAB order of the page to be:

"Skip to content" link
"Surveys" breadcrumb link
"State of React 2023" breadcrumb link
Language selector
First <a> element in the progress <nav> at the top of the page

4. Actual Result

Number 5. above was actually the first <a> element in the Table of Contents <ol> on the lefthand side of the page

5. Screen recording

Here's a screen recording (with no sound) demonstrating the Actual Result I observed when running through the Steps to Reproduce.

https://github.com/Devographics/Monorepo/assets/43934258/dbd7a848-e6ae-4ded-82f3-d07b576d7b1c

6. Relevant Code Links

I didn't spend too much time searching for the affected code, but I believe this is the line of code where tabindex is set on these elements:

https://github.com/Devographics/Monorepo/blob/cf9ed11752b383d212a93db81dafd147f7786b8e/surveyform/src/components/form/FormNavItem.tsx#L101

7. Recommended Fix

Like I mentioned a few lines up, I didn't dig too deep into this code so I don't have all the context, but here is my current recommendation as a frontend React accessibility consultant: Just remove the tabindex property from here.

If I had to guess based on what I've seen in various projects over the years, it may have been intended for this users to navigate within this widget using arrow keys. That functionality is often an over-engineered solution with widgets like this one, which do not have a native HTML element they're modeled off that uses arrow keys for operation (e.g. <select> and <input type="radio" /> groups).

When a user comes across <a> elements like this that are used for high-level navigation, they expect to use the TAB and SHIFT+TAB keys to navigate between them. They aren't expecting to have to use their arrow keys to navigate between them. That is unless additional semantics are added to the elements (or the element that contains them all) using ARIA roles, states, or properties. It's certainly an option to do this, but there are a few great reasons not to:

No ARIA is better than bad ARIA: "A role is a promise" and "ARIA Can Both Cloak and Enhance, Creating Both Power and Danger"
The first rule of ARIA use: "If you can use a native HTML element or attribute with the semantics and behavior you require already built in, instead of re-purposing an element and adding an ARIA role, state or property to make it accessible, then do so."
The second rule of ARIA use: "Do not change native semantics, unless you really have to."

If the file I linked to above is indeed the correct file with this issue, I would be delighted to donate my time to fixing it and documenting the change! I would take a deeper look at the code in that case and come to a better understanding of the original intent to use the tabindex property. Any additional information would also be helpful beforehand. ☺️

eric-burel commented 11 months ago

@SachaG ok to remove those tabIndex?

ashleemboyer commented 4 months ago

Hi @eric-burel! Any updates? 😊

eric-burel commented 4 months ago

Hi, I'll do as second pass on the surveyform UI when we manage to stabilize the Next.js architecture, it's getting better with Next 15, and I have great hope with React 19 compiler in particular for form inputs that are terrible to handle with current versions of React. So yeah still a bit bogged in architectural concerns but no issue will be forgotten!

Devographics / Monorepo