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

[suggestion] eslint auto-fix support? #951

Closed
ckundo opened this issue Sep 5, 2023 · 8 comments
Closed

[suggestion] eslint auto-fix support? #951

ckundo opened this issue Sep 5, 2023 · 8 comments

Comments

@ckundo
Copy link

ckundo commented Sep 5, 2023

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!

@ckundo ckundo changed the title eslint auto-fixes? [suggestion] eslint auto-fixes support? Sep 5, 2023
@ckundo ckundo changed the title [suggestion] eslint auto-fixes support? [suggestion] eslint auto-fix support? Sep 5, 2023
@ljharb
Copy link
Member

ljharb commented Sep 5, 2023

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
Copy link
Author

ckundo commented Sep 5, 2023

@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
Copy link
Member

ljharb commented Sep 5, 2023

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
Copy link
Author

ckundo commented Sep 5, 2023

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
Copy link
Member

ljharb commented Sep 5, 2023

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
Copy link
Author

ckundo commented Sep 5, 2023

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
Copy link
Member

ljharb commented Sep 5, 2023

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
Copy link
Author

ckundo commented Sep 5, 2023

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.

@ckundo ckundo closed this as completed Sep 5, 2023
lb- added a commit to lb-/eslint-plugin-jsx-a11y that referenced this issue Oct 23, 2024
Add basic support for Eslint rule suggestions for the `interactive-supports-focus` for tabIndex values when elements are interactive but do not have implicit focusable browser behaviour.
These are intentionally not auto-fix but simply optional suggestions to help developers resolve their issue.

Fixes jsx-eslint#952
See also jsx-eslint#951
lb- added a commit to lb-/eslint-plugin-jsx-a11y that referenced this issue Oct 23, 2024
Add basic support for ESlint rule suggestions for the `interactive-supports-focus` for tabIndex values when elements are interactive but do not have implicit focusable browser behaviour.
These are intentionally not auto-fix but simply optional suggestions to help developers resolve their issue.

Fixes jsx-eslint#952
See also jsx-eslint#951
lb- added a commit to lb-/eslint-plugin-jsx-a11y that referenced this issue Oct 23, 2024
Add basic support for ESlint rule suggestions for the `interactive-supports-focus` for tabIndex values when elements are interactive but do not have implicit focusable browser behaviour.
These are intentionally not auto-fix but simply optional suggestions to help developers resolve their issue.

Fixes jsx-eslint#952
See also jsx-eslint#951
lb- added a commit to lb-/eslint-plugin-jsx-a11y that referenced this issue Oct 23, 2024
Add basic support for ESlint rule suggestions for the `interactive-supports-focus` for tabIndex values when elements are interactive but do not have implicit focusable browser behaviour.
These are intentionally not auto-fix but simply optional suggestions to help developers resolve their issue.

Fixes jsx-eslint#952
See also jsx-eslint#951
lb- added a commit to lb-/eslint-plugin-jsx-a11y that referenced this issue Oct 25, 2024
Add basic support for ESlint rule suggestions for the `interactive-supports-focus` for tabIndex values when elements are interactive but do not have implicit focusable browser behaviour.
These are intentionally not auto-fix but simply optional suggestions to help developers resolve their issue.

Fixes jsx-eslint#952
See also jsx-eslint#951
ljharb pushed a commit to lb-/eslint-plugin-jsx-a11y that referenced this issue Oct 26, 2024
Add basic support for ESlint rule suggestions for the `interactive-supports-focus` for tabIndex values when elements are interactive but do not have implicit focusable browser behaviour.
These are intentionally not auto-fix but simply optional suggestions to help developers resolve their issue.

Fixes jsx-eslint#952
See also jsx-eslint#951
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants