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

New rule: Flag elements with tabindex >= 0 without semantics that signal interactivity (Best Practice) #632

Closed
0ddfell0w opened this issue Dec 6, 2017 · 39 comments · Fixed by #667

Comments

@0ddfell0w
Copy link
Contributor

0ddfell0w commented Dec 6, 2017

Flag elements inserted into focus order without semantics that signal interactivity.

Checks that all elements without an aria role have a tabindex value of -1. Elements without an interactive role (implicit or explicit) are unexpected in the tab order.

Tags: cat.keyboard
Impact: minor

Selector

div, span, p, and heading elements inserted into the focus order


Checks

keyboard/focus-order-semantics

Passes if

  • element has role of type widget or composite OR
  • element has a tag name that implies it might scroll1 OR
  • element has a role that implies it might scroll2

Other, generate a violation

1 Tags for scrollable elements: article, section, nav, aside
2 Roles for scrollable elements: article, complementary, contentinfo, directory, document, form, main, navigation, region

@WilcoFiers
Copy link
Contributor

I don't see how this is an accessibility violation. Can you elaborate what you based this on? I've been considering something similar for element contained within role=application. Is that what you're looking for?

@0ddfell0w
Copy link
Contributor Author

I should have been clearer and included some reference to focus/tab order in the description, adding one now.

The idea is that flat text would be unexpected within the tab order. If the flat text is somehow interactive, that should be exposed to assistive tech using an aria role. Thoughts?

Success criteria for focus order: https://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-order.html

@dylanb
Copy link
Contributor

dylanb commented Dec 6, 2017

@0ddfell0w if you wanted to create a rule, it should be:

Look for all elements with tabindex >= 0 defined

  1. If they do not have a widget role (implicit or explicit) and
  2. they are screen reader visible

generate a violation

@marcysutton
Copy link
Contributor

I think it's the naming that got me confused: "Require a tabindex value of -1 for all flat text elements without an aria role". It's more like "Flag elements with tabindex of 0 or 1+ and flat text without an ARIA role". We could potentially exempt uses inside of role="application".

@0ddfell0w
Copy link
Contributor Author

You are both correct, will edit it accordingly

@0ddfell0w 0ddfell0w changed the title New rule: Require a tabindex value of -1 for all flat text elements without an aria role New rule: Flag elements with tabindex >= 0 that don't have an aria-role Dec 6, 2017
@0ddfell0w
Copy link
Contributor Author

Updated to remove references to "text" elements, thanks @dylanb.

@WilcoFiers
Copy link
Contributor

I think we need an exception for elements within a role=application element.

Furthermore, I want to have a closer look at if this is a true WCAG violation, or if this should be a best practice. I doubt it's a violation of 2.1.1. Is just having one of these on a whole page really a problem? I doubt it tbh. Now if the whole page is peppered with them you could maybe argue that it's significantly more difficult to operate the page with a keyboard then it is with the mouse. Even that is stretching the requirement quite a bit. The argument would have to be that it is impossible to use a page by keyboard if it has a lot of focusable elements that don't do anything. I don't think that that's true.

The alternative I see here is to argue for 4.1.2 (name, role, value). One might argue that if something is focusable, it should have a role. Then the argument would be that anything that's focusable is a user interface component. That interpretation makes a little more sense, but then you can't make exceptions for role=application, which I'm almost certain we will need.

In short, I don't know. I'll do a little more research and ask around a bit, see who's actually calling this out as a violation and what arguments they use.

@dylanb
Copy link
Contributor

dylanb commented Dec 7, 2017

@WilcoFiers I think it should be a best practice rule TBH - I am also concerned about false positives, s o if we implement this, I'd like to see it as experimental first

@WilcoFiers
Copy link
Contributor

Agreed. @0ddfell0w Can you update the rule accordingly?

@dylanb
Copy link
Contributor

dylanb commented Dec 8, 2017

More thoughts:

  1. This will definitely generate false positives for scrollable regions containing text, so we should exclude scrollable regions
  2. Impact should be 'minor' because it causes irritation rather than inaccessibility
  3. Category should be 'cat.keyboard'

