Initial accessibility improvents and set up Eslint jsx-a11y plugin #5365
Conversation
rafawendel
force-pushed
the
add-eslint-jsx-a11y-plugin
branch
from
eeb5048 to
7eff1e1
Compare
rafawendel
changed the title
There was a problem hiding this comment.
Thanks for doing this! a11y is huge and often underappreciated.
I'd like to understand a little more the reasoning behind some of the roles and things. I feel like some of them could be changed or altered -- I tried not to clutter the PR with redundant comments if I came across the same issue more than a couple times.
client/.eslintrc.js
Outdated
| @@ -20,6 +21,8 @@ module.exports = { | |||
| // allow debugger during development | |||
| "no-debugger": process.env.NODE_ENV === "production" ? 2 : 0, | |||
| "jsx-a11y/anchor-is-valid": "off", | |||
| "jsx-a11y/no-autofocus": "off", | |||
| "jsx-a11y/click-events-have-key-events": "off", // TODO: create a wrapper for key events | |||
There was a problem hiding this comment.
Not yet. It might not be needed depending on how far we are moving into the a11y issue.
client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx
Show resolved
Hide resolved
client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx
Outdated
Show resolved
Hide resolved
client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx
Outdated
Show resolved
Hide resolved
client/app/components/dashboards/dashboard-widget/VisualizationWidget.jsx
Outdated
Show resolved
Hide resolved
rafawendel
force-pushed
the
add-eslint-jsx-a11y-plugin
branch
from
885c2f2 to
09f606e
Compare
There was a problem hiding this comment.
I did a time-boxed audit of https://deploy-preview-5365--redash-preview.netlify.app/alerts
Accessibility Insights for Web
- Homepage: There are still some issues, but they don't appear to touch the parts of the app you've started with.
- Other pages: Still a bunch of warnings and things. I think the plugin is a great start, but it definitely doesn't cover all of our usages.
Manual Testing
- Tabbing around the main menu is pretty good. The
Createbutton, since it has the ant popover menu, isn't accessible, and that's pretty frustrating (but we've already discussed it). Might be out of scope for round 1. - EditInPlace also has a click handler, and should be focusable (the same way the link form is focusable).
- Got some complaints on the
menuitemroles -- I think anymenuitemthat isn't a child of amenuormenubaris going to be upset (and in our case, I think it's happening when links have amenuitemrole within theMenu.Itemcomponent). Tabbing around QueryControlDropdown wasn't too bad, though! Could use higher contrast when focused, but it worked.
There's more, but I think you get the idea: I think this is a reasonable first increment once the comments are addressed. Nice!
rafawendel
force-pushed
the
add-eslint-jsx-a11y-plugin
branch
from
24bbe1b to
3aa2f66
Compare
|
@rafawendel Please keep an eye on Percy snapshots: this PR shouldn't affect the UI |
There was a problem hiding this comment.
I think the blast radius of this PR is growing, and we've got a lot of voices in the room. I agree with @kravets-levko 's concerns, but I don't have too much more to add at this point. If it's good with him, it's good with me.
To be honest, I think the best possible approach, here, would be to split this PR up into small (<100 lines) PRs that tackle a single thing in each PR (menu / menu item roles, adding an accessiblility component, using that accessibility component, etc.). It's up to you, but I'm worried that there's enough here that we'll end up having a lot of long discussions while the uncontroversial parts stay unmerged.
| @@ -71,9 +72,9 @@ export default function DesktopNavbar() { | |||
| const canCreateAlert = currentUser.hasPermission("list_alerts"); | |||
|
|
|||
| return ( | |||
| <div className="desktop-navbar"> | |||
| <nav className="desktop-navbar" role="navigation"> | |||
There was a problem hiding this comment.
nav shouldn't need an additional role.
There was a problem hiding this comment.
It is actually recommended by WAI ARIA, as navs are not supported in older browsers. Of course this is questionable, since these are not under our supported browsers. I'll just remove it
There was a problem hiding this comment.
Good news is that jsx/a11y actually enforces this behavior (try adding a role="link" to an anchor 🤠)
client/app/components/ApplicationArea/ApplicationLayout/DesktopNavbar.jsx
Outdated
Show resolved
Hide resolved
rafawendel
force-pushed
the
add-eslint-jsx-a11y-plugin
branch
from
55e9357 to
68e8676
Compare
| @@ -52,9 +52,9 @@ function AddToDashboardDialog({ dialog, visualization }) { | |||
| notification.success( | |||
| "Widget added to dashboard", | |||
| <React.Fragment> | |||
| <Link href={`${dashboard.url}`} onClick={() => notification.close(key)}> | |||
| <Button type="link" href={`${dashboard.url}`} onClick={() => notification.close(key)}> | |||
There was a problem hiding this comment.
I'm not 100% sure about semantics of this component. On the one hand, it's link, on the other - it's button. But for me it looks that link role is primary (as if you click it - it will bring you to a dashboard page), and onClick action is secondary (just to hide irrelevant UI). So my suggestion is to revert it to Link
What type of PR is this? (check all applicable)
Description
This aims to add the referred plugin, set up the appropriate rules and fix current suggestions.
After this PR the following standards should be met:
<Link>- Renders<a><Button>- Renders Antd<Button><Link.Button>- Renders Antd<Button>, which is rendered as an<a><PlainButton>- Renders styleless<button>Todos (might be moved to another PR)
roleattributes