fix: warnings in examples page #8836

AlimurtuzaCodes · 2023-06-24T15:04:51Z

What does this PR do

Fixes all warnings in examples page.

AlimurtuzaCodes · 2023-06-25T08:50:42Z

geoffrich

Thanks for the PR! There's a few issues I see:

some of the places where you added role='button' would be better to use an actual <button> element. This is because we use role=button, we also have to listen for key events and handle Space and Enter appropriately. It's simpler to just use the native button
some of the examples heavily rely on mouse movement so can't really be made accessible by adding buttons. We may want to rethink these examples in the future to not be so mouse-reliant but for now should ignore the warnings instead of adding roles that don't actually make the example more accessible.
some of the warnings you resolved were false positives and should be rightfully ignored.

I think this PR shows that our current a11y warnings could definitely be improved to be more helpful and/or actionable (and avoid warnings that are false positives). They're pretty ARIA-heavy right now, and in most cases the solution is to use native HTML elements instead of ARIA. But that's something to be tackled separately from this PR.

geoffrich · 2023-06-26T02:24:28Z

documentation/examples/04-events/00-dom-events/App.svelte

@@ -7,7 +7,7 @@
 	}
 </script>

-<div on:mousemove={handleMousemove}>
+<div on:mousemove={handleMousemove} role='button' tabindex='0'>


This should stay as is - the tutorial is showing reacting to mouse movements. Adding role="button" tabindex="0" doesn't help here. It would be better to ignore the warning instead, at least for now.

geoffrich · 2023-06-26T02:27:29Z

documentation/examples/11-easing/00-easing/Controls.svelte

@@ -28,6 +28,7 @@
 		{:else}
 			<ul>
 				{#each [...eases] as [name]}
+				<!-- svelte-ignore a11y-click-events-have-key-events a11y-no-noninteractive-element-interactions -->


instead of ignoring the warnings here, nest a button inside the <li> instead and apply the click handler there. e.g.

<li><button on:click={() => (current_ease = name)}>{name}</button></li>

(note: this may require adjusting some styles)

geoffrich · 2023-06-26T02:28:08Z

documentation/examples/11-easing/00-easing/Controls.svelte

@@ -46,6 +47,7 @@
 		{:else}
 			<ul>
 				{#each types as [name, type]}
+				<!-- svelte-ignore a11y-click-events-have-key-events a11y-no-noninteractive-element-interactions -->


same comment as above - create a button inside the <li> instead of ignoring the warnings

geoffrich · 2023-06-26T02:31:21Z

documentation/examples/15-composition/05-modal/Modal.svelte

 <dialog
 	bind:this={dialog}
 	on:close={() => (showModal = false)}
 	on:click|self={() => dialog.close()}
 >
-	<div on:click|stopPropagation>
+	<div on:click|stopPropagation role='button' tabindex='0'>


This shouldn't be made a button - the click handler is just listening to all child events. In this case it's okay to ignore the warning since screen reader users have other ways to close a native dialog and don't need to use this click handler.

geoffrich · 2023-06-26T02:37:58Z

documentation/examples/17-special-elements/00-svelte-self/Folder.svelte

@@ -11,7 +11,8 @@
 	}
 </script>

-<span class:expanded on:click={toggle}>{name}</span>
+<!-- svelte-ignore a11y-click-events-have-key-events -->
+<span class:expanded on:click={toggle} role='button' tabindex='0'>{name}</span>


In this case, it would be better to make this a native <button> element instead of applying role + tabindex. This is because we use role=button, we also have to listen for key events and handle Space and Enter appropriately. It's simpler to just use the native button

geoffrich · 2023-06-26T02:42:14Z

documentation/examples/21-miscellaneous/02-immutable-data/ImmutableTodo.svelte

@@ -15,7 +15,8 @@

 <!-- the text will flash red whenever
 		the `todo` object changes -->
-<div bind:this={div} on:click>
+<!-- svelte-ignore a11y-click-events-have-key-events -->
+<div bind:this={div} on:click role="button" tabindex='0'>


Instead of ignoring this warning, we should add a button instead that will complete the todo and attach the click handler to that

geoffrich · 2023-06-26T02:43:03Z

documentation/examples/04-events/01-inline-handlers/App.svelte

@@ -2,7 +2,7 @@
 	let m = { x: 0, y: 0 };
 </script>

-<div on:mousemove={(e) => (m = { x: e.clientX, y: e.clientY })}>
+<div on:mousemove={(e) => (m = { x: e.clientX, y: e.clientY })} role='button' tabindex='0'>


Same comment - we should ignore this warning instead of adding role="button" (which won't help in this case)

geoffrich · 2023-06-26T02:43:58Z

documentation/examples/08-motion/01-spring/App.svelte

@@ -28,6 +28,7 @@
 	on:mousemove={(e) => coords.set({ x: e.clientX, y: e.clientY })}
 	on:mousedown={() => size.set(30)}
 	on:mouseup={() => size.set(10)}
+	role='button' tabindex='0'


We should ignore this warning for now - this tutorial is very reliant on mouse interactions, adding role="button" won't address the root issue

geoffrich · 2023-06-26T02:44:48Z

documentation/examples/15-composition/03-slot-props/Hoverable.svelte

@@ -10,6 +10,6 @@
 	}
 </script>

-<div on:mouseenter={enter} on:mouseleave={leave}>
+<div on:mouseenter={enter} on:mouseleave={leave} role="button" tabindex='0'>


We should ignore this warning for now, since this example relies on hovering and can't be made keyboard accessible

geoffrich · 2023-06-26T02:48:20Z

documentation/examples/21-miscellaneous/02-immutable-data/MutableTodo.svelte

+<!-- svelte-ignore a11y-click-events-have-key-events -->
+<div bind:this={div} on:click role="button" tabindex='0'>


As above, we should add a button to complete todos instead of ignoring the warning

AlimurtuzaCodes · 2023-06-26T04:20:28Z

Thanks for the PR! There's a few issues I see:

some of the places where you added role='button' would be better to use an actual <button> element. This is because we use role=button, we also have to listen for key events and handle Space and Enter appropriately. It's simpler to just use the native button

some of the examples heavily rely on mouse movement so can't really be made accessible by adding buttons. We may want to rethink these examples in the future to not be so mouse-reliant but for now should ignore the warnings instead of adding roles that don't actually make the example more accessible.

some of the warnings you resolved were false positives and should be rightfully ignored.

I think this PR shows that our current a11y warnings could definitely be improved to be more helpful and/or actionable (and avoid warnings that are false positives). They're pretty ARIA-heavy right now, and in most cases the solution is to use native HTML elements instead of ARIA. But that's something to be tackled separately from this PR.

I will fix the issues.

AlimurtuzaCodes · 2023-06-26T05:14:33Z

@geoffrich Made the changes as per your request. Review it.

dummdidumm · 2023-06-26T07:53:42Z

Thanks for updating these! I think what Geoff meant with "ignore this warning" was to add a  for each a11y warning that still appears.

AlimurtuzaCodes · 2023-06-26T07:56:16Z

Thanks for updating these! I think what Geoff meant with "ignore this warning" was to add a  for each a11y warning that still appears.

I have tried all the warning codes as per docs, but none of the warnings worked.

dummdidumm

Strange that it didn't work for you - I add the ignore tags which seems to silence them for me.

I think we're good to go now - thank you!

fix: warnings in examples page

3f27ebb

geoffrich requested changes Jun 26, 2023

View reviewed changes

fix: warnings in examples

e031c4f

vercel bot deployed to Preview June 26, 2023 08:13 View deployment

ignore several warnings

8944380

dummdidumm approved these changes Jun 26, 2023

View reviewed changes

dummdidumm merged commit bbcb5f5 into sveltejs:master Jun 26, 2023

		<!-- svelte-ignore a11y-click-events-have-key-events -->
		<div bind:this={div} on:click role="button" tabindex='0'>

Uh oh!

fix: warnings in examples page #8836

fix: warnings in examples page #8836

Uh oh!

Conversation

AlimurtuzaCodes commented Jun 24, 2023

What does this PR do

Uh oh!

AlimurtuzaCodes commented Jun 25, 2023

Uh oh!

geoffrich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlimurtuzaCodes commented Jun 26, 2023

Uh oh!

AlimurtuzaCodes commented Jun 26, 2023

Uh oh!

dummdidumm commented Jun 26, 2023

Uh oh!

AlimurtuzaCodes commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dummdidumm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlimurtuzaCodes commented Jun 26, 2023 •

edited

Loading