Make a11y warnings on `svelte:element` based on computed value(s) of `this={...}`

[emmnull]

Describe the problem

After the recent update touching a11y, dynamic elements (<svelte:element />) often end up coated with warnings that are not valid based on the possible element outcome, breaking the expected consistency form a DX point of view.

Take this simple example:

<script lang="ts">
  export let href: string = undefined;
  export let tabindex: number = undefined;
</script>

<!-- These work as expected -->
<button on:click {tabindex}></button>
<a {href} on:click {tabindex}>Content</a>

<!-- This one doesn't:
  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: not interactive element cannot have positive tabIndex valuesvelte(a11y-no-noninteractive-tabindex)
-->
<svelte:element this={href ? 'a' : 'button'} {href} on:click {tabindex}>Content</svelte:element>

Describe the proposed solution

Somehow parse the contents of svelte:element's this={}?

Alternatives considered

Quick fix for those who want: mute the concerned warnings.

Importance

nice to have

[AlbertMarashi]

try role="button" on elements that behave as a button to fix the tabindex issue

I also noticed another bug in typescript where the e inside of on:keypress={e => {}} and on:click={e => {}} are untyped/typed as any

[rodrigodagostino]

I’ve been digging around for a few days for possible solutions to this exact same issue, and someone in StackOverflow very kindly pointed me here :)

I don’t think the problem is in how TS is evaluating your code, because it is covering every possible outcome for your component, and the one that is causing the error is the combination of the <a> element with the click event. Things like role="button" and on:keydown would be missing, and that’s a problem in accessibility terms.

What I think could actually help would be having conditional events (I don’t even know if something like that is possible), so, for example, the on:click event would only be part of the component if href is not provided. Maybe something like {href && on:click} as an attribute/prop for the component.

For the moment, the following approach (which I know is not the most elegant one) works without any kind of issue:

{#if href}
  <a class="button" {href} {target}>
    <slot />
  </a>
{:else}
  <button class="button" {type} on:click>
    <slot />
  </button>
{/if}

But it would be nice to have something that would allow us to keep the <svelte:element> in place, just in case we run into bigger and/or more complex scenarios :)

[emmnull]

@rodrigodagostino I'm fairly certain (but could be wrong though) that a tags with a non-empty href attribute (and non-#, that could be the source of the problem) are considered valid regardless of if they have a click handler or not (https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/main/docs/rules/anchor-is-valid.md). Either way, my gripe is more with the inconsistency of warnings/validity when comparing both illustrated cases that should result in identical outcomes.

Also, the if...else alternative is what I had before, but things quickly get awfully big when you have many props, for example a button primitive component that assigns ~35 attributes to the element (conditional classes, binding forwards, etc.)

[rodrigodagostino]

@iolyd well, it is possible to actually turn a simple <div> into some sort of button too, which is actually a common practice, but a very bad one in terms of accessibility. You need to provide the required attributes and events to the element for it to properly qualify as an alternative to the actual one. That’s why TS is correctly pointing that out. It is asking for you to add the required attributes and events so the <a> tag can properly function as a <button> is case an on:click is provided. If the on:click is not provided, then none of those will be needed, and that’s why the {#if}/{:else} does work without issues.

Here’s a little test ground for you to experience the difference between the two: https://svelte.dev/repl/ed6d42010bb549f98278a58c206f98ef?version=3.55.1

Using your keyboard, press Tab until you focus the <a> element, and then press Enter. You should now see a message printed in the console. Now focus the <button> element and press Enter. You should see a very similar message printed in the console again. So far so good, right? Now repeat the process using the Spacebar instead of Enter and see what happens.

As you might have realized, it’s not just a matter of switching tags, there are some other things that are important to cover in terms of a11y for everything to work as expected. And that’s why I’m pretty certain there is no issue in how TS is assessing our components. If there is a chance that an on:click event is added to the <a> tag, this <a> tag needs to be able to handle it properly in a11y terms.

I 100% agree with you though. There should be a way for us to make use of <svelte:element> without having certain props becoming an issue.

[emmnull]

@rodrigodagostino I understand your points, but I feel you haven't read the issue description and its example rigorously. The problem is the inconsistency of the ~~errors~~ warnings contrasted to the consistency of the resulting markup. Take these cases as an illustration:

<script lang="ts">
  let seed = Math.random() > .5;
  const el = 'a';
</script>

<svelte:element this="button" on:click>Button</svelte:element>
<!-- Perfectly fine, no warning, as expected -->

<svelte:element this="a" on:click href="/">Link</svelte:element>
<!-- Perfectly fine, no warning, as expected -->
<!-- Again, according to eslint click handlers on anchor tags are perfectly valid regarding a11y expectations, no need for key handlers.-->

<svelte:element this={seed ? 'a' : 'button'} on:click href={seed ? '/' : undefined}>Schrodinger's button</svelte:element>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
<!-- Whoops, we get a11y warnings that weren't raised in either previous cases although the possible outcomes are identical. -->

<svelte:element this={el} on:click href="/">Link</svelte:element>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
<!-- Same warnings, so something is not going through with the value of [this] and some unwarranted ambiguity appears eventhough the type of el is 'a', not string. --->

[rodrigodagostino]

@iolyd believe me that I’ve understood your situation perfectly, which is in fact the same situation I am going through. You mentioned that you understood my points, but you keep bringing up that click events are valid in <a> elements according to a11y guidelines, which is not quite correct. As I’ve mentioned before, click events are valid (in a11y terms) as long as you provide all the proper support to them. So to me, from those 4 examples, the only broken one seems to be the second one, which should be displaying a warning, but it’s not.

[emmnull]

@rodrigodagostino Fair points, I agree with your stance that, if we want to ensure a 100% identical behavior in terms of a11y, the one case standing out as invalid is the second one.

Although, I would note that the tests defined in the WCAG 2.1 specification only mention handling Enter as a requirement to be valid, handling Space key events - as you've illustrated is not done natively in your repl - is not a requirement. I do think your stance is fair and we ideally should strive to provide it, but when I'm mentionning the validity of click handlers on <a>, I'm referring to the extent that is framed by Eslint / TS and their depth in processing the markup, not to the ideal a11y case. It can't determine if the handler function itself is properly implemented, that is an exercise left to the rigor of developpers. But in light of the somewhat forgiving approach linters seem to have taken in regards to event handlers (and to which they are bound by the language's limitations), I think the viable solution would be to treat, at least, #4 identically to #2 and omit warnings.

[rodrigodagostino]

Even though I would prefer to always have the warning there. just in case I forget or someone else might end up learning how to properly do this taking a11y into consideration, I completely agree we should strive for consistency :)

Ohhu, and I would really like to know what you think about having something like “conditional events” added to Svelte.

[emmnull]

While reading through https://github.com/sveltejs/rfcs/pull/60, I vaguely remember seeing someone mention some syntactic or compiler-imposed limitations that, from my understanding, currently block the ability to have things such as conditional events. Unless you mean strictly from a typing perspective,... in that case yeah I would vouch for conditional events the same way we currently get attribute checking from SvelteHTMLElement: https://github.com/sveltejs/svelte/blob/ba8f979f03da248aa081c7208e0bfcaf63b73a06/elements/index.d.ts#L1398

[rodrigodagostino]

I was thinking on something beyond typing, which I was guessing would have its own very strong limitations for it to be implemented :P

Let’s hope we can still get some attention from the core team on this issue :)

[rodrigodagostino]

Just wanted to add that I ran into this other issue right here in the Svelte repo: https://github.com/sveltejs/svelte/issues/8001.

My apologies, mate. I insisted way to hard on it and it seems I was wrong. The click event seems to be allowed for <a> elements too. We still have a consistency issue, but now we should know where to properly point it to :P

[emmnull]

@rodrigodagostino Haha, no, no worries mate. In fact I would say your argument made me rethink some of my implementations so that's a plus!

Good find on the related issue.

I haven't delved into svelte's language tools, but I feel our issue here might be a missing as const parameter assertion somewhere that leads to any strict typing being stripped when a variable is passed instead of a string. But my understanding might be flawed, so I don't know to what extent we can really expect it to be fixable :/

[Conduitry]

This should be fixed now in 3.56.0.