@0ddfell0w 0ddfell0w changed the title New rule: Flag elements with tabindex >= 0 that don't have an aria-role New rule: Flag elements with tabindex >= 0 that don't have a role signaling interactivity (Best Practice) Dec 8, 2017
@0ddfell0w
Copy link
Contributor Author

0ddfell0w commented Dec 8, 2017

Sorry for the verbosity, let me know if I can clarify anything.

@dylanb your point about scrollable regions is interesting. There are several non-widget roles that might appropriately imply interactivity on a scrollable region, and I would argue that such a region does need a landmark role that helps explain what it is and why it's focusable.

I've updated the description and changed the wording about widget roles to encompass other roles that might signal the interactivity of the element (including cases where the only interaction is scrolling).

There are a few options i think are reasonable, let me know what you think.

Suggestion 1: Have one blacklist of roles we know are inappropriate for elements with tabindex > -1. We would still allow landmark roles like 'region' that might be appropriate for scrollable content, but we wouldn't allow landmark roles like banner to be interactive.

Suggestion 2: Have separate rules, one for scrollable regions and one for everything else with tabindex > -1. Allow all widget and some landmark roles for scrollable regions, but only allow widget roles for other elements with tabindex > -1.

Suggestion 3: Ignore scrollable regions entirely (@dylanb's original suggestion)

Example of a scrollable region without a role: Google Docs keyboard shortcut list

With respect to the risk of false positives on scrollable regions, here's an example of a scrollable region where it's pretty clear from the label what's going on, but I still think it should have a (landmark) role.

  1. https://docs.google.com/document/create
  2. Use Super+/ to open keyboard shortcuts dialog
  3. Tab to the scrollable region

Because there is no role on this, when a screen reader user focuses on the scrollable region they'll hear only "Group" or "Section" followed by the name of the element (which might be descriptive but might not be).

@WilcoFiers
Copy link
Contributor

I don't really understand this new part:

excluding elements within scrollable regions

I think to Dylan's point, we shouldn't exclude the scrollable regions themselves, I don't think that extends to anything within a scrollable region. There's another problem to consider with regard to scrollable regions. Different regions may or may not be scrollable depending on the dimensions of the window, or even depending on the page content.

We'd need a way to know that a region might be scrollable, even if it isn't in the specific situation you're testing. Unless this best practice requires some type of markup for regions that might scroll, such as a landmark indication, this rule will undoubtably have false positives.

The alternative would be to limit the rule. We could restrict it to inline elements online. That would probably still catch most offenders without us having to deal with the scrollable regions, even potential ones.

@0ddfell0w
Copy link
Contributor Author

You're right, changed the description from "excluding elements within scrollable regions" to "excluding elements that are scrollable regions.

I'm not sure I understand the suggestion about inline elements.

I can understand not wanting to distinguish scrollable regions from other elements, what do we think of the idea of having a blacklist of aria roles for elements with tabindex > -1? And/or recommending that all elements with tabindex > -1 must have a whitelisted role (i.e. a widget role or one of the landmark roles that could be appropriate)

@WilcoFiers
Copy link
Contributor

I see where you're coming from in the blacklist idea, but I don't think we should require scrollable regions to have a role, even as a best practice. I've searched and asked around, and I didn't find anyone who has suggested this before. That would make this a best practice we invented because we couldn't find another way to test it. I don't think aXe should be inventing best practices. That invites discussions which I'd rather happen somewhere else. If there are any recommendations I missed I'm happy to revisit them though.

As for inline elements, this was a possible alternative. If we only fail elements with display:inline we're a lot safer. We could even move all other non-scrollable elements into Needs review, but we'd be much closer to an actual best practice, and we wouldn't have to invent anything to make it work. What do you think @0ddfell0w?

@marcysutton
Copy link
Contributor

The usefulness of this would be really limited if we only flagged inline elements. After seeing web applications littered with extra tab stops, they are often caused by a fundamental misunderstanding of how screen readers work (and are a huge pain for both keyboard and screen reader users). But they are frequently put on block elements like DIVs, including wrapping elements (that may or may not scroll, as pointed out).

Perhaps we could explore this as one of our new tools instead of a traditional / best practice rule. If the tool could visit scrollable regions with the expectation that it might modify the page a bit more than usual, we could potentially be more accurate and provide adequate value to the user.

That said, doesn't axe-core already know about scrollable regions thanks to the color contrast rule?

@0ddfell0w
Copy link
Contributor Author

Thanks for clarifying. I'm trying to think of ways to encourage good behavior without risking false positives, it seems like there should be something we can do around elements that have been inserted into the tab order that shouldn't have been.

Couple options spring to mind,

  1. [Preferred] We specify that inline elements as well as headings and paragraphs should have a whitelisted role if it's been inserted into the focus order. This gets us most of the value I'm looking for

  2. We can have the blacklist/whitelist of appropriate roles for elements that have been inserted into the focus order, but we can include null as a valid role for now. Maybe an option could exist, that users could toggle, to add null to the blacklist and thus flag all interactive elements without a role.

@WilcoFiers
Copy link
Contributor

@marcysutton Axe knows which elements are scrollable, but what it doesn't know is what elements may become scrollable if the screen size or if it's content was different. For instance in the axe extension, the list of issues is scrollable if there are more items in it then the height of the panel can hold.

@0ddfell0w The other option I considered was to allow tabindex (>=0) on scrollable elements, and on elements with overflow:auto. It's not 100% air tight though, because something can have both of those in one layout, and with a narrower view, overflow could be changed with a media query, but it's pretty darn close. Close enough that I'd be okay with it, given that it's a best practice. WDYT?

@marcysutton
Copy link
Contributor

what it doesn't know is what elements may become scrollable if the screen size or it's content was different.

We tell users to test the page in multiple states–we should also be advocating for testing at multiple viewports, since it's impossible to axe-core to know the intent in all responsive scenarios. In my mind, it's a similar situation to hidden menu buttons. You have to put the page into a mobile viewport to test those.

@WilcoFiers
Copy link
Contributor

@marcysutton Are you suggesting people update the tabindex based on the viewport size then? Here's my scenario:

<div style="height:300px; overflow:auto;" tabindex="0">
   Lorum ipsum ...
</div>

If this element scrolls on a viewport of 800px, but not on 1280px, then it's a pass in the 800px, and a violation in the 1280px given the current proposal. If you as a dev wanted to fix that, you'd have to remove the tabindex on a resize event. That creates an inconsistency in the UI that's hard to understand for people who can't see it. It's also fairly involved adding/removing tabindex on a resize event.

So what I'm saying is, we should write the rule in such a way that it allows this element to pass, regardless of if it actually has a scroll bar or not.

@0ddfell0w
Copy link
Contributor Author

0ddfell0w commented Dec 14, 2017

@WilcoFiers i want to make sure I understand what we potentially agree on, does the following sound right?

Selector: Check all elements artificially inserted into tab index (it's in the focus order && not natively focusable)

Checks:
if element has overflow:auto or overflow:scroll, it's fine
if element has an appropriate1 role, it's fine
otherwise, generate a violation


1 appropriate could mean that it has any role, or it could be stipulated that it must be a widget or composite role. the landmark roles appropriate for scrollable regions can be ignored because we're ignoring scrollable regions

@dylanb
Copy link
Contributor

dylanb commented Dec 15, 2017

@0ddfell0w this seems like a reasonable definition

@WilcoFiers
Copy link
Contributor

Sound good. Which rules would you be excluding from this list though?

@0ddfell0w
Copy link
Contributor Author

@WilcoFiers from which list, sorry?

@slauriat
Copy link

Jumping in to clarify the motivation behind this, since 0ddfell0w filed this at my request.

Definitely a best practice type rule, basically looking at flagging elements developers have added tabindex to because they don't know how to test with screen readers (resulting in the typical anti-pattern where you end up with every heading and paragraph in the tab order).

I disagree with the "if element has overflow:auto or overflow:scroll, it's fine" part, mainly because I can't think of a valid use case for that where the developer shouldn't have used a landmark role to indicate the semantics of the scrollable area. Anything scrollable generally means one of two things: lengthy content given restricted space, or lousy CSS usage where something shouldn't've scrolled in the first place.

Thinking for cases where tabindex could make sense:
ARIA

  • any widgets
  • article
  • complementary
  • contentinfo
  • directory
  • document
  • form
  • main
  • navigation
  • region
    HTML
  • any widgets
  • article
  • section
  • nav
  • aside

Basically, anything interactive, and inclusive of semantics that indicate an arguably valid case for having a reason to scroll. For the Docs shortcut dialog example noted above (decidedly my own fault, since I forgot to require a role on that when working with the folks who made that keyboard scrollable), it should have a role of some sort to indicate what the user has landed on.

Totally open to reasons not to do this! I just can't think of a valid practice where something semantic-less or with flat text semantics (heading and paragraph examples above) should appear in the tab order. Also happy to just restrict the blacklist of tab-able things to something conservative in order to ensure we stay away from potential false positives, in order to at least still catch the common anti-patterns.

@dylanb
Copy link
Contributor

dylanb commented Dec 15, 2017

So clarifying where we stand:

We all agree that:

  1. Widget roles are allowed tabindex >= 0 (for the purposes of this rule)
  2. Non-scrollable regions without a landmark are not allowed to have a tabindex >= 0
  3. The rule should be a best practice

Where we need discussion:

  1. are there valid non-widget roles for tabindex >= 0

Where we disagree:

  1. scrollable areas without a role should be allowed tabindex >= 0

I suggest that we try to resolve the disagreement first before moving on to the discussion area. Before we do that - does my summary reflect accurately the current position?

@WilcoFiers
Copy link
Contributor

I found a few more potential false positives for us to avoid:

<main style="overflow:auto"><div tabindex="0">
   ... content
</div></main>
<pre style="overflow-x:auto" tabindex="0"><code>
   axe.run().then(out => console.log(out));
</code></pre>

Both of these seem perfectly valid solutions, where the tabindex is on an element that doesn't have a role. The first one also breaks with what I proposed about overflow, so that's out too I think. I'm not sure how to save this. We could make this an experimental rule, leave it sit for a while and see what feedback we get. But I'm not convinced that even if we do that, we can figure out a good way to make this work.

@slauriat
Copy link

Dylan: Yep, exactly my understanding as well. Thank you for summing up!

Wilco: Neither of those examples look valid to me. The first should just use <main style="overflow: auto;" tabindex="0" aria-labeledby="[someheadinginsideit]">…</main> without a div, and the second shouldn't use an HTML element for styling, instead using something like <code style="display: block; white-space: pre;…" tabindex="0" aria-describedby="[somecaptionforthecode]">…</code> (though it probably shouldn't have horizontal scroll in the first place - I'd expect that on the overall article/document/thing container, if at all).

I like the idea of adding it as an experimental rule! I definitely think, given the discussion, we could use more perspectives on this. Even if we do that and it shows that we can't make this work, I'll count that as a successful experiment (and will have happily learned something along the way).

@WilcoFiers
Copy link
Contributor

@slauriat I don't see how either one of the examples I gave could be an accessibility concern. Can you elaborate? I grant you that this isn't the best way to code it, but that's a different problem.

@slauriat
Copy link

VoiceOver announces the semantics as "group" and NVDA doesn't announce any semantics for the div, which makes for a confusing experience when trying to tab to a control. Depending on the content, users may think they've ended up on a button, or something else interactive, since they've tabbed there. If the element has semantics that better convey "Hey, you've landed on some flat text here" that helps avoid that confusion.

Not an issue that blocks people from using an app or navigating a page, but an issue that causes confusion and/or frustration for users with screen readers.

@dylanb
Copy link
Contributor

dylanb commented Dec 18, 2017

@slauriat ok, there are multiple ways that you can indicate the structure of a page and provide information for the user

  1. headings
  2. caption in a table
  3. landmarks
  4. frame title

Here is an example, that is legitimate a perfectly legitimate use of tabindex=0 that also does not confuse screen reader users http://dylanb.github.io/rwd/part4/comparison.html

@slauriat
Copy link

That example, while a bit weird to me that the table has a tabindex, totally makes sense that it wouldn't confuse users, since your focus goes to a table (only one on the page, and which has a table caption as well) and not a div. I also totally agree that you can (and should) provide information to the user in different ways.

More looking to catch the anti-pattern I described earlier ("a tabindex for every element!" - I've seen it many times…), so open to keeping the blacklist of elements conservative (headings, paragraphs, divs, spans, those sorts of things) so it wouldn't flag things like your table example.

@dylanb
Copy link
Contributor

dylanb commented Dec 19, 2017

@slauriat the use case is the following - the rest of the page is responsive to device size but because the table is used to compare features of products, it does not wrap but allows the user to scroll the table left to right using touch/mouse or the keyboard.

I believe that the most common legitimate use of a tabindex on text content regions is for scrolling purposes (whether vertical or horizontal).

So what we are debating here is a tradeoff between something that is valuable for sighted keyboard users but (your argument) negatively impacts screen reader users.

The experience for a screen reader user if they encounter a scrollable element with a tabindex is that the screen reader will read out the content of the element and then (depending on screen reader), the role of "group" if the element does not have another role (like heading, table etc.).

I could get behind the concept that if it is <div>, <span>, <p> we should add a landmark region (and an accessible name if it is not main)

@WilcoFiers any reason in your first example that the <main> region could not be expected to be the focusable element (just for a best practice)?

@slauriat
Copy link

slauriat commented Dec 19, 2017

I believe that the most common legitimate use of a tabindex on text content regions is for scrolling purposes (whether vertical or horizontal).

Agreed, but I think we disagree on where it makes sense to add tabindex: on a container with semantics (article, aside, main, document, etc.) or on non-interactive or semanticless elements (divs, paragraphs, etc.).

So what we are debating here is a tradeoff between something that is valuable for sighted keyboard users but (your argument) negatively impacts screen reader users.

Almost: I don't see this as an either or, but a best practice that helps both sets of users (and their intersection) by offering focusable scrolling where it makes sense to and not adding tabindex all over the place on otherwise non-interactive and semanticless elements. The downside of tabindex on flat elements doesn't just make for hearing "group" after things, it also adds confusion for those actively trying to tab through interactive elements and ending up on things that lack interactive semantics.

I could get behind the concept that if it is <div>, <span>, <p> we should add a landmark region (and an accessible name if it is not main)

Excellent! I'd like to add headings (HTML and ARIA, both) to that list as well, if cool? Again, my main concern: flagging the anti-pattern of adding tabindex to every element because the author doesn't know how screen readers work. Totally fine moving the overall best practice details discussion out of this bug if we have consensus on a short blacklist.

@0ddfell0w
Copy link
Contributor Author

Does the following look right to everybody?


For each element inserted into the focus order (in the focus order, but not natively focusable)

if it has a widget or composite role:
	PASS
if it has a [tag/role appropriate for scrollables:
	PASS
if marked up as a heading (h1-h6 tag or role=heading):
	FAIL
if its tag is one of {div, span, p}:
	FAIL
everything else:
	PASS

@slauriat already commented with the tags and roles appropriate for scrollables

@WilcoFiers
Copy link
Contributor

So you're saying don't allow tabindex <= 0 on headings, div, span or p, except if they have one of those roles, but allow it on all other elements? It's not perfect but I think it would work well enough.

@0ddfell0w
Copy link
Contributor Author

correct (for elements with tabindex specified as >= 0). I'll update the first comment in this conversation accordingly

@0ddfell0w 0ddfell0w changed the title New rule: Flag elements with tabindex >= 0 that don't have a role signaling interactivity (Best Practice) New rule: Flag elements with tabindex >= 0 without semantics that signal interactivity (Best Practice) Dec 21, 2017
@0ddfell0w
Copy link
Contributor Author

Updated. Let me know if there are objections, otherwise I'll prepare a PR

@WilcoFiers
Copy link
Contributor

You could probably move the div / span / p part into the selector. If we're not interested in other elements, I think we shouldn't bother with calling a pass on them.

@0ddfell0w
Copy link
Contributor Author

@WilcoFiers sounds good, updated

0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Dec 22, 2017
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Dec 22, 2017
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 2, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 2, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 3, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 3, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 4, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 4, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 4, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 4, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 4, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 9, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
0ddfell0w added a commit to 0ddfell0w/axe-core that referenced this issue Jan 11, 2018
Flag elements inserted into focus order without semantics that signal
interactivity.

fixes dequelabs#632
mrtnvh pushed a commit to mrtnvh/axe-core that referenced this issue Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants