jsx-eslint / eslint-plugin-jsx-a11y

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

[suggestion] eslint auto-fix support? #951

Closed ckundo closed 1 year ago

ckundo commented 1 year ago

Hi folks, thanks for all the hard work on this. I've been thinking about support for auto-fixing, if only to illustrate possible approaches to developers on how to fix accessibility issues. I didn't see any discussion in past issues so putting it out there.

I think the challenge to auto-fixing might be defining what "correct" looks like for many categories of failure, but maybe worth a try to at least present partial fixes?

Curious to hear your thoughts or any pointers to previous discussions i might've missed. Thanks!

ljharb commented 1 year ago

Not everything needs to be, nor should be autofixable.

That said, anything that's safe to autofix should be autofixed.

If there's a finite number of correct options, and the user has to choose, suggestions are appropriate.

Is there something specific you have in mind?

ckundo commented 1 year ago

@ljharb makes sense. I was thinking about the interactivity rules, for example jsx-a11y/click-events-have-key-events.

<div onClick={this.props.onClick}>My Button</div>

would yield both click-events-have-key-events and interactive-supports-focus issues. Autofixing both might be changed to something like:

<div
  tabIndex={0}
  onClick={this.props.onClick}
  onKeyDown={(e) => {
    if (e.code === "Space" || e.code === "Enter") {
      this.props.onClick();
    }
  }}
>My Button</div>
ljharb commented 1 year ago

Selecting a tabIndex definitely isn't a safe autofix, but it could be a suggestion.

As for the onKeyDown, that's not something a computer can ever just magically come up with, so that'd just have to be manually written.

ckundo commented 1 year ago

that's not something a computer can ever just magically come up with

Do you think its implausible to get a solution that is or approaches correct? Anything with an onClick handler should have, at a minimum, a space key handler that corresponds to the onClick event. If it has a button role, it should be space and enter. So, if there's an onclick, and there is no keydown, add a keydown handler. in the case that the role is link, trigger onclick on space. else if the role is button, trigger onclick on space and enter.

I'm not super navigating and asserting on the AST, but seems like this should be possible to write code to do this?

ljharb commented 1 year ago

I think that it is strictly inappropriate for an eslint rule to be that loose; autofixes must always be safe - no false corrections - and suggestions only make sense when there's a few correct options the user should pick from. Beyond that, users can just write the code, it's not that big a deal :-)

ckundo commented 1 year ago

Thanks @ljharb, the use case I was thinking about was a global autofix via eslint --fix ., I still think it'd be useful to autofix usage even in seemingly trivial cases like keyboard handlers, in very large codebases. That said, I agree this is not generically safe. We've played with some application-specific eslint fix overrides, and can continue down that path for those rules.

@ljharb if you want I can create a followup issue for suggesting tabindex=[0|-1]

ljharb commented 1 year ago

You can definitely make your own plugin and rule, that composes this one and adds your own autofixes.

No need for an issue if you want to just make a PR :-) otherwise an issue is fine.

ckundo commented 1 year ago

Thanks, makes sense. We're currently using eslint-rule-composer for exactly that. Would be happy and grateful for a better suggestion if you know of examples.

For the suggestion, I will create an issue for my own sake, to track the work since it might be a bit before I implement something.