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

ESLint Config: Adds some more basic a11y rules #25770

Merged
merged 3 commits into from Jul 27, 2021
Merged

ESLint Config: Adds some more basic a11y rules #25770

merged 3 commits into from Jul 27, 2021

Conversation

JeffersonBledsoe
Copy link
Contributor

Following on from #25462, I've added a few more accessibility-related ESLint rules. The rules added are as follows:

These rules were chosen as they are mostly focused on validation of ARIA, helping prevent misuse of ARIA without being too invasive. The rules are all currently set to warnings to further reduce the invasiveness of these rules.

@ijjk

This comment has been minimized.

@housseindjirdeh
Copy link
Collaborator

Thanks for this @JeffersonBledsoe!

+1 on setting those rules to warnings (for now) to minimize invasiveness. Wdyt about extending from the entire recommended ruleset in addition to the rules above (discussion here)?

@JeffersonBledsoe
Copy link
Contributor Author

Wdyt about extending from the entire recommended ruleset in addition to the rules above (discussion here)?

@housseindjirdeh I think it's a great idea, I'm all for best-practice accessibility out-the-box! However, some of the rules are a little more opinionated than others. For example, tabindex-no-positive and no-autofocus go against what is valid HTML, even though it's almost certainly for the better. Would Next.JS want to take an opinion in these kinds of situations? Could it be possible to keep some of these options behind a feature flag?

@JeffersonBledsoe
Copy link
Contributor Author

Just watched Next.js conf and the after party for the announcement of Next.JS 11. Great work from you and the whole team!
I think my questions & concerns were reflected well. It seems that Next.JS is willing to take a bit more of an opinionated stance to make the web better, which is great! My concern about it being opinionated was reflected in the after-party question about configured conformance. I think if it were possible to extended from the entire recommended ruleset but also make it easy to opt-out of the more controversial rules than having to dive too deep into ES lint, then that would be a great solution. Of course, users can still manually configure their .eslintrc to get more fine-grained control

@housseindjirdeh
Copy link
Collaborator

However, some of the rules are a little more opinionated than others. For example, tabindex-no-positive and no-autofocus go against what is valid HTML, even though it's almost certainly for the better.

@JeffersonBledsoe Ah, understood! I also realized that almost all rules checked in recommended mode of the plugin error and I definitely agree that we would probably want to stick to warnings to begin with. Thanks for your thoughts here :)

I think if it were possible to extended from the entire recommended ruleset but also make it easy to opt-out of the more controversial rules than having to dive too deep into ES lint, then that would be a great solution.

Yep! Our stance so far has been leaning on the strict side to begin with and then give users the option to opt out where appropriate. However, although I'm not too worried about rules that are defined that go against valid HTML, I am a little concerned with incorporating the full list of recommended rules since they are all almost errors. I think a good path forward would be to add the list of rules you've included here as warnings and then continue to add more if we'd like to before considering making the switch to the recommended ruleset.

Great work from you and the whole team!

Thank you, and thanks for helping out to make the whole experience better!

@ijjk

This comment has been minimized.

@housseindjirdeh housseindjirdeh self-assigned this Jul 8, 2021
@JeffersonBledsoe
Copy link
Contributor Author

@timneutkens This okay to be merged? (apologies for the tag, I wasn't sure who else to nudge 🙂 )

@ijjk
Copy link
Member

ijjk commented Jul 27, 2021

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
buildDuration 17.8s 17.9s ⚠️ +10ms
buildDurationCached 4s 3.9s -80ms
nodeModulesSize 50.3 MB 50.3 MB ⚠️ +2 B
Page Load Tests Overall increase ✓
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
/ failed reqs 0 0
/ total time (seconds) 3.239 3.216 -0.02
/ avg req/sec 771.77 777.35 +5.58
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.168 2.158 -0.01
/error-in-render avg req/sec 1152.98 1158.42 +5.44
Client Bundles (main, webpack, commons)
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
359.HASH.js gzip 2.96 kB 2.96 kB
745.HASH.js gzip 180 B 180 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 21 kB 21 kB
webpack-HASH.js gzip 1.53 kB 1.53 kB
Overall change 67.9 kB 67.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB
Overall change 31.1 kB 31.1 kB
Client Pages
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
_app-HASH.js gzip 803 B 803 B
_error-HASH.js gzip 3.06 kB 3.06 kB
amp-HASH.js gzip 554 B 554 B
css-HASH.js gzip 329 B 329 B
dynamic-HASH.js gzip 2.52 kB 2.52 kB
head-HASH.js gzip 2.28 kB 2.28 kB
hooks-HASH.js gzip 903 B 903 B
image-HASH.js gzip 5.63 kB 5.63 kB
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 1.66 kB 1.66 kB
routerDirect..HASH.js gzip 319 B 319 B
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 320 B 320 B
bb14e60e810b..30f.css gzip 125 B 125 B
Overall change 19.1 kB 19.1 kB
Client Build Manifests
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
_buildManifest.js gzip 489 B 489 B
Overall change 489 B 489 B
Rendered Page Sizes
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB

Webpack 4 Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
buildDuration 14s 13.8s -167ms
buildDurationCached 5.5s 5.4s -51ms
nodeModulesSize 50.3 MB 50.3 MB ⚠️ +2 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
/ failed reqs 0 0
/ total time (seconds) 3.245 3.234 -0.01
/ avg req/sec 770.41 772.92 +2.51
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 2.119 2.197 ⚠️ +0.08
/error-in-render avg req/sec 1179.78 1137.68 ⚠️ -42.1
Client Bundles (main, webpack, commons)
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
17.HASH.js gzip 2.98 kB 2.98 kB
18.HASH.js gzip 185 B 185 B
677f882d2ed8..HASH.js gzip 13.8 kB 13.8 kB
framework.HASH.js gzip 41.9 kB 41.9 kB
main-HASH.js gzip 8.4 kB 8.4 kB
webpack-HASH.js gzip 1.22 kB 1.22 kB
Overall change 68.5 kB 68.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
polyfills-HASH.js gzip 31.3 kB 31.3 kB
Overall change 31.3 kB 31.3 kB
Client Pages
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
_app-HASH.js gzip 791 B 791 B
_error-HASH.js gzip 3.76 kB 3.76 kB
amp-HASH.js gzip 552 B 552 B
css-HASH.js gzip 333 B 333 B
dynamic-HASH.js gzip 2.71 kB 2.71 kB
head-HASH.js gzip 2.97 kB 2.97 kB
hooks-HASH.js gzip 911 B 911 B
index-HASH.js gzip 231 B 231 B
link-HASH.js gzip 1.64 kB 1.64 kB
routerDirect..HASH.js gzip 298 B 298 B
script-HASH.js gzip 3.02 kB 3.02 kB
withRouter-HASH.js gzip 294 B 294 B
e025d2764813..52f.css gzip 125 B 125 B
Overall change 17.6 kB 17.6 kB
Client Build Manifests
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
_buildManifest.js gzip 500 B 500 B
Overall change 500 B 500 B
Rendered Page Sizes
vercel/next.js canary JeffersonBledsoe/next.js eslint-a11y-rules Change
index.html gzip 577 B 577 B
link.html gzip 588 B 588 B
withRouter.html gzip 569 B 569 B
Overall change 1.73 kB 1.73 kB
Commit: e443902

@timneutkens timneutkens merged commit 26105e2 into vercel:canary Jul 27, 2021
@timneutkens
Copy link
Member

Merged based on Houssein's review, thanks @JeffersonBledsoe!

flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants