Accessibility warnings that rely on event directives are wrong

[cdcarson]

Describe the bug

This perfectly legit code...

<div on:click={handler}><slot/></div>

...produces these a11y errors...

A11y: visible, non-interactive elements with an on:click event must be accompanied by an on:keydown, on:keyup, or on:keypress event. svelte(a11y-click-events-have-key-events)
A11y: <div> with click handler must have an ARIA role svelte(a11y-no-static-element-interactions)

This is wrong in two ways:

• Events propagate. Simply attaching an event to a non-interactive element has no effect in and of itself on a11y or the semantics on which AT relies. (If it does -- if user agents do indeed infer the role of an element from the event listeners that have been attached to it -- then I'm wrong. But that would be...ugh...their problem?)
• More importantly. The a11y errors and suggestions Svelte displays encourage, rather than discourage, bad a11y practices. "Sure buddy, it's ok to make your div a link. Just mess around until you hit on the right combination." This can easily lead to the following...

<div on:click={handler} role="button" tabindex="0" on:keyup={() => {}}>
  <slot/>
</div>

...which passes Svelte's a11y linting with flying colors even though it is obviously tripe. Less obviously, if my intention was simply to have the div listen for clicks, rather than turn it into a button, then the linter, rather than helping, has done significant harm, both to the accessibility of the end product and to my (the developer's) understanding of a11y and javascript.

Fix: A11y linting rules that rely on Svelte event directives (e.g. on:eventName) should be gotten rid of. There's no way (at least that I can think of) to divine what the presence of a listener implies.

There's an issue on this repo where folks have been talking about ways to disable a11y warnings in VsCode, etc. It's been open for two and a half years. Wouldn't it be better just to fix the the underlying problems rather than continue to tell people that they can skirt around the defects by doing this, that and the other?

[jasonlyu123]

It's indeed a false positive for catching bubbled event. But the compiler won't know that there is a button inside the slot. That's why there is the <!--svelte-ignore--> comment that can be used to disable when you're sure it is a false positive. https://svelte.dev/docs/accessibility-warnings

It's way more common for people to use on:event on the exact element the event triggers. We're not going to give up most of the cases we can catch because of the false positives.

As for hints about using a button instead, there is already an issue in the svelte core https://github.com/sveltejs/svelte/issues/8001.

This is overall a svelte core issue, so I'll close this later in favour of the upstream issue. What we can do is add a code action quick fix to turn div into a button. And maybe even add styles to remove the border and background. But I'll prefer to add this after svelte core add the hint to the warning message.

[cdcarson]

It's way more common for people to use on:event on the exact element the event triggers. We're not going to give up most of the cases we can catch because of the false positives.

I disagree. The salient case is listening for child events, where it would be inconvenient or impossible to attach the relevant listeners to each child.

Leave this open, please.

[jasonlyu123]

Transferred to the svelte core for the bubble problem. But I think this is a false positive where you can use the ignore comment to hide it. It's a comparatively rare case that is not justifiable for removing the rule entirely. Let's see what other maintainers think about it.

[dummdidumm]

Agree that this is a less common use case so while it's a false positive we need to keep the current rule. We should find a way to make the description less "oh I'm slapping this on there and be done with it" and maybe point to the documentation where that should be explained in more details including when this is a false positive. We also should have some kind of option to silence specific warnings project-wide.

[205g0]

We also should have some kind of option to silence specific warnings project-wide.

When?

[Prinzhorn]

https://github.com/sveltejs/svelte/issues/8001

[cdcarson]

Agree that this is a less common use case

What's the common use case? Trying to game the semantics and turn a div into a link? contenteditable?

I can give you many cases where one might legitimately listen for child mouse and keyboard events on divs. Modals, dropdowns, etc. These cases are perhaps even more common in Svelte than other frameworks, since Svelte, nicely, is not officious about HTML (e.g., no <Link/> component, not everything is assumed to be a component, etc.)

false positive

These errors are not false positives. The methodology is wrong, making the results universally suspect. Svelte tries to infer the a11y of an element from the presence of event listeners. The goodness or badness of this...

<!-- obviously bad -->
<div on:click={() => goto('/foo')}>Foo</div>

...and this...

<!-- probably legit -->
<div on:click={childListener}><slot/></div>

