Improve icon a11y #5424

rafawendel · 2021-03-12T19:10:54Z

What type of PR is this? (check all applicable)

Feature

Description

This aims to add accessibility to icons by:

Hiding decorative icons via aria-hidden
Adding visually hidden "screen-reader only" helper text where applicable
Adding aria-label, aria-labelledby and aria-describedby where applicable

Ant Icons already have somewhat comprehensible labels 🎉, so only particularly relevant ones will be a11y-zed

Related Tickets & Documents

#5418, #5419, #5420, #5421

Screenshots

This PR is not supposed to result in visual changes or regressions, but I used the opportunity to fix minor ones.
(Cc. @wenbow)

Before	After

Note for reviewers

The commit order is intended to be comprehensive to review.

client/app/assets/less/inc/generics.less

client/app/components/ApplicationArea/ApplicationLayout/VersionInfo.jsx

client/app/pages/queries/VisualizationEmbed.jsx

client/app/components/EmailSettingsWarning.jsx

client/app/components/FavoritesControl.jsx

gabrieldutra · 2021-03-16T01:26:40Z

client/app/components/groups/ListItemAddon.jsx

  }
  if (alreadyInGroup) {
    return (
      <Tooltip title="Already selected">
-        <i className="fa fa-check" />
+        <i className="fa fa-check" aria-hidden="true" />
+        <span className="sr-only">Already selected</span>


I feel like tooltips should have accessibility support by Antd rather than us adding manually to each of our uses. However it doesn't 😕, so I'm no so sure how to proceed anyway. Maybe we should have a component to make it easier to drop off once Antd starts to have accessible tooltips?

Tooltips are sort of a pain I will try to handle separately. In the meantime I'm uncertain whether I should provide a minimal approach to them or wait until they can be dealt with. Probably I'll just remove these changes.

wenbow · 2021-03-16T17:40:04Z

The high level bullet points definitely make a lot of sense. The aria tag looks great. Wrt the antd or our design system, I will defer to our awesome engs. :)

gabrieldutra · 2021-03-16T21:00:56Z

client/app/components/ApplicationArea/ApplicationLayout/VersionInfo.jsx

@@ -15,7 +15,7 @@ export default function VersionInfo() {
          {/* eslint-disable react/jsx-no-target-blank */}
          <Link href="https://version.redash.io/" className="update-available" target="_blank" rel="noopener">
            Update Available <i className="fa fa-external-link m-l-5" aria-hidden="true" />
-            <span className="sr-only">&#40;External link&#41;</span>
+            <span className="sr-only">{"(External link)"}</span>


Sorry, I may have been not that clear since I also put the quotes, but what I mean is: does this need this at all? Can't it be just <span className="sr-only">(External link)</span>?

No, it can't. I changed back bc I thought this way was clearer.

Why it can't? HTML/jsx wise there's nothing that holds you from writing it directly (we do it in several other places in the codebase). As for screen-readers I don't really see how having it escaped would make a difference 🤔

I'm just curious because this really looks weird to me

For some reason it didn't work on my initial tests but now it seems fine. Thanks for pointing this, just reverted it.

rafawendel · 2021-03-17T01:10:07Z

client/app/components/BigMessage.jsx

@@ -3,7 +3,7 @@ import PropTypes from "prop-types";

 function BigMessage({ message, icon, children, className }) {
  return (
-    <div className={"big-message p-15 text-center " + className}>
+    <div className={"big-message p-15 text-center " + className} aria-live="assertive">


Oriented by this and the notification guide.

gabrieldutra · 2021-03-17T02:21:26Z

client/app/pages/queries/components/QueryPageHeader.jsx

-            <Button data-test="QueryPageHeaderMoreButton">
-              <EllipsisOutlinedIcon rotate={90} />
+            <Button data-test="QueryPageHeaderMoreButton" aria-label="More actions">
+              <EllipsisOutlinedIcon rotate={90} aria-label="" />


May be a newbie question as I'm no expert with those, but what's aria-label="" used for? Why not aria-hidden="true"?

The intention here is to make sure Ant's aria-labels will not be displayed, so as to avoid confusion. Theoretically aria-hidden does the trick, but I wanted to override it in some cases either way. Not a lot to back me up on this, though.

Just checked with a real SR. It does the trick.

client/app/components/ApplicationArea/ApplicationLayout/VersionInfo.jsx

client/app/assets/less/inc/generics.less

client/app/components/ApplicationArea/ApplicationLayout/VersionInfo.jsx

client/app/components/BigMessage.jsx

gabrieldutra · 2021-03-19T20:55:20Z

client/app/components/EmailSettingsWarning.jsx

@@ -16,16 +15,18 @@ export default function EmailSettingsWarning({ featureName, className, mode, adm
  }

  const message = (
-    <span>
+    <span id="sr-mail-description">


Same thing about the id here

This one is actually Ok, because it's not being replicated in a way that could end up rendering two on the screen.

I think, in general though, it might be best to generate ids for accessibility. It prevents us from colliding as well as relying on them in ways we shouldn't (e.g. using document.getElementById in a different component, or in a test).

gabrieldutra · 2021-03-19T21:04:15Z

client/app/components/ParameterApplyButton.jsx


  return (
    <div className="parameter-apply-button" data-show={!!paramCount} data-test="ParameterApplyButton">
+      {/* TODO: This button stays detectable even when invisible */}


Same here, but I'd actually favor just creating Follow Up issues to track these instead of putting TODOs in the application (they are usually forsaken).

I've been doing both, particularly because I intend to tackle these in the near future and it helps me keeping it in mind. Are you in favor of not keeping the comments for a particular reason?

My reason is that they bring little or no value. Even more when it is about new features as they always have a "room to be improved". The result is that we start to have multiple comments in the codebase that are not relevant to who's reading the code as they won't even be aware of the context 🤷‍♂️.

That said, I'm against TODO comments in the code. There are a couple of exceptions I consider for them however:

When you actually want to index your files with some easily searchable term (e.g.: we used ANGULAR_REMOVE_ME to track any of the still dependent leftovers while migrating away from it)

When the TODO is actually relevant to anyone that will read the code (I can't think about an example for this now 😕)

Anyway, this is more of an opinion, maybe someone else has a different thought...

client/app/components/queries/QueryEditor/AutocompleteToggle.jsx

client/app/pages/dashboards/components/DashboardHeader.jsx

gabrieldutra · 2021-03-22T00:54:02Z

client/app/components/BigMessage.jsx


 function BigMessage({ message, icon, children, className }) {
+  const messageId = uniqueId("bm-message");


This one has to be in a useRef:

const messageIdRef = useRef(); if (!messageIdRef.current) { messageIdRef.current = uniqueId("bm-message"); }

otherwise it will regenerate the id on every re-render.

Also in the other component (EmailSettingsWarning) as it is not specific to a a page (it could be used anywhere), maybe it's safer to do the same thing in there too?

Maybe just:

Suggested change

const messageId = uniqueId("bm-message");

const { current: messageId } = useRef(uniqueId("bm-message"));

This partially solves the thing as it will execute uniqueId regardless. Problem with this is that iirc, Lodash uses some sort of global var to track and sum up the ids. (We would end up with some possibly big ones 😝)

In brief, that was to follow this as that will avoid the component id to be updated, but not the Lodash reference to the "next id"

thielium

Aside from the messageId ref conversation, just nits! Way to go! This is a huge step forward!

thielium · 2021-03-22T16:15:11Z

client/app/components/BigMessage.jsx


 function BigMessage({ message, icon, children, className }) {
+  const messageId = uniqueId("bm-message");


Maybe just:

Suggested change

const messageId = uniqueId("bm-message");

const { current: messageId } = useRef(uniqueId("bm-message"));

thielium · 2021-03-22T16:15:49Z

client/app/components/BigMessage.jsx

+      aria-live="assertive"
+      aria-relevant="additions removals">
+      <h3 className="m-t-0 m-b-0" aria-labelledby={messageId}>
+        <i className={"fa " + icon} aria-hidden="true" />


Do or not: could use classnames, not a big deal.

thielium · 2021-03-22T16:17:28Z

client/app/components/EmailSettingsWarning.jsx

@@ -16,16 +15,18 @@ export default function EmailSettingsWarning({ featureName, className, mode, adm
  }

  const message = (
-    <span>
+    <span id="sr-mail-description">


I think, in general though, it might be best to generate ids for accessibility. It prevents us from colliding as well as relying on them in ways we shouldn't (e.g. using document.getElementById in a different component, or in a test).

thielium · 2021-03-22T16:19:18Z

client/app/components/HelpTrigger.jsx

+                    <>
+                      {" "}
+                      <i className="fa fa-external-link" style={{ marginLeft: 5 }} aria-hidden="true" />
+                      <span className="sr-only">(opens in a new tab)</span>


Feels like this is getting repeated, some. I don't have a great idea for abstracting it, yet, but it feels like it maybe could be.

Coming soon 😉

thielium · 2021-03-22T17:05:28Z

client/app/components/dashboards/dashboard-widget/VisualizationWidget.jsx

-            <i className={cx("zmdi zmdi-refresh", { "zmdi-hc-spin": refreshClickButtonId === 1 })} />{" "}
+            <i className={cx("zmdi zmdi-refresh", { "zmdi-hc-spin": refreshClickButtonId === 1 })} aria-hidden="true" />
+            <span className="sr-only">
+              {refreshClickButtonId === 1 ? "Refreshing, please wait. " : "Press to refresh. "}


Do or not: feels like some duplication, here -- if you feel like you can abstract it, go for it! But if you think it could create business logic changes, don't worry about it.

client/app/components/groups/ListItemAddon.jsx

thielium · 2021-03-22T17:16:26Z

client/app/pages/alert/components/NotificationTemplate.jsx

@@ -94,7 +94,8 @@ function NotificationTemplate({ alert, query, columnNames, resultValues, subject
            data-test="CustomBody"
          />
          <HelpTrigger type="ALERT_NOTIF_TEMPLATE_GUIDE" className="f-13">
-            <i className="fa fa-question-circle" /> Formatting guide
+            <i className="fa fa-question-circle" aria-hidden="true" /> Formatting guide
+            <span className="sr-only"> (help)</span>


(nit) Extra space?

gabrieldutra · 2021-03-22T19:54:21Z

client/app/lib/hooks/useLazyRef.ts

+  return lazyRef as React.MutableRefObject<T>;
+}
+
+export default useLazyRef;


nit: prefer the named export export function useLazyRef(...)

gabrieldutra · 2021-03-22T19:55:16Z

client/app/lib/hooks/useUniqueId.ts

+  return id;
+}
+
+export default useUniqueId;


same thing about the named export 🙂

gabrieldutra

Thanks for the work and patience here @rafawendel 🐳

rafawendel marked this pull request as draft March 12, 2021 19:29

rafawendel force-pushed the improve-icon-a11y branch from e9d66ee to d576ff7 Compare March 12, 2021 19:40

rafawendel changed the title ~~Added screen reader CSS~~ Improve icon a11y Mar 15, 2021

rafawendel force-pushed the improve-icon-a11y branch from 9c30078 to ff9b50e Compare March 15, 2021 20:03

rafawendel self-assigned this Mar 15, 2021

rafawendel added the Frontend label Mar 15, 2021

rafawendel marked this pull request as ready for review March 15, 2021 20:06

rafawendel requested review from thielium and gabrieldutra March 15, 2021 21:01

gabrieldutra reviewed Mar 16, 2021

View reviewed changes

rafawendel requested a review from gabrieldutra March 16, 2021 20:47

gabrieldutra reviewed Mar 16, 2021

View reviewed changes

rafawendel commented Mar 17, 2021

View reviewed changes

gabrieldutra reviewed Mar 17, 2021

View reviewed changes

rafawendel requested a review from gabrieldutra March 17, 2021 16:26

thielium reviewed Mar 18, 2021

View reviewed changes

client/app/components/ApplicationArea/ApplicationLayout/VersionInfo.jsx Show resolved Hide resolved

gabrieldutra reviewed Mar 19, 2021

View reviewed changes

rafawendel requested a review from gabrieldutra March 21, 2021 23:47

gabrieldutra reviewed Mar 22, 2021

View reviewed changes

thielium approved these changes Mar 22, 2021

View reviewed changes

rafawendel requested a review from gabrieldutra March 22, 2021 19:44

rafawendel added 8 commits March 22, 2021 16:50

Added screen reader CSS

552a806

Added description to external links

6f6d02d

Added spinner icon accessibility

15de05e

Added accessibility to exclamation and big message

344ac1d

Added question and exclamation accessibility

f342397

Hide decorative icons

f9954d5

Standardized link design

ef95de9

Added a11y to refresh icons

95cc8f5

rafawendel added 18 commits March 22, 2021 16:50

Added aria-label to anchors and buttons

9afa8ad

Added a11y to conditional icons

f44ad0a

Added applicable labels to Ant Icons

ff08b47

Changed escape to interpolation

9a22ac3

Replaced external links with opens in new tab

c871cbe

Improved Tooltip hosts

c5fa094

Added aria live to temporary elements

7dab395

Removed mistakenly added redundant helper

2cf46e5

Undoes unnecessarily added interpolation

fa816db

Replaced empty label with hidden

33f7eee

Improved full icon label

aa111a3

Improved display of live regions

7b88834

Added note

d9dbf85

remove unused class

e602055

Created unique id

1bf06d0

Remove TODOs

b031b6b

Proper action label

87e2dfa

Improved feedback for autocomplete toggle

1180bc8

gabrieldutra reviewed Mar 22, 2021

View reviewed changes

rafawendel added 3 commits March 22, 2021 17:04

feature: add id hook

9e44edf

refactor: use id hook

3bdbb24

standardize white space

ee8ebb3

rafawendel force-pushed the improve-icon-a11y branch from b96692d to ee8ebb3 Compare March 22, 2021 20:08

gabrieldutra approved these changes Mar 22, 2021

View reviewed changes

rafawendel merged commit fb90b50 into master Mar 22, 2021

rafawendel deleted the improve-icon-a11y branch March 23, 2021 18:31

rafawendel mentioned this pull request Mar 23, 2021

Replace <a> and <button> with <PlainButton> #5433

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve icon a11y #5424

Improve icon a11y #5424

rafawendel commented Mar 12, 2021 •

edited

Loading

gabrieldutra Mar 16, 2021

rafawendel Mar 16, 2021

wenbow commented Mar 16, 2021

gabrieldutra Mar 16, 2021 •

edited

Loading

rafawendel Mar 16, 2021

gabrieldutra Mar 17, 2021

rafawendel Mar 17, 2021

rafawendel Mar 17, 2021 •

edited

Loading

gabrieldutra Mar 17, 2021

rafawendel Mar 17, 2021 •

edited

Loading

rafawendel Mar 17, 2021

gabrieldutra Mar 19, 2021

rafawendel Mar 19, 2021

thielium Mar 22, 2021

gabrieldutra Mar 19, 2021

rafawendel Mar 19, 2021

gabrieldutra Mar 19, 2021 •

edited

Loading

rafawendel Mar 21, 2021

gabrieldutra Mar 22, 2021 •

edited

Loading

thielium Mar 22, 2021

gabrieldutra Mar 22, 2021

gabrieldutra Mar 22, 2021 •

edited

Loading

thielium left a comment

thielium Mar 22, 2021

thielium Mar 22, 2021

thielium Mar 22, 2021

thielium Mar 22, 2021

rafawendel Mar 22, 2021

thielium Mar 22, 2021

thielium Mar 22, 2021

gabrieldutra Mar 22, 2021

gabrieldutra Mar 22, 2021

gabrieldutra left a comment


		function BigMessage({ message, icon, children, className }) {
		const messageId = uniqueId("bm-message");

	const messageId = uniqueId("bm-message");
	const { current: messageId } = useRef(uniqueId("bm-message"));

Improve icon a11y #5424

Improve icon a11y #5424

Conversation

rafawendel commented Mar 12, 2021 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Screenshots

Note for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenbow commented Mar 16, 2021

gabrieldutra Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafawendel Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafawendel Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

thielium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrieldutra left a comment

Choose a reason for hiding this comment

rafawendel commented Mar 12, 2021 •

edited

Loading

gabrieldutra Mar 16, 2021 •

edited

Loading

rafawendel Mar 17, 2021 •

edited

Loading

rafawendel Mar 17, 2021 •

edited

Loading

gabrieldutra Mar 19, 2021 •

edited

Loading

gabrieldutra Mar 22, 2021 •

edited

Loading

gabrieldutra Mar 22, 2021 •

edited

Loading