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/no-autofocus` should not warn for false #1014

Open khinshankhan opened 1 week ago

khinshankhan commented 1 week ago

I know the docs https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/docs/rules/no-autofocus.md show fail as

<div autoFocus />
<div autoFocus="true" />
<div autoFocus="false" />
<div autoFocus={undefined} />

but I feel like this is bad linting for components outside of developer control. We use the prop on chakra ui components to force it to be false --which is proper a11y.

Ideally a framework would have set it up properly but they didn't. The workaround to set no-autofocus to warning or to throw in comments to ignore each case. However, I think a better way forward would be if the plugin allowed for falsy values (maybe as an option) so it would not fail on <Component autoFocus="false" /> or <Component autoFocus={false} />.

Comments work fine so completely understandable if this isn't the right mentality.

ljharb commented 1 week ago

Can you explain why you think it's proper a11y to set autofocus of a div, ever?

khinshankhan commented 1 week ago

Components implementations outside of developer control may implement it with autofocus true out of the box (which is wrong), so to make it proper, setting autofocus explicitly to false gives back expected behavior (which is right).

Some examples would be chakra ui's drawer component or bootstrap's modal component.

ljharb commented 1 week ago

I'm not sure I understand - the linter only warns on a literal <div>, which in that scenario would appear in the third party code, not your own (and you never ever lint third party code).

khinshankhan commented 1 week ago

That's with ignoreNonDOM enabled. I want it to keep checking developer created components.

This should be fine:

// from third party, unlinted
function ImagineFromPackage(props) {
  return <div autoFocus="true" {...props} />;
}

// my code, linted
function MyComponent() {
  return <ImagineFromPackage autoFocus={false} />;
}

And this should not:

// from third party, unlinted
function ImagineFromPackage(props) {
  return <div {...props} />;
}

// my code, linted
function MyComponent() {
  return <ImagineFromPackage autoFocus />;
}
ljharb commented 3 days ago

OK, I agree that should be fine.