Activate more a11y checks in ESLINT: Proposal

[AlmeroSteyn]

The plugin eslint-plugin-jsx-a11y is already loaded in the create-react-app configuration. Currently, only a number of rules have been activated by default.

The rules can be found here: https://github.com/evcohen/eslint-plugin-jsx-a11y

The purpose of this issue is to determine if more rules may be activated by default to assist with creating more accessible JSX.

Another request is if we can upgrade the version of the plugin to 4.0.0.

I have had a look at the rules available in the plugin and will propose which rules I think can be activated by default with no impact on current applications where no gross HTML errors exist, which rules should be activated but will require some effort on the part of the developer and which rules could be too disruptive if switched on at once.

Important to note that this does not reflect on the importance of the rules. They should really all be on but considering the nature of create-react-app and its prolific use some rules could have too much of an impact. Considering that these rules can still be activated to report in the IDE's by using the method described in the docs, developers are still able to make use of a more strict configuration should they need to.

The following rules are already enabled:
'jsx-a11y/aria-role'
'jsx-a11y/img-has-alt' - Changed to 'jsx-a11y/alt-text': Slightly larger impact than previous but easy fixes and users already used to this activated in CRA
'jsx-a11y/img-redundant-alt'
'jsx-a11y/no-access-key'

I believe that the following rules can be enabled without causing any untoward impact on existing applications. They will only report where there are gross HTML/ARIA misuse:

The following are only checked when ARIA is used and therefore only impact if a11y is already in scope
'jsx-a11y/aria-activedescendant-has-tabindex'
'jsx-a11y/aria-props'
'jsx-a11y/aria-proptypes'
'jsx-a11y/aria-unsupported-elements'
'jsx-a11y/role-has-required-aria-props' - as 'jsx-a11y/aria-role' is already applied
'jsx-a11y/role-supports-aria-props'
'jsx-a11y/scope'

Only triggers when the element is actually empty or ARIA is applied wrongly
'jsx-a11y/heading-has-content'
'jsx-a11y/anchor-has-content'

Other
'jsx-a11y/no-distracting-elements'
'jsx-a11y/no-redundant-roles'

The following rules should really be on but will require some developer interaction in many cases. But the fixes are relatively easy:
'jsx-a11y/accessible-emoji' - All emojis should be in a <span> with appropriate role.
'jsx-a11y/href-no-hash' - All anchors should have a proper target href. Could however have a large impact in apps where anchors are used as buttons.
'jsx-a11y/html-has-lang' - HTML page should have a language attribute. Very important to have but the html file falls outside the scope of ESLINT.
'jsx-a11y/iframe-has-title' - IFrames should have a title attribites. Very easy fix and incredibly helpful
'jsx-a11y/no-autofocus' - Autofocus should not be used. This is even flagged by the W3C. However apps heavily leaning on autofocus could have a hard time removing it.
'jsx-a11y/onclick-has-role' - Elements with an onClick event should have a role. Adding a role is an easy fix to mitigate a bigger problem. - Rule Removed
'jsx-a11y/tabindex-no-positive' - Tabindexes higher than zero should not be used. Removing this is easy, however apps leaning on this could break visually.
'jsx-a11y/interactive-supports-focus'- New Rule: Any clickable elements should be in the keyboard focus flow
'jsx-a11y/no-interactive-element-to-noninteractive-role' - New Rule: Although user is already busy with ARIA roles, they can be copied and pasted from examples such as bootstrap and cause warnings that require HTHML changes.

The following rules, although very helpful and important will probably add too much difficulty to fix, expecially if activated on existing projects:
'jsx-a11y/click-events-have-key-events', 'jsx-a11y/mouse-events-have-key-events' - Click and mouse events should have keyboard counterparts. However sometimes it is also possible to solve this on other elements contained in the element in question which seems not be taken into account by the metric.
'jsx-a11y/label-has-for' - All labels should be accessibly linked to form controls. Very VERY important but currently does not support implicit labelling so could be wrongly blocking. Will log an issue on the plugin.
'jsx-a11y/no-onchange' - Select boxes should not trigger a change of context with an onchange. Handy but too restrictive for all uses of select.
'jsx-a11y/no-static-element-interactions' - Divs should ideally not be clickable. Whereas this is very good to have the anti-pattern is too prolific to fail builds on.
'jsx-a11y/no-noninteractive-element-interaction' - New rule: Important check for non-interactive elements having interactions. Whereas this is very good to have the anti-pattern is too prolific to fail builds on.
'jsx-a11y/media-has-caption' - New Rule: Captions should be provided for media. Will required a lot of work on existing if not present
'jsx-a11y/no-noninteractive-tabindex'- New Rule: Due to rule config this can be hard to manage in a no config environment
'jsx-a11y/no-noninteractive-element-interactions' - New Rule: Whereas this is very good to have the anti-pattern is too prolific to fail builds on.

What are your opinions in this matter?

If we do extend the config, perhaps this could be highlighted a bit in the docs? Would be happy to submit a proposal for a doc section/page.

@evcohen, would love to hear your view on this if you have time? Would be much appreciated.

[beefancohen]

@AlmeroSteyn thanks for this thorough write-up! I agree that we should start funneling in more of these rules.

@gaearon - quick question: do we have stats on how many people eject the config from CRA? If not, I think we should start keeping metrics this as to better inform some of the product decisions on this in the future. For instance, if 90% of people end up ejecting these configs, I think it makes sense to start becoming a bit more liberal with applying certain rules.

With that said, I am in favor of the breakdown that's been outlined here. Ideally, the second tier of rules (The following rules should really be on but will require some developer interaction in many cases. But the fixes are relatively easy:) is included, but I'll defer to you @gaearon. I think those are the most commonly missed rules and have the greatest potential to teach new users about a11y, but I can understand why you wouldn't want to unload tons of new information and forced changes onto early learners. However, I am of the opinion that teaching these things from the start is the most effective way to establish good a11y practices for newer devs.

[Timer]

I'm not a lawyer, but aren't there legal implications and required opt-ins for collecting metrics? I agree metrics would be useful, however.

[Timer]

Comparing react-scripts and react-dev-utils on NPM suggest out of 200,000 installs, 30,000 are ejected [in the past month]. Probably need to extract the growth from this, but it's a ballpark.

[beefancohen]

hah you're probably right, no idea though! i was just envisioning a basic stat counter, since we don't necessarily care about this on a per-user level more on a per-project level.

interesting! that suggests most people are not ejecting, in which case i think that the config should lean towards being more conservative when adding new rules

[AlmeroSteyn]

@evcohen It's only a pleasure!

We are also not ejecting yet as the benefits of having config managed outweighs the freedoms of ejecting. So I can understand these stats.

Perhaps we can look at the second tier rules then individually to consider whether the changes they would demand can be enforced or not? Would be great for accessibility if some rules nudged people into writing more accessible JSX, perhaps?

However, I do agree with not creating a monster that actually keeps new devs from using CRA.

[weyert]

Can never be a bad idea to enable more accessibility rules!

[AlmeroSteyn]

Hey everyone, has there been any thoughts around this issue?

[beefancohen]

@AlmeroSteyn do we want to hold off on this until the next major of the plugin is published? @jessebeach just made some major PRs that will be important changes and we can re-evaluate then.

However, I do agree that we should be adding more of these rules into CRA.

[Timer]

We would love to enable more of these where they make sense in the upcoming release.

I'm sure we can wait for v5 @evcohen.

[AlmeroSteyn]

@evcohen @Timer That sounds great! Thank you.

[AlmeroSteyn]

@evcohen @Timer With v5 out there do you think we can start looking at this again?

[Timer]

Sure @AlmeroSteyn! Would you like to draw up a PR enabling the new rules?

[AlmeroSteyn]

@Timer Will do.

[AlmeroSteyn]

I have so far updated the description above to match the changes in the plugin since I originally wrote this PR.

NOTE As part of this PR I will also do an upgrade to the current v5.0.1 of the plugin.

Rules that no longer exist are in strikethrough.
New rules are completely in bold.

With the exception of the change of jsx-a11y/img-has-alt to jsx-a11y/alt-text there are no changes in the first grouping of rules to be added so I will go ahead and add them.

Do we have an agreement yet whether the second grouping of rule under The following rules should really be on but will require some developer interaction in many cases. But the fixes are relatively easy are in or out? From the eject stats I assume we are not adding them at this stage? Or do we still want to discuss the rules on a case by case basis?

[Timer]

These seem reasonable, the others I feel are too restrictive (for that grouping):
The ones above this list all seem fine imo.

'jsx-a11y/accessible-emoji' - All emojis should be in a with appropriate role.
'jsx-a11y/href-no-hash' - All anchors should have a proper target href. Could however have a large impact in apps where anchors are used as buttons.
'jsx-a11y/html-has-lang' - HTML page should have a language attribute. Very important to have but the html file falls outside the scope of ESLINT.
'jsx-a11y/iframe-has-title' - IFrames should have a title attribites. Very easy fix and incredibly helpful

So no on these as well:

'jsx-a11y/no-autofocus' - Autofocus should not be used. This is even flagged by the W3C. However apps heavily leaning on autofocus could have a hard time removing it.
'jsx-a11y/onclick-has-role' - Elements with an onClick event should have a role. Adding a role is an easy fix to mitigate a bigger problem. - Rule Removed
'jsx-a11y/tabindex-no-positive' - Tabindexes higher than zero should not be used. Removing this is easy, however apps leaning on this could break visually.
'jsx-a11y/interactive-supports-focus'- New Rule: Any clickable elements should be in the keyboard focus flow

[gaearon]

Do we have a PR for this?

[AlmeroSteyn]

@gaearon I am putting one together.

[AlmeroSteyn]

I have opened a PR in #2163

[AlmeroSteyn]

@Timer Thank you for the reply. I have added the four extra rules to the config in the PR.

Agreed on the rest. In the docs I indicated how you can activate the full ruleset if using outside of CRA or when integrating with your IDE. Best of both worlds :-)

[gaearon]

Fixed via #2163.

[beefancohen]

thanks @AlmeroSteyn, @Timer, and @gaearon! hoping this helps a bunch of people 😄

[AlmeroSteyn]

Thank you everyone for your interest and feedback!