Add a11y properties to icon component #646

brentswisher · 2023-11-13T13:09:21Z

This change: (check at least one)

Is this a breaking change? (check one)

Yes
No

Is the: (complete all)

Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
Test suite(s) passing?
Code coverage maximal?
Changeset added?
Component status page up to date?

What does this change address?
Adds both a a11y-title and and a11y-hidden property to the icon component. This is a replacement for the previous description which is now marked as deprecated and will be removed in the next major release (#467). It also replaces the use of aria-label with setting a title in the svg of the icon which after talking to @sirrah-tam is the preferred method.

How does this change work?
Adds the a11y-title and a11y-hidden property and maps them to a title and aria-hidden attribute on the icon svg. The previous functionality of aria-hidden automatically being set if no description being provided is preserved here to enable adding the new attributes without a breaking change.

Additional context
See #467 to track the breaking change that follows this in the next major release.

Added both a11y-label and a11y-hidden properties to the icon component. Also marks the previous icon property description as deprecated. This is in preparation of the v14 release which will remove the description propererty and add an error if either a11y-label or a11y-hidden are not provided.

changeset-bot · 2023-11-13T13:09:25Z

🦋 Changeset detected

Latest commit: 3d34847

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@ithaka/pharos	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-11-13T13:11:06Z

size-limit report 📦

Path	Size
packages/pharos/lib/index.js	53.26 KB (+0.29% 🔺)

The most accesssible way to label an svg icon is to use a title element in the svg itself. This is better than using aria-label which should be reserved for cases where an accessible name can not be derived. See this page for more details: https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title

Because aria-hidden is a boolean attribute, it should be set to true when the icon is hidden, and not exist at all when it is visible. Lit's nothing attribute enables this behavior: https://lit.dev/docs/templates/expressions/#removing-attribute This also updates the logic to add aria-hidden if there is not description or a11y-title to preserve the current behavior until description is removed.

If the title is not provided, fall back to the description. Previously, it only fell back if the a11y-title was not provided, not an empty string. This made the storybook stories fail.

packages/pharos/src/components/icon/pharos-icon.ts

daneah · 2023-11-29T13:51:09Z

packages/pharos/src/components/icon/pharos-icon.ts

    return html`
      <svg
        xmlns="http://www.w3.org/2000/svg"
        version="1.1"
        viewBox="0 0 ${size} ${size}"
        class="icon"
        role="img"
-        aria-hidden=${this.description === ''}
-        aria-label=${this.description || ''}
+        aria-hidden=${hideIcon || nothing}


I think there's better shorthand for this unless I'm missing something:

Suggested change

aria-hidden=${hideIcon || nothing}

?aria-hidden=${hideIcon}

I think this results in the attribute being present, but empty aria-hidden="" which is the same as aria-hidden="true" to a browser. https://lit.dev/docs/templates/expressions/#removing-attribute

I'll test that out though, would be nice if we didn't need to import nothing as it's not particularly clear what it does by its name.

Here are the relevant portion of the docs that I think mean the attribute will indeed be removed.

Okay, I think I might see what is happening. When using ?aria-hidden=${hideIcon} I get a bunch of failing unit tests from other components. That matched what I saw before when trying it. I had thought it was because it was not removing the aria-hidden attribute when hideIcon was false. The tests are actually failing because when hideIcon is set to true, it is rendering the svg as aria-hidden="" which the axe plugin we use doesn't seem to recognize as being hidden.

With the nothing syntax aria-hidden is either not present or set explicitly to aria-hidden="true" which axe does seem to recognize. I'll see if there is a bug report for axe about that or if it is expected and we can go from there, it does seem to be rendering correctly in the browser with either syntax.

A lil bit more research, I think that aria-hidden needs to have an explicit value set per the spec because it's a state and not a boolean attribute. So would need to be "true", "false", or undefined.

Reference: https://stackoverflow.com/questions/55061083/does-aria-hidden-need-a-value

This makes sense. @brentswisher @sirrah-tam do we want to make a11yHidden align with the aria-hidden spec for convergence/consistency's sake, and keep the rendering line here as-is?

Okay, I have updated this so that a11yHidden is a string attribute which allows the same arguments as aria-hidden, as defined by the new shared type.

The question now is, if someone sets a11y-hidden="false" Do we:

Render aria-hidden="false" on the svg

Not render aria-hidden at all? (Which this code does now)

@sirrah-tam is wondering if it will be confusing to consumers if it isn't rendered at all. I am on the fence, the WCAG spec specifically calls out some browser issues with setting it to "false" even thought that is the official API. Not sure which is more helpful to Pharos consumers, guardrails or fully matching the official aria API in the matching a11y attribute.

Curious your thoughts @daneah ?

The overlap of meaning of JavaScript boolean true|false values and undefined with the string values "true"|"false"|"undefined" makes this challenging. I note that in Button's pressed state we handle everything explicitly as a string, so that they're always truthy ("true"|"false"|"undefined" are all truthy, as strings). I feel like that is closest to authoring raw HTML as well; if what you type is what ends up in the browser, most attributes then become either boolean (that is, present or not) or string.

In Button we codified this even more directly for its pressed state; note that its type is 'true' | 'false' | 'undefined' | undefined (different than what you have). I feel that the spec is really forcing aria-hidden to be a thing whose value is really a string, which gets coerced to a boolean or to a "idk, let the browser decide" behavior. I would vote we stick to that because I'd rather people maintain a mental model of the web than add a layer of indirection and muddy understanding when using these. My two cents.

I have worked with Mat a bit to come up with this and believe the most recent code matches these expectations. I can also reuse the ariaHiddenState from the button PR once that gets merged into develop for consistency.

This is very helpful, thank you for documenting!

Co-authored-by: Dane Hillard <github@danehillard.com>

packages/pharos/src/components/icon/storyArgs.ts

packages/pharos/src/components/icon/pharos-icon.ts

Co-authored-by: Mat Harris <hurris88@gmail.com>

As we add a11y-* attributes as part of our accessibility work, we need a global place to store shared types that multiple components can use.

Because the end result of adding this attribute is setting a string value, it makes sense to have the prop be a string. The end result is that aria-hidden is either set to the string true, or not set at all.

brentswisher added status/not ready accessibility component/icon labels Nov 13, 2023

brentswisher self-assigned this Nov 13, 2023

brentswisher force-pushed the feature/add-icon-a11y-properties branch from 20b1eb7 to 4dd8bfb Compare November 27, 2023 22:00

brentswisher force-pushed the feature/add-icon-a11y-properties branch from 4dd8bfb to bf9bb0c Compare November 27, 2023 22:28

brentswisher added 4 commits November 28, 2023 16:35

fix(icon): fall back to description for title

5852799

If the title is not provided, fall back to the description. Previously, it only fell back if the a11y-title was not provided, not an empty string. This made the storybook stories fail.

docs(icon): use new a11y attributes in storybook

6e51328

fix(icon): correct attribute name in changeset

ff930b7

brentswisher added status/code review and removed status/not ready labels Nov 28, 2023

brentswisher marked this pull request as ready for review November 28, 2023 22:26

brentswisher requested a review from a team as a code owner November 28, 2023 22:26

brentswisher requested review from eslawski, jialin-he, mtorres3, sirrah-tam and daneah and removed request for a team and eslawski November 28, 2023 22:26

daneah reviewed Nov 29, 2023

View reviewed changes

fix(icon): Fix typo in deprecation warning

1d61003

Co-authored-by: Dane Hillard <github@danehillard.com>

sirrah-tam reviewed Nov 29, 2023

View reviewed changes

packages/pharos/src/components/icon/storyArgs.ts Outdated Show resolved Hide resolved

packages/pharos/src/components/icon/pharos-icon.ts Outdated Show resolved Hide resolved

packages/pharos/src/components/icon/pharos-icon.ts Outdated Show resolved Hide resolved

brentswisher and others added 2 commits November 29, 2023 11:38

fix(icon): Update description deprecation text

4c46526

Co-authored-by: Mat Harris <hurris88@gmail.com>

docs(icon): Update new a11yTitle property definition

8e631bc

Co-authored-by: Mat Harris <hurris88@gmail.com>

docs(icon): update title example in storybook

b008121

daneah mentioned this pull request Nov 29, 2023

Require descriptions for non-decorative icons without other visible descriptive text #467

Closed

brentswisher added 3 commits November 29, 2023 17:12

feat(a11y): add global types for aria values

58abaa3

As we add a11y-* attributes as part of our accessibility work, we need a global place to store shared types that multiple components can use.

feat(icon): update a11yHidden to be a string

df421ca

Because the end result of adding this attribute is setting a string value, it makes sense to have the prop be a string. The end result is that aria-hidden is either set to the string true, or not set at all.

fix(icon): update a11yHidden false logic

75e4c75

sirrah-tam approved these changes Dec 8, 2023

View reviewed changes

brentswisher added 2 commits December 8, 2023 11:16

Merge branch 'develop' into feature/add-icon-a11y-properties

e1002cf

Merge branch 'develop' into feature/add-icon-a11y-properties

030a040

brentswisher requested a review from daneah December 8, 2023 18:56

brentswisher requested a review from satya-achanta-venkata December 20, 2023 13:42

Merge branch 'develop' into feature/add-icon-a11y-properties

3f752c7

daneah approved these changes Jan 2, 2024

View reviewed changes

satya-achanta-venkata approved these changes Jan 2, 2024

View reviewed changes

shoupeva-ithaka approved these changes Jan 2, 2024

View reviewed changes

Merge branch 'develop' into feature/add-icon-a11y-properties

3d34847

brentswisher merged commit 05eca95 into develop Jan 12, 2024
11 checks passed

brentswisher deleted the feature/add-icon-a11y-properties branch January 12, 2024 14:00

This was referenced Jan 17, 2024

Internal icon instances are using deprecated syntax causing console warnings #659

Closed

Update all internal icon usages to use new a11y-hidden and a11y-title syntax #660

Merged

github-actions bot mentioned this pull request Jan 19, 2024

Version Packages #667

Merged

brentswisher mentioned this pull request Feb 2, 2024

Require icon accessibility attributes #679

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a11y properties to icon component #646

Add a11y properties to icon component #646

brentswisher commented Nov 13, 2023 •

edited

changeset-bot bot commented Nov 13, 2023 •

edited

github-actions bot commented Nov 13, 2023 •

edited

daneah Nov 29, 2023

brentswisher Nov 29, 2023 •

edited

daneah Nov 29, 2023

brentswisher Nov 29, 2023 •

edited

sirrah-tam Nov 29, 2023

daneah Nov 29, 2023

brentswisher Nov 29, 2023

daneah Nov 29, 2023

brentswisher Dec 8, 2023

daneah Jan 2, 2024

Add a11y properties to icon component #646

Add a11y properties to icon component #646

Conversation

brentswisher commented Nov 13, 2023 • edited

changeset-bot bot commented Nov 13, 2023 • edited

🦋 Changeset detected

github-actions bot commented Nov 13, 2023 • edited

size-limit report 📦

Choose a reason for hiding this comment

brentswisher Nov 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentswisher Nov 29, 2023 • edited

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

brentswisher commented Nov 13, 2023 •

edited

changeset-bot bot commented Nov 13, 2023 •

edited

github-actions bot commented Nov 13, 2023 •

edited

brentswisher Nov 29, 2023 •

edited

brentswisher Nov 29, 2023 •

edited