...cannot be inferred solely from the presence of on:click. The fact that the errors on the first guy are approximately correct does not make the methodology sound.

we need to keep the current rule

A11y in general is hard. The ARIA API is, well, ahem (~70 aria- attributes plus lord knows how many roles.) The official documentation is not great. There's not much overlap among people proficient in modern javascript and people proficient in a11y. (I'm not claiming to be anywhere on that Venn diagram.)

Given that environment, developers would be better served by Svelte doing only what it can do. In this case it's pretending to do something it can't. The pretense is demonstrably harmful. Folks are turning off the a11y linter entirely because certain errors, like the ones above, are unreliable. Other folks, like me in this case, get sent down rabbit holes -- "Oh @$#^, do user agents actually infer the a11y role from the listener?" -- and end up having to confirm, at great length, that the linter is talking nonsense. (Then writing lengthy GitHub issues.)

If the linter is key feature, rather than just being able to say "yeah, sure, Svelte has one," then it should work as well as everything else in the framework does.

In the meantime, I'll use comments to suppress the errors that bother me. Thanks as always for your work.

[brunnerh]

I highly doubt the "probably legit" part.

People adding click handlers on divs, unaware of any problems, is very common. And I would agree with the maintainers that valid cases are relatively rare. The most common thing should always be regular buttons with immediate handlers.

I just wish that #8001 would be addressed, just about anything would be better than the current state of affairs. If the warning explains everything properly, there will be no rabbit holes and lengthy research necessary to determine what to do with the warning. Either the user will realise they made a mistake and correct it or can just confidently ignore it.

[cdcarson]

