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

Is no-onchange still a relevant rule? #398

Closed
brendanthomas1 opened this issue Feb 5, 2018 · 16 comments · Fixed by #757
Closed

Is no-onchange still a relevant rule? #398

brendanthomas1 opened this issue Feb 5, 2018 · 16 comments · Fixed by #757

Comments

@brendanthomas1
Copy link

Hey, all. Like the title says, is this still a necessary rule? onchange is a pretty ubiquitous technique. React suggests using it for <select> elements to set state. Using onblur instead of onchange in this case means a user could potentially select an option with their keyboard and submit the form without ever losing focus, and the state wouldn't be updated.

Of course, not everyone is using React and I could just ignore the rule, but I wanted to throw this question out. W3C even has a suggested technique for their Web Content Accessibility Guidelines 2.0 that shows how to use onchange with a <select> element.

If this is still relevant, would we be able to get some more current references in the docs that support this rule? Of the two current citations, one is from 2004 and the other is from 2005. Considering this is an eslint plugin, it might be nice to have documentation that's not from a time when IE6 dominated the web 😝

@jnachtigall
Copy link

Also if using only onBlur without onChange then React prints out a warning in the Browser console:

Warning: Failed prop type: You provided a value prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultValue. Otherwise, set either onChange or readOnly.

So this is rule is in conflict with what React suggests. Like @brendanthomas1 says the official React select docs also say that one should use onChange:

@jessebeach jessebeach added this to To do in Deque aXe Hackathon at CSUN via automation Feb 11, 2019
@herecydev
Copy link

Still getting burnt by this, @evcohen can you weigh in on this?

@diverent2
Copy link

Working with the newest version of Svelte I now too have run into the warning.
"Fixing" it would break my functionality, but leaving it (or in my case disabling the warning) feels like cheating.

The comment sveltejs/svelte#4946 (comment) possibly points out why this ruleset is redundant and shouldn't be used any longer.

Any thoughts on this?

@diverent2
Copy link

diverent2 commented Sep 2, 2020

Okay I did some research:
According to http://www.themaninblue.com/writing/perspective/2004/10/19/

when you use the keyboard to explore the options in a select menu (using cursor keys) it triggers the onchange event handler (in IE).

However in my tests using IE 11 running

document.querySelector('#my-select').addEventListener('change', function(e) {
    console.log("change");
})

The log change only appears when I confirm my selection, as in: clicking outside or pressing enter, which would make this rule redundant.

Can't tell how older browsers handle it though. 🤔

@karlhorky
Copy link
Contributor

I ran into this also just now, teaching students.

I researched a bit, and I guess this rule can be deprecated and removed now? I didn't find the behavior described in the original motivation blog posts.

Would the maintainers accept a pull request to remove no-onchange?

@jessebeach @backwardok @octatone @evcohen @ljharb

@ljharb
Copy link
Member

ljharb commented Nov 17, 2020

Removing it would be a breaking change, so that PR would sit open for a long time.

Either way, it’d have to be tested back to IE 6 so we at least know for which browsers the rule is relevant - not everyone can drop IE support, or even drop support of IE < 11.

@karlhorky
Copy link
Contributor

karlhorky commented Nov 17, 2020

sit open for a long time

Maybe it's worth it to open it, for the next major version in the next year or so?

I suppose changing the default to 'off' in the built-in configs would also require a major version bump?

Either way, it’d have to be tested back to IE 6 so we at least know for which browsers the rule is relevant

Good point, that seems like a reasonable pre-requisite.

@ljharb
Copy link
Member

ljharb commented Nov 17, 2020

There is no default, unless you mean in one of the included configs - turning it off there would be a patch.

@karlhorky
Copy link
Contributor

Right sorry, was unclear there - I meant to remove the 'error' currently set in the recommended and strict configs. I've opened a PR for feedback here: #757

I think if this PR is acceptable, one more thing to do would be to mark the rule as deprecated in the docs for the rule (not sure about format for this - I guess somewhere near the top?).

Then I suppose this issue could be closed - the rule itself could stay around for those who want to enable it.

karlhorky added a commit to upleveled/eslint-config-upleveled that referenced this issue Nov 19, 2020
@karlhorky
Copy link
Contributor

So the rule has been deprecated and removed from the recommended and strict configs in #757.

Since the current version of eslint-plugin-jsx-a11y is 6.4.1, then I'm assuming we're looking at a release in versions 6.4.2 or 6.5.0 - would either of those be correct @jessebeach ?

@ljharb
Copy link
Member

ljharb commented Nov 28, 2020

Certainly it’ll be one of those :-)

@jessebeach
Copy link
Collaborator

I've been reading along. I need to find the time to sit down and consider the proposal. Thank you for the thoughtful change suggestion!

@ljharb
Copy link
Member

ljharb commented Nov 30, 2020

@jessebeach its already been merged; but there’s certainly time to change it if you disagree :-)

@jessebeach
Copy link
Collaborator

I'll have a read later, but I trust you all were thoughtful.

@James-Reed-cognito
Copy link

@ljharb @jessebeach is a new version of this plugin going to be released any time soon? :)

p-jackson added a commit to Automattic/wp-calypso that referenced this issue Feb 12, 2021
As outlined in jsx-eslint/eslint-plugin-jsx-a11y#398, the rule is no
longer relevant. It goes against the official React docs and keyboard
navigation in <select> controls works fine using the `onChange` prop.
The issue was in browsers older than IE 11.

The rule will be removed from jsx-a11y/recommended in a future release.
p-jackson added a commit to Automattic/wp-calypso that referenced this issue Feb 14, 2021
* Disable the jsx-a11y/no-onchange lint rule

As outlined in jsx-eslint/eslint-plugin-jsx-a11y#398, the rule is no
longer relevant. It goes against the official React docs and keyboard
navigation in <select> controls works fine using the `onChange` prop.
The issue was in browsers older than IE 11.

The rule will be removed from jsx-a11y/recommended in a future release.

* Remove unnecessary lint rule exceptions
audunru added a commit to netliferesearch/snomed-search that referenced this issue Apr 30, 2021
@efraindx
Copy link

efraindx commented Jul 5, 2021

You can temporarily bypass this error (until the fix is released), doing the following:
Add this: 'jsx-a11y/no-onchange': 'off' into the rules object in the .eslintrc.js file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

9 participants