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

Plugin svelte: A11y: on:blur must be used instead of on:change, unless absolutely necessary and it causes no negative consequences for keyboard only or screen reader users. #4946

Closed
pablote opened this issue May 31, 2020 · 16 comments · Fixed by #6457
Labels
compiler Changes relating to the compiler feature request

Comments

@pablote
Copy link

pablote commented May 31, 2020

I recently updated Svelte to latest version and this warning started to show up.

So I decided to try that out, change the on:change on some selects to on:blur, but they stopped working. It's only after I click away from the select that the event gets triggered, simply selecting the option doesn't do it.

I know I can simply ignore these, but I wanted to see if I can actually go ahead with this recommendation, maybe I'm doing something wrong. Thanks!

@dimfeld
Copy link
Contributor

dimfeld commented Jun 1, 2020

My understanding is that's the behavior change you described is actually the entire point of using on:blur as opposed to on:change.

If the user is accessing the select control with a keyboard, then as they go through the options in the select menu, on:change will be fired for each one. But if you use blur, then you only look at the value of the select once it has closed, so you aren't taking action against intermediate values that the user didn't want to select.

I'm not an a11y expert, but I think so long as you don't do anything unreasonable in your change handler, like making large alterations to the page layout, triggering a bunch of network queries, or anything that might cause the select menu to close, then you're just fine continuing to use on:change.

@pablote
Copy link
Author

pablote commented Jun 1, 2020

I didn't express myself correctly. I didn't mean to say that the event doesn't trigger as the user goes through the options of the select. I meant that, on a mouse interaction, even after selecting an option, an the option list goes away, still the blur event doesn't get triggered, because the select is still focused I guess. Only after clicking away on some whitespace I see it getting triggered, I can't allow for that sort of ux.

@dimfeld
Copy link
Contributor

dimfeld commented Jun 1, 2020

Ah, I see. Here's the official (as far as I've been able to find) rule that the Svelte one was adapted from: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/master/docs/rules/no-onchange.md.

Really, I think there's frustratingly little guidance on this rule. It's a big change from what most web developers currently do and the description of the rule is extremely vague on when it's ok to use onchange or not if you aren't already an accessibility expert. The link I posted above has two article links, one of which is dead and the other which is 15 years old: http://www.themaninblue.com/writing/perspective/2004/10/19/

That article is basically is making the case against doing what I mentioned in the last paragraph of my previous comment. This was common back in 2004 when select menus were often used as a kind of simple dropdown navigation, and selecting an option would trigger a page change. But I think that kind of behavior is pretty rare today though.

So I would just sum this one up as "make sure your select's on:change handler is friendly to keyboard users" and keep on using on:change. I think that's what I'm going to do.

@GrygrFlzr
Copy link
Member

There's a webaim.org mailing thread that discusses this issue: https://webaim.org/discussion/mail_thread?thread=8036

Quoting the reply:

In most cases [using onChange] is actually ok.
There are 3 scenarios where this is absolutely a problem (and defect
under WCAG 3.2.2)

  1. If the onchange event automatically moves focus to somewhere else
    on the page.
    This is a problem, because when you are in a dropdown and press the
    arrow key the onchange event is triggered. Then your focus may land
    somewhere else. If you are a keyboard only user and wanted to select
    the 6th option in the dropdown it could take you a long time if you
    have to refocus the dropdown for every arrow down press.
  2. If the onchange event triggers navigating to a different page or
    opening of another user agent (like auto playing a video or displaying
    a PDF file) .. I have never seen this except in theoretical scenarios ,
    but it could be coded that way.
  3. If the onchange event changes content that is located, in content
    order, before the dropdown. This could be remediated in some cases
    using an ARIA live region, but it is usually just a bad idea.
    If your interaction with a control changes the content, the change
    should occur after the control, it is commonsensical.

@rodryquintero
Copy link

How do I disable this warning.... on:blur does not work for my use case. It is annoying to see on Visual Studio Code.

@dimfeld
Copy link
Contributor

dimfeld commented Jun 16, 2020

You should be able to use rollup's onwarn hook to suppress it: https://rollupjs.org/guide/en/#onwarn

Not sure about other bundlers but there's probably something similar.

@pielambr
Copy link

pielambr commented Jun 26, 2020

How would you ignore this warning if you are running Webpack?

@rdmurphy
Copy link

rdmurphy commented Jul 1, 2020

Something I just realized while interacting with this warning — wouldn't this imply that bind:value is also inadvertently breaking the a11y rule? I was helping someone "fix" this warning and our answer was to swap to using bind:value instead (which was the better choice anyway/more Svelte-y). But it did get me thinking about what it was doing behind the scenes and sure enough — it's a change listener!

image

@dennis-f
Copy link

How do I disable this warning.... on:blur does not work for my use case. It is annoying to see on Visual Studio Code.

Edit your rollup.config.js like this. This will give you the opportunity to add more warnings later on.

const onwarn = (warning, onwarn) => {
	const isCircularWarning = warning.code === 'CIRCULAR_DEPENDENCY' && /[/\\]@sapper[/\\]/.test(warning.message); // only for @sapper
	const isOnBlurInsteadOfOnChangeWarning = warning.code === 'PLUGIN_WARNING' && warning.pluginCode && warning.pluginCode === 'a11y-no-onchange';

	return isCircularWarning
		|| isOnBlurInsteadOfOnChangeWarning
		|| onwarn(warning);
}


export default {
	client: {
          ...
          onwarn,
        },
       server: {
         ..., 
         onwarn,
       }
}

@paulovieira
Copy link

How would you ignore this warning if you are running Webpack?

For webpack it's very similar to the solution given by @dennis-f. In the options for svelte-loader add a new onwarn, like so:

// [...]

rules: [
    {
        test: /\.(svelte|html)$/,
        use: {
            loader: 'svelte-loader',
            options: {
                dev,
                hydratable: true,
                hotReload: false, // pending https://github.com/sveltejs/svelte/issues/2377
                onwarn: function (warning, handleWarning) {

                    if (warning.code === 'a11y-no-onchange') { return }

                    // process as usual 

                    handleWarning(warning);
                }
            }
        }
    },
    {
        // some other loader here...
    }
],

// [...]

This option is undocumented in https://github.com/sveltejs/svelte-loader, but it's easy to find it by looking at the source.

@jbg
Copy link

jbg commented Sep 9, 2020

Something I just realized while interacting with this warning — wouldn't this imply that bind:value is also inadvertently breaking the a11y rule? I was helping someone "fix" this warning and our answer was to swap to using bind:value instead (which was the better choice anyway/more Svelte-y). But it did get me thinking about what it was doing behind the scenes and sure enough — it's a change listener!

image

The idea isn't that change should never be listened to. It's that if you do listen to it, you should ensure that the event handler doesn't do anything that would cause the select control to be unusable for a keyboard user (since the change event will fire for each arrow key press as they move through the options, unlike for a mouse user where it doesn't fire until they click on their chosen option). For example, if the change handler can cause the select control to lose focus, or be removed from the document, then this could make the control unusable for a keyboard user.

I think it's probably reasonable for Svelte to push this responsibility onto the developer even when using bind:value, although it might be worth a warning so that people are aware that this is something to think about. The alternative (using on:blur instead of on:change internally for bind:value) would not work correctly for a lot of use cases, because a second click to defocus the select control would be required for the value to update.

The a11y warning could be worded better to make it clear that listening to change can be absolutely fine (and in fact is often the desired event), as long as you give consideration to these issues.

@orhanmaden
Copy link

orhanmaden commented Oct 1, 2020

I disabled it by putting a comment one line above the warning:

<!-- svelte-ignore a11y-no-onchange -->
<select  on:change={(e) => alert('hallo')}>
    <option>Item 1</option>
    <option>Item 2</option>
</select>

Edit: added select to example

@AlexGalays
Copy link
Contributor

AlexGalays commented Oct 13, 2020

Thanks, this warning is way over the top. It's absolutely untrue that it doesn't change anything for desktop.

@geoffrich
Copy link
Member

Imo, this rule should be removed.

  1. The corresponding jsx-a11y rule was deprecated in Nov 2020.
  2. The WebAIM mailing thread linked above says that using onchange is fine in most cases. There shouldn't be a warning for something that is usually okay, especially since the Svelte a11y warnings cannot be configured as easily as eslint.

The original reasoning behind this rule seemed to be that IE fired the change event when navigating between options with the arrow keys. From the linked article in the jsx-a11y docs:

The only problem is, when you use the keyboard to explore the options in a select menu (using cursor keys) it triggers the onchange event handler (in IE). So, if you have any actions that rely on that handler, they'll probably be executed before the user has time to listen to all the options in the select menu. It'll also mean they'll probably only ever be able to access the first item.

This was from 2004 and seems to be out of date now. I wrote a quick CodePen to see when the change event is fired for a <select> and tested in IE11, Chrome, and Firefox. In all browsers tested, tabbing to the select, opening the menu with Space, and using the arrow keys to navigate between different items did not fire a change event. A change event was only fired when an item was selected with Enter or if the arrow keys were used to select an option without opening the menu.

While older versions of IE may have made this necessary, I don't think it's reasonable to enforce a warning for all developers that only applies to a very small subset of people supporting IE < 11.

tl;dr the jsx-a11y this was based on is now deprecated. The rationale is many years old now and not applicable to most use cases when using a modern browser or IE11.

maxlath added a commit to inventaire/inventaire-client that referenced this issue Jun 17, 2021
as there is a growing consensus that this rule should be removed
see sveltejs/svelte#4946 (comment)
@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@geoffrich
Copy link
Member

Since the reaction to my comment above was positive, I've created a PR to remove the warning.

@stale stale bot removed the stale-bot label Jun 26, 2021
@pngwn pngwn added the compiler Changes relating to the compiler label Jun 27, 2021
maxlath added a commit to inventaire/inventaire-client that referenced this issue Aug 10, 2021
as there is a growing consensus that this rule should be removed
see sveltejs/svelte#4946 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Changes relating to the compiler feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.