User agents check for tabindex to determine whether a semantically non-interactive element (let's just say "div") is in the accessibility tree ("the tree".)

By contrast, the Svelte a11y linter considers any div with an attached mouse or keyboard event to be in the tree. This is wrong in two ways. First, it doesn't match what user agents do. Second, it ignores the existence of event propagation in javascript.

Suggestion: The linter should use tabindex as the marker that a div is in the tree, just like user agents. So...

<!-- not in the tree, no a11y errors -->
<div on:click={handler}>...</div>

<!-- in the tree, a11y errors -->
<div on:click={handler} tabindex="0">...</div>

A likely objection: It doesn't automatically catch the delinquent poster child, the div masquerading as a link or button. (See the first example above and assume handler does something button- or link-like.)

Yep. The case is and should be ignored. Reasoning...

1. There's plenty of documentation nowadays on how to remove user agent styles from buttons and links. Plus Tailwind. The temptation to game HTML semantics has largely gone away. (I'm not saying people won't do it, I'm just saying it's much harder in 2023 to be unaware of correct alternatives to the anti-pattern than it was, say, in 2010.)
2. A piece of UI is not "accessible" just because there are no linter errors. From WAI: "Knowledgeable human evaluation is required to determine if a site is accessible." In this case, the absence of tabindex on the div would prevent it from being focused via the keyboard. The problem would surface with a bare minimum of keyboard testing. And once tabindex is added, the linter can proceed, knowing for sure that the div is interactive.

Using tabindex is a more correct, cleaner solution than either copy editing (#8001) or hiding errors behind flags (comment.) It seems doable in the Element.validate_attributes_a11y method...

https://github.com/sveltejs/svelte/blob/d2d219f8ca0fd19dd7a076fa812d89596c69bae5/packages/svelte/src/compiler/compile/nodes/Element.js#L600-L887

...just check early on for the presence of tabindex >= 0 and use that to decide whether to surface the existing mouse and keyboard event errors.

Happy to post a pr if people think this is the right approach. Thanks!

[brunnerh]

The problem would surface with a bare minimum of keyboard testing

You are just way too optimistic about this. This is assuming that there even is any keyboard testing to begin with.

[cdcarson]

You are just way too optimistic about this. This is assuming that there even is any keyboard testing to begin with.

That's as may be (I haven't been accused of optimism for a couple of decades, so I'll take it as a compliment.)

Seriously -- there's only so much babysitting the linter can do. It can only be a reasonable starting point. If developers or their employers choose to ship software without adequate testing, then that's not Svelte's fault.

On the other hand, if developers turn off or ignore the linter because parts of it are unreliable, then that is Svelte's concern.

[non25]

I'd just use patch-package. It's more productive. :grin:

[oscarhermoso]

@cdcarson I believe you are correct about tabindex being more informative just event listeners... see the separator role - it derives from the abstract roles widget or structure depending on whether the element is "focusable". But people generally write the on:click and forget the tabindex, so it won't do much to improve accessibility.

With regards to your example - the "perfectly legit code" may not be perfect, because it's not just listening to bubbled events from child elements, it also listens to events from itself.

And of course, you could resolve around that with a bind:this={wrapperDiv} and returning early in your event handler if event.currentTarget === wrapperDiv.

For example, if there was an event modifier for notself (it would be the opposite of the self modifier), that both guarded against the code smell and resolved the non-interactive warning, that would possibly be a better solution?

[cdcarson]

@oscarhermoso Your example seems to prove my point. The LogWrapper component isn't doing any harm. It's not masquerading as a button or a link or some other interactive thing that should be marked as part of the accessibility tree, i.e. that should be focusable and tabbable to. Its existence does not affect the accessibility of the buttons within it. They can still be tabbed to and receive focus from the keyboard.

The component and its markup have no accessibility to "improve." Trying to improve it, as the Svelte linter now does based on incorrect (as I believe I've adequately shown in my comments above) assumptions, has the opposite effect.

[oscarhermoso]

@cdcarson - but accessibility is impacted, because there's an interaction that can be accessed by mouse but not keyboard (clicking on the wrapper itself)

Hence the proposal for a notself event modifier...

• If the LogWrapper was intended to be "clickable", then it should have an interactive role, a tabindex, and also be "keypressable".

• If the LogWrapper was not intended to be "clickable", then the notself modifier should be applied so it only listens to events bubbling from children, and resolves the warning

[oscarhermoso]

I raised a PR for a new event modifier named nonself (so that self/nonself matches the existing passive/nonpassive)

[cdcarson]

but accessibility is impacted, because there's an interaction that can be accessed by mouse but not keyboard

@oscarhermoso, fair enough. Your point might be better made / more easily understood with a real-world example. But I'll concede the point, and go with the assumption that the div in the example belongs in the accessibility tree.

So, let's compare the proposals, tabindex (comment above) vs. |nonself:

What gets fixed

• tabindex: Primarily, does away with the wrong warnings that appear in the common case of listening for child events, where HTML/a11y semantics are not gamed. Secondarily, accounts for gaming HTML semantics (if that is indeed necessary in real life. )
• |nonself: Primarily leaves things as they are, on the assumption that gaming semantics is the salient case. Secondarily, provides a hoop for developers to jump through to fix the event propagation problem.

Alignment with reality

• tabindex: Aligned. User agents and assistive tech use tabindex to determine inclusion in the tree.
• |nonself: Not aligned. Assumes users agents use event attachment to determine inclusion.

Svelte-ishness

• tabindex: Not Svelte-ish. Just add tabindex="0", which is table stakes for the gaming case with or without Svelte.
• |nonself: Svelte-ish. Developers would have to know/remember one more special thing about Svelte. (For example, I didn't remember the existence of the |self modifier until it was brought up here.)

Explanation in docs

(please assume some of the terms below are linked to WAI or MDN documentation)

• tabindex: Heads up! The a11y linter does not assume a semantically non-interactive element (like a div) with a mouse or keyboard listener is meant to be in the accessibility tree. If you intend the element to be part of the tree, then add tabindex="0". User agents will include the element in the tree, and the linter suggest appropriate attributes to make the markup accessible.
• |nonself: Heads up! The a11y linter assumes a semantically non-interactive element (like a div) with a mouse or keyboard listener is meant to be in the accessibility tree. This differs from the behavior of user agents and assistive technology, and ignores event propagation. If your non-interactive element isn't doing anything interactive, you must add the |nonself modifier to each on:eventName directive in order to suppress the incorrect linter warnings.

IMO, it's pretty clear that tabindex is the way to go.

[repulsio]

We also should have some kind of option to silence specific [ally] warnings project-wide.

@dummdidumm A simple on:click is causing my entire Svelte code to be underlined in squiggly yellow. This is dizzying and makes me want to halt all development on Svelte until there's a simple way to disable all ally warnings

Is making this option available to Svelte developers a priority?