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 #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]

#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...

svelte/packages/svelte/src/compiler/compile/nodes/Element.js

Lines 600 to 887 in d2d219f

	validate_attributes_a11y() {
	const { component, attributes, handlers } = this;
	const attribute_map = new Map();
	const handlers_map = new Map();
	attributes.forEach((attribute) => attribute_map.set(attribute.name, attribute));
	handlers.forEach((handler) => handlers_map.set(handler.name, handler));
	attributes.forEach((attribute) => {
	if (attribute.is_spread) return;
	const name = attribute.name.toLowerCase();
	// aria-props
	if (name.startsWith('aria-')) {
	if (invisible_elements.has(this.name)) {
	// aria-unsupported-elements
	component.warn(attribute, compiler_warnings.a11y_aria_attributes(this.name));
	}
	const type = name.slice(5);
	if (!aria_attribute_set.has(type)) {
	const match = fuzzymatch(type, aria_attributes);
	component.warn(attribute, compiler_warnings.a11y_unknown_aria_attribute(type, match));
	}
	if (name === 'aria-hidden' && regex_heading_tags.test(this.name)) {
	component.warn(attribute, compiler_warnings.a11y_hidden(this.name));
	}
	// aria-proptypes
	let value = attribute.get_static_value();
	if (value === 'true') value = true;
	if (value === 'false') value = false;
	if (
	value !== null &&
	value !== undefined &&
	aria.has(/** @type {import('aria-query').ARIAProperty} */ (name))
	) {
	const schema = aria.get(/** @type {import('aria-query').ARIAProperty} */ (name));
	if (!is_valid_aria_attribute_value(schema, value)) {
	component.warn(
	attribute,
	compiler_warnings.a11y_incorrect_attribute_type(schema, name)
	);
	}
	}
	// aria-activedescendant-has-tabindex
	if (
	name === 'aria-activedescendant' &&
	!this.is_dynamic_element &&
	!is_interactive_element(this.name, attribute_map) &&
	!attribute_map.has('tabindex')
	) {
	component.warn(attribute, compiler_warnings.a11y_aria_activedescendant_has_tabindex);
	}
	}
	// aria-role
	if (name === 'role') {
	if (invisible_elements.has(this.name)) {
	// aria-unsupported-elements
	component.warn(attribute, compiler_warnings.a11y_misplaced_role(this.name));
	}
	const value = attribute.get_static_value();
	if (typeof value === 'string') {
	value.split(regex_any_repeated_whitespaces).forEach(
	/** @param {import('aria-query').ARIARoleDefinitionKey} current_role */ (
	current_role
	) => {
	if (current_role && is_abstract_role(current_role)) {
	component.warn(attribute, compiler_warnings.a11y_no_abstract_role(current_role));
	} else if (current_role && !aria_role_set.has(current_role)) {
	const match = fuzzymatch(current_role, aria_roles);
	component.warn(attribute, compiler_warnings.a11y_unknown_role(current_role, match));
	}
	// no-redundant-roles
	if (
	current_role === get_implicit_role(this.name, attribute_map) &&
	// <ul role="list"> is ok because CSS list-style:none removes the semantics and this is a way to bring them back
	!['ul', 'ol', 'li'].includes(this.name)
	) {
	component.warn(attribute, compiler_warnings.a11y_no_redundant_roles(current_role));
	}
	// Footers and headers are special cases, and should not have redundant roles unless they are the children of sections or articles.
	const is_parent_section_or_article = is_parent(this.parent, ['section', 'article']);
	if (!is_parent_section_or_article) {
	const has_nested_redundant_role =
	current_role === a11y_nested_implicit_semantics.get(this.name);
	if (has_nested_redundant_role) {
	component.warn(
	attribute,
	compiler_warnings.a11y_no_redundant_roles(current_role)
	);
	}
	}
	// role-has-required-aria-props
	if (
	!this.is_dynamic_element &&
	!is_semantic_role_element(current_role, this.name, attribute_map)
	) {
	const role = roles.get(current_role);
	if (role) {
	const required_role_props = Object.keys(role.requiredProps);
	const has_missing_props = required_role_props.some(
	(prop) => !attributes.find((a) => a.name === prop)
	);
	if (has_missing_props) {
	component.warn(
	attribute,
	compiler_warnings.a11y_role_has_required_aria_props(
	current_role,
	required_role_props
	)
	);
	}
	}
	}
	// interactive-supports-focus
	if (
	!has_disabled_attribute(attribute_map) &&
	!is_hidden_from_screen_reader(this.name, attribute_map) &&
	!is_presentation_role(current_role) &&
	is_interactive_roles(current_role) &&
	is_static_element(this.name, attribute_map) &&
	!attribute_map.get('tabindex')
	) {
	const has_interactive_handlers = handlers.some((handler) =>
	a11y_interactive_handlers.has(handler.name)
	);
	if (has_interactive_handlers) {
	component.warn(
	this,
	compiler_warnings.a11y_interactive_supports_focus(current_role)
	);
	}
	}
	// no-interactive-element-to-noninteractive-role
	if (
	is_interactive_element(this.name, attribute_map) &&
	(is_non_interactive_roles(current_role) || is_presentation_role(current_role))
	) {
	component.warn(
	this,
	compiler_warnings.a11y_no_interactive_element_to_noninteractive_role(
	current_role,
	this.name
	)
	);
	}
	// no-noninteractive-element-to-interactive-role
	if (
	is_non_interactive_element(this.name, attribute_map) &&
	is_interactive_roles(current_role) &&
	!a11y_non_interactive_element_to_interactive_role_exceptions[this.name]?.includes(
	current_role
	)
	) {
	component.warn(
	this,
	compiler_warnings.a11y_no_noninteractive_element_to_interactive_role(
	current_role,
	this.name
	)
	);
	}
	}
	);
	}
	}
	// no-access-key
	if (name === 'accesskey') {
	component.warn(attribute, compiler_warnings.a11y_accesskey);
	}
	// no-autofocus
	if (name === 'autofocus') {
	component.warn(attribute, compiler_warnings.a11y_autofocus);
	}
	// scope
	if (name === 'scope' && !this.is_dynamic_element && this.name !== 'th') {
	component.warn(attribute, compiler_warnings.a11y_misplaced_scope);
	}
	// tabindex-no-positive
	if (name === 'tabindex') {
	const value = attribute.get_static_value();
	// @ts-ignore todo is tabindex=true correct case?
	if (!isNaN(value) && +value > 0) {
	component.warn(attribute, compiler_warnings.a11y_positive_tabindex);
	}
	}
	});
	// click-events-have-key-events
	if (handlers_map.has('click')) {
	const role = attribute_map.get('role');
	const is_non_presentation_role =
	role?.is_static &&
	!is_presentation_role(
	/** @type {import('aria-query').ARIARoleDefinitionKey} */ (role.get_static_value())
	);
	if (
	!this.is_dynamic_element &&
	!is_hidden_from_screen_reader(this.name, attribute_map) &&
	(!role || is_non_presentation_role) &&
	!is_interactive_element(this.name, attribute_map) &&
	!this.attributes.find((attr) => attr.is_spread)
	) {
	const has_key_event =
	handlers_map.has('keydown') || handlers_map.has('keyup') || handlers_map.has('keypress');
	if (!has_key_event) {
	component.warn(this, compiler_warnings.a11y_click_events_have_key_events);
	}
	}
	}
	const role = attribute_map.get('role');
	const role_static_value = /** @type {import('aria-query').ARIARoleDefinitionKey} */ (
	role?.get_static_value()
	);
	const role_value = /** @type {import('aria-query').ARIARoleDefinitionKey} */ (
	role ? role_static_value : get_implicit_role(this.name, attribute_map)
	);
	// no-noninteractive-tabindex
	if (
	!this.is_dynamic_element &&
	!is_interactive_element(this.name, attribute_map) &&
	!is_interactive_roles(role_static_value)
	) {
	const tab_index = attribute_map.get('tabindex');
	if (tab_index && (!tab_index.is_static || Number(tab_index.get_static_value()) >= 0)) {
	component.warn(this, compiler_warnings.a11y_no_noninteractive_tabindex);
	}
	}
	// role-supports-aria-props
	if (typeof role_value === 'string' && roles.has(role_value)) {
	const { props } = roles.get(role_value);
	const invalid_aria_props = new Set(aria.keys().filter((attribute) => !(attribute in props)));
	const is_implicit = role_value && role === undefined;
	attributes
	.filter((prop) => prop.type !== 'Spread')
	.forEach((prop) => {
	if (
	invalid_aria_props.has(/** @type {import('aria-query').ARIAProperty} */ (prop.name))
	) {
	component.warn(
	prop,
	compiler_warnings.a11y_role_supports_aria_props(
	prop.name,
	role_value,
	is_implicit,
	this.name
	)
	);
	}
	});
	}
	// no-noninteractive-element-interactions
	if (
	!has_contenteditable_attr(this) &&
	!is_hidden_from_screen_reader(this.name, attribute_map) &&
	!is_presentation_role(role_static_value) &&
	((!is_interactive_element(this.name, attribute_map) &&
	is_non_interactive_roles(role_static_value)) ||
	(is_non_interactive_element(this.name, attribute_map) && !role))
	) {
	const has_interactive_handlers = handlers.some((handler) =>
	a11y_recommended_interactive_handlers.has(handler.name)
	);
	if (has_interactive_handlers) {
	component.warn(
	this,
	compiler_warnings.a11y_no_noninteractive_element_interactions(this.name)
	);
	}
	}
	const has_dynamic_role = attribute_map.get('role') && !attribute_map.get('role').is_static;
	// no-static-element-interactions
	if (
	!has_dynamic_role &&
	!is_hidden_from_screen_reader(this.name, attribute_map) &&
	!is_presentation_role(role_static_value) &&
	!is_interactive_element(this.name, attribute_map) &&
	!is_interactive_roles(role_static_value) &&
	!is_non_interactive_element(this.name, attribute_map) &&
	!is_non_interactive_roles(role_static_value) &&
	!is_abstract_role(role_static_value)
	) {
	const interactive_handlers = handlers
	.map((handler) => handler.name)
	.filter((handlerName) => a11y_interactive_handlers.has(handlerName));
	if (interactive_handlers.length > 0) {
	component.warn(
	this,
	compiler_warnings.a11y_no_static_element_interactions(this.name, interactive_handlers)
	);
	}
	}
	}

...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.