Skip to content
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

💅 a11y/noNoninteractiveTabindex incorrectly labels treegrid child roles as non-interactive #4394

Open
1 task done
drwpow opened this issue Oct 26, 2024 · 6 comments
Open
1 task done
Labels
S-Needs triage Status: this issue needs to be triaged

Comments

@drwpow
Copy link
Contributor

drwpow commented Oct 26, 2024

Environment information

N/A

Rule name

lint/a11y/noNoninteractiveTabindex

Playground link

https://biomejs.dev/playground/?code=ZQB4AHAAbwByAHQAIABmAHUAbgBjAHQAaQBvAG4AIABUAHIAZQBlAEcAcgBpAGQAKAApACAAewAKACAAIAByAGUAdAB1AHIAbgAgACgACgAgACAAIAAgADwAZABpAHYAIAByAG8AbABlAD0AIgB0AHIAZQBlAGcAcgBpAGQAIgAgAGEAcgBpAGEALQBhAGMAdABpAHYAZQBkAGUAcwBjAGUAbgBkAGUAbgB0AD0AIgBjAGUAbABsAC0AMQAtADIAIgA%2BAAoAIAAgACAAIAAgACAAPABkAGkAdgAgAHIAbwBsAGUAPQAiAGMAbwBsAHUAbQBuAGcAcgBvAHUAcAAiAD4ACgAgACAAIAAgACAAIAAgACAAPABkAGkAdgAgAHIAbwBsAGUAPQAiAHIAbwB3ACIAPgAKACAAIAAgACAAIAAgACAAIAAgACAAPABkAGkAdgAgAGkAZAA9ACIAYwBvAGwALQAxACIAIAByAG8AbABlAD0AIgBjAG8AbAB1AG0AbgBoAGUAYQBkAGUAcgAiACAAdABhAGIASQBuAGQAZQB4AD0AewAwAH0APgBDAG8AbAB1AG0AbgAgADEAPAAvAGQAaQB2AD4ACgAgACAAIAAgACAAIAAgACAAIAAgADwAZABpAHYAIABpAGQAPQAiAGMAbwBsAC0AMgAiACAAcgBvAGwAZQA9ACIAYwBvAGwAdQBtAG4AaABlAGEAZABlAHIAIgAgAHQAYQBiAEkAbgBkAGUAeAA9AHsAMAB9AD4AQwBvAGwAdQBtAG4AIAAyADwALwBkAGkAdgA%2BAAoAIAAgACAAIAAgACAAIAAgACAAIAA8AGQAaQB2ACAAaQBkAD0AIgBjAG8AbAAtADMAIgAgAHIAbwBsAGUAPQAiAGMAbwBsAHUAbQBuAGgAZQBhAGQAZQByACIAIAB0AGEAYgBJAG4AZABlAHgAPQB7ADAAfQA%2BAEMAbwBsAHUAbQBuACAAMwA8AC8AZABpAHYAPgAKACAAIAAgACAAIAAgACAAIAA8AC8AZABpAHYAPgAKACAAIAAgACAAIAAgADwALwBkAGkAdgA%2BAAoAIAAgACAAIAAgACAAPABkAGkAdgAgAHIAbwBsAGUAPQAiAHIAbwB3AGcAcgBvAHUAcAAiACAAdABhAGIASQBuAGQAZQB4AD0AIgAwACIAIABhAHIAaQBhAC0AZQB4AHAAYQBuAGQAZQBkAD0AIgBmAGEAbABzAGUAIgA%2BAAoAIAAgACAAIAAgACAAIAAgADwAZABpAHYAIAByAG8AbABlAD0AIgByAG8AdwAiAD4ACgAgACAAIAAgACAAIAAgACAAIAAgADwAZABpAHYAIABpAGQAPQAiAGMAZQBsAGwALQAxAC0AMQAiACAAcgBvAGwAZQA9ACIAZwByAGkAZABjAGUAbABsACIAIAB0AGEAYgBJAG4AZABlAHgAPQB7ADAAfQA%2BAEEAPAAvAGQAaQB2AD4ACgAgACAAIAAgACAAIAAgACAAIAAgADwAZABpAHYAIABpAGQAPQAiAGMAZQBsAGwALQAxAC0AMgAiACAAcgBvAGwAZQA9ACIAZwByAGkAZABjAGUAbABsACIAIAB0AGEAYgBJAG4AZABlAHgAPQB7ADAAfQA%2BAEIAPAAvAGQAaQB2AD4ACgAgACAAIAAgACAAIAAgACAAIAAgADwAZABpAHYAIABpAGQAPQAiAGMAZQBsAGwALQAxAC0AMwAiACAAcgBvAGwAZQA9ACIAZwByAGkAZABjAGUAbABsACIAIAB0AGEAYgBJAG4AZABlAHgAPQB7ADAAfQA%2BAEMAPAAvAGQAaQB2AD4ACgAgACAAIAAgACAAIAAgACAAPAAvAGQAaQB2AD4ACgAgACAAIAAgACAAIAA8AC8AZABpAHYAPgAKACAAIAAgACAAPAAvAGQAaQB2AD4ACgAgACAAKQA7AAoAfQA%3D

Expected result

Both treegrid and grid are described to work like spreadsheets on the web, where data can be edited and individual cells can be focused within and navigated using arrow keys.

Treegrid takes it one step further and allows expanding/collapsing of rows and columns like a tree.

So when trying to implement focus behavior, this throws an error when it shouldn’t:

// The HTML element div is non-interactive. Do not use tabIndex. (❌ Incorrect)
<div role="gridcell" tabIndex={0}>
                     ~~~~~~~~~~~~

This conflicts with the recommended behavior of treegrid children:

Right Arrow
If focus is on a collapsed row, expand the row. …

In other words, a gridcell MUST be focusable when inside a treegrid if it’s expandable (via tabIndex), and also MUST be able to receive other keyboard commands.

lint/a11y/noNoninteractiveTabindex is correct in other instances, so it seems this is only a problem with grid and treegrid children.

Other notes

  • The useSemanticElements rule does apply to this example but is irrelevant; noNoninteractiveTabindex still raises the same errors
  • Worth noting that, yes, by default many gridcells (and related roles) aren’t interactive, this issue is in certain scenarios they ARE REQUIRED to be
  • The “roving tabindex” approach is optional here (where only the focusable element receives tabIndex={0} and everything else gets tabIndex={-1}), but also doesn’t affect the outcome

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@drwpow drwpow added the S-Needs triage Status: this issue needs to be triaged label Oct 26, 2024
@drwpow
Copy link
Contributor Author

drwpow commented Oct 26, 2024

I’d be open to raising a PR, but wanted to discuss first whether or not it would be sufficient to remove gridcell (and friends—row, rowgroup, etc.) from the “always non-interactive” list.

I’m inclined to do so because looking at the base grid role docs, we see keyboard interactions suggested even for native <table> elements. This leads me to believe that even though the base elements aren’t focusable using browser defaults, they are still recommended to be in many usecases. Therefore tabIndex shouldn’t be discouraged.

However, if we wanted to take a more complex approach, where gridcell and friends are only interactive under certain conditions (e.g. only if a child of treegrid), I’d feel less confident opening a PR until we defined that criteria (and whether or not we could lint that if the components were broken up). Either way, open to thoughts!

@ematipico
Copy link
Member

For these cases, where elements can be interactive based on certain conditions, we assume they can always be interactive.

So, to answer your question @drwpow, let's remove them :)

@drwpow
Copy link
Contributor Author

drwpow commented Nov 10, 2024

Works for me! Will open up a PR with that change then 🙏

@Conaclos
Copy link
Member

@drwpow I am landing important refactoring to biome_aria. I recommend working on the current issue once #4495 got merged.

@drwpow
Copy link
Contributor Author

drwpow commented Nov 11, 2024

@drwpow I am landing important refactoring to biome_aria. I recommend working on the current issue once #4495 got merged.

Great work on that! I didn’t get super far but did notice that it was basically a binary check of “is this a subclass or widget or not” and was going to make a worse hack than your more-correct fix.

Agree with the reasoning stated there, too. After having worked with my own treegrid component some more, and reading more on it, I was just going to have the following roles be possibly-interactive (the other roles I can’t say for sure they should never get tabindex, but I couldn’t find recommendations they do):

  • row
  • rowgroup
  • columnheader
  • rowheader
  • gridcell
  • cell*

* Note: cell is the debatable one here, since it seems like children of table are usually NOT interactive. But in some of the W3 examples they have things like <table role="grid"> … <td> and assert that td elements are just gridcell roles because they’re children, even though I wasn’t aware that is expected behavior (I still thought that elements retained their default roles regardless of parent/children and role was always needed to change that).

All that said, just seems like the easiest path to just say “cell can get tabindex sometimes” rather than “only gridcell can ever get tabindex and we must do deep analysis to make sure it’s a child of grid” which just sounded like a lot of work 😅

If any future PRs make those test cases possible then you can close this issue! But I can also start on this once #4241 is in a good place to make this change

@Conaclos
Copy link
Member

For these cases, where elements can be interactive based on certain conditions, we assume they can always be interactive.
So, to answer your question @drwpow, let's remove them :)

The fix should be easy: we just need to edit AriaRole::is_interactive. We should add proper comments and a link to the current issue.

In other words, a gridcell MUST be focusable when inside a treegrid if it’s expandable (via tabIndex), and also MUST be able to receive other keyboard commands.

I think, at some point a proper, we should add a proper Aria query that allows retrieving the roles of parent nodes.
This might allow us to check this sort of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Needs triage Status: this issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants