jsx-eslint / eslint-plugin-jsx-a11y

Static AST checker for a11y rules on JSX elements.
MIT License
3.39k stars 637 forks source link

`jsx-a11y/label-has-associated-control` behavior is apparently misdocumented #976

Closed avkliskova closed 7 months ago

avkliskova commented 7 months ago

The foregoing rule in strict mode requires both an htmlFor attribute and an implicit association. This is the correct behavior, but the documentation doesn't mention this anywhere, AFAICT.

Cf. https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/docs/rules/label-has-associated-control.md

ljharb commented 7 months ago

by "strict mode" do you mean, in the strict config? included configs aren't documented anywhere, you'd have to check the code: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/src/index.js#L202-L293

avkliskova commented 7 months ago

I see. I assumed the behavior was different between configs for some reason, but your link tells me that both behave exactly the same way.

Either way, I'm showing errors on a <label> that wraps an <input> XOR has a htmlFor. This differs from the documented behavior:

This rule checks that any label tag (or an indicated custom component that will output a label tag) either (1) wraps an input element (or an indicated custom component that will output an input tag) or (2) has an htmlFor attribute and that the label tag has text content.

My guess is a recent pull request changed the behavior but not the documentation.

ljharb commented 7 months ago

In which version do you see this changing? Are you using the plugin directly, or via the airbnb config?

avkliskova commented 7 months ago

I believe it traces to #457. It doesn't mention a specific commit, but I do see the same XOR verbiage in the initial documentation commit: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/commit/d8ff54283279a9f1e4bd544863beeceff5362b20#diff-3ebaf3198dc642deb25323844e832cb908e0cfd2ffe0ae03578af74dae81aa81.

The rule changed at least by #759.

ljharb commented 7 months ago

PRs don't land in order, so the important thing is to figure out if this behavior actually changed in a specific release.

avkliskova commented 7 months ago

I see the trouble now. The documentation does mention assert: "both", and it appears this is the default. Since assert: "both" is in fact the correct behavior, I feel the paragraph I cited earlier should read something like

By default, this rule checks that any label tag (or an indicated custom component that will output a label tag) both (1) wraps an input element (or an indicated custom component that will output an input tag) and (2) has an htmlFor attribute and that the label tag has text content.

I'd be willing to submit a PR to this effect.

ljharb commented 7 months ago

The default is "either", per https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/src/rules/label-has-associated-control.js#L57. This suggests that the docs are correct.

Can you share your eslint config? Something must be setting it to "both".

avkliskova commented 7 months ago

Oh, heck, you're correct. It's airbnb. I was going to mention this in a prior comment, but got lost copy-pasting to a new one.

718 indicates that I'm not the only one who has been caught by this. I appreciate that documentation is not the place for debate that goes into issues, but the configuration difference is buried in a different repository. I wish now that I hadn't gotten caught in this maze.

Of course a11y linting != a11y but finding disparate best practices for something so seemingly simple says something regrettable about the current state of things. But nothing I can do about that.

ljharb commented 7 months ago

"Both" is the better and more accessible setting, so if we ever do a major version, I'll want to change the default to match.

avkliskova commented 6 months ago

Oh, it's just a semver thing. Okay, well, that makes sense. As someone in another thread said, you're a beast. Thank you for what you do.