Utilize `aria-describedby` on all `ActionList` descriptions #4666

TylerJDev · 2024-06-11T18:25:36Z

Closes https://github.com/github/primer/issues/2501, https://github.com/github/primer/issues/3471

Changelog

Changed

Added aria-labelledby to ActionList.TrailingVisual

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

…o `TrailingVisual`

changeset-bot · 2024-06-11T18:25:39Z

🦋 Changeset detected

Latest commit: cf00630

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

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 · 2024-06-11T18:28:46Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	95.85 KB (+0.1% 🔺)
packages/react/dist/browser.umd.js	96.11 KB (+0.05% 🔺)

…ctionlist

TylerJDev · 2024-07-12T20:53:24Z

@primer/engineer-reviewers - Could I get a review on this PR? 👀

…ctionlist

siddharthkp

Left some clarifying questions

siddharthkp · 2024-07-30T15:17:54Z

packages/react/src/ActionList/Item.tsx

-      'aria-describedby': slots.blockDescription
-        ? [blockDescriptionId, inactiveWarningId].join(' ')
-        : inactiveWarningId,
+      'aria-labelledby': `${labelId} ${slots.trailingVisual ? trailingVisualId : ''}`,


How do we decide if trailing visual should be in label or description?

Are there places in dotcom which would end up with the wrong label / description from this?

Re: moving inline description from label to description, can we create a different PR for that because I'm less confident about that change. (I brought inline description from aria-describedby to aria-label #1599 after an accessibility review 😅)

I see from https://github.com/github/primer/issues/2501#issuecomment-1674987399,

We should probably have all instances of this component be referenced to via aria-describedby, instead of only the block variant. This is because the content in description might be a bit verbose to be included in the accessible name, regardless of the variant used.

This is really old, so forgive me for missing the details, but I think inline description was assumed to be short and label-like, which block description is for longer and even multi-line text. It's not mentioned in the design docs, so I wonder if primer-designers have thoughts on this as well

How do we decide if trailing visual should be in label or description?

I've added it as a label via aria-labelledby instead of a description because if there's text content within trailing visual, it's usually short and connects directly to the main item's text (accessibl, e.g.

<ActionList> <ActionList.Item>Tokens Available <ActionList.TrailingVisual>8</ActionList.TrailingVisual> </ActionList.Item> </ActionList>

Accessible name would be: "Tokens Available 8".

I mainly based this on examples I've seen in the wild. If we added it as a description, there's a chance it might be ignored, so I think aria-labelledby is a fair bet.

Are there places in dotcom which would end up with the wrong label / description from this?

I'd say it's very unlikely, since any text within the trailing visual isn't attached to ActionList.Item's accessible name/description currently.

Re: moving inline description from label to description, can we create a different PR for that because I'm less confident about that change.

Yup that's fair! I think it's worth its own discussion to see if we'd want to switch it or not, as it definitely works both ways, as part of the accessible name, or as part of the description. I'll take that change out of this PR and create another so that we can discuss/see if we want to include it or not!

Should now only include the proposed changed for TrailingVisual!

I mainly based this on examples I've seen in the wild.

Does this also include within menus/selectpanels?

(Happy to go ahead with this, either of label vs description is better than aria-hidden i guess)

packages/react/src/ActionList/Item.tsx

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

.changeset/lovely-days-march.md

…ved-labels-descriptions-actionlist' into tylerjdev/add-improved-labels-descriptions-actionlist

siddharthkp

Left a few tidying tasks, otherwise looks ready!

Approving in advance :)

siddharthkp · 2024-07-31T13:01:59Z

packages/react/src/ActionList/Item.tsx

+        [slots.blockDescription ? blockDescriptionId : undefined, inactiveWarningId ?? undefined]
+          .filter(String)
+          .join(' ')
+          .trim() || undefined,


Optional: Don't think we need trim now that we are filtering strings

I added this because we'd get aria-describedby=" " if none of the values were true. With the trim() || undefined, it will remove the attribute altogether since it's just an empty string at that point 🤔

siddharthkp · 2024-07-31T13:03:02Z

packages/react/src/NavList/__snapshots__/NavList.test.tsx.snap

@@ -339,7 +339,7 @@ exports[`NavList renders a simple list 1`] = `
      >
        <a
          aria-current="page"
-          aria-labelledby=":r2:--label "
+          aria-labelledby=":r2:--label  "


Could me make the filtering string on aria-labelledby as well so that we don't get these extra spaces 😅

Yes, definitely! I forgot I turned on auto merge 🤣, I can follow up in one of the PRs that I'll make for one of the sev-3 issues!

* Add `aria-describedby` to inline description; add `aria-labelledby` to `TrailingVisual` * Update snapshots * Add changeset * Update snapshot * Update packages/react/src/ActionList/Item.tsx Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * changes from PR feedback * Update .changeset/lovely-days-march.md * Update snapshots * Update lovely-days-march.md --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

* Focus close button on second step * Remove `autofocus` prop * Add dependencies to focus trap * Add changeset * lint fix * Remove `body` from dependency * add state to Dialog example * chore(changeset): enter prerelease mode for v37 (#4789) * chore(changeset): enter pre-release mode for v37 * ci: remove snapshots when in pre mode * chore: add version info to all packages * Revert "chore: add version info to all packages" This reverts commit 4665bb3. * chore: update canary to remove pre.json when running --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> * Autocomplete: Only open menu on click (#4771) * Only open menu on click instead of just focus * Update tests * Add changeset * Fix typo * Set `openOnFocus` to `true` * Add test, move `onFocus` function * Update docs * Adjust changeset * Remove `useCallback` * Add deprecated notice * Prep for high contrast theme border-color changes (#4774) * borders * fallback * test(vrt): update snapshots * test color changes * alright * bump * test(vrt): update snapshots * more fixes * test(vrt): update snapshots * copy from main * test(vrt): update snapshots * Create fluffy-ravens-thank.md * snippy snaps * update seg control border * test(vrt): update snapshots * remove fallbacks * copy from main * test(vrt): update snapshots --------- Co-authored-by: langermank <langermank@users.noreply.github.com> * chore: add package version numbers (#4796) Co-authored-by: Josh Black <joshblack@users.noreply.github.com> * feat: add postcss-preset-primer (#4751) * feat: add postcss-preset-primer * docs: update usage snippet * chore: fix eslint rules and remove postcss-mixins --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> * Utilize `aria-describedby` on all `ActionList` descriptions (#4666) * Add `aria-describedby` to inline description; add `aria-labelledby` to `TrailingVisual` * Update snapshots * Add changeset * Update snapshot * Update packages/react/src/ActionList/Item.tsx Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * changes from PR feedback * Update .changeset/lovely-days-march.md * Update snapshots * Update lovely-days-march.md --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * Wrap `header` and `footer` with `<Box>` * Move `<Box>` * Add back focus to story * Update behaviors package * Move back `useFocusTrap` --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: Josh Black <joshblack@github.com> Co-authored-by: Josh Black <joshblack@users.noreply.github.com> Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com> Co-authored-by: langermank <langermank@users.noreply.github.com>

Add aria-describedby to inline description; add aria-labelledby t…

d8cd733

…o `TrailingVisual`

TylerJDev temporarily deployed to github-pages June 11, 2024 18:29 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 June 11, 2024 18:29 Inactive

TylerJDev added the update snapshots label Jun 12, 2024

github-actions bot removed the update snapshots label Jun 12, 2024

TylerJDev added 2 commits June 12, 2024 10:40

Update snapshots

377ef5d

Add changeset

6c00d25

TylerJDev temporarily deployed to github-pages June 12, 2024 14:47 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 June 12, 2024 14:47 Inactive

Merge branch 'main' into tylerjdev/add-improved-labels-descriptions-a…

26800fb

…ctionlist

TylerJDev marked this pull request as ready for review June 12, 2024 17:12

TylerJDev requested a review from a team as a code owner June 12, 2024 17:12

TylerJDev requested a review from camertron June 12, 2024 17:12

TylerJDev temporarily deployed to github-pages June 12, 2024 17:16 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 June 12, 2024 17:16 Inactive

Merge branch 'main' into tylerjdev/add-improved-labels-descriptions-a…

69a57f1

…ctionlist

TylerJDev temporarily deployed to github-pages June 21, 2024 17:53 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 June 21, 2024 17:53 Inactive

Merge branch 'main' into tylerjdev/add-improved-labels-descriptions-a…

a672887

…ctionlist

TylerJDev temporarily deployed to github-pages July 12, 2024 20:21 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 July 12, 2024 20:21 Inactive

Update snapshot

5fad1fd

TylerJDev temporarily deployed to github-pages July 12, 2024 20:35 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 July 12, 2024 20:36 Inactive

siddharthkp self-requested a review July 25, 2024 16:18

Merge branch 'main' into tylerjdev/add-improved-labels-descriptions-a…

c62806e

…ctionlist

TylerJDev temporarily deployed to github-pages July 29, 2024 14:02 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 July 29, 2024 14:02 Inactive

TylerJDev enabled auto-merge July 29, 2024 14:15

siddharthkp requested changes Jul 30, 2024

View reviewed changes

TylerJDev and others added 2 commits July 30, 2024 15:52

Update packages/react/src/ActionList/Item.tsx

3b4f8c0

Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>

changes from PR feedback

9dbc082

TylerJDev temporarily deployed to github-pages July 30, 2024 20:11 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 July 30, 2024 20:11 Inactive

TylerJDev commented Jul 30, 2024

View reviewed changes

.changeset/lovely-days-march.md Outdated Show resolved Hide resolved

Update .changeset/lovely-days-march.md

57a5034

TylerJDev temporarily deployed to github-pages July 30, 2024 20:23 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 July 30, 2024 20:23 Inactive

TylerJDev added 2 commits July 30, 2024 20:36

Update snapshots

0b11bc6

Merge remote-tracking branch 'refs/remotes/origin/tylerjdev/add-impro…

baf63b7

…ved-labels-descriptions-actionlist' into tylerjdev/add-improved-labels-descriptions-actionlist

TylerJDev temporarily deployed to github-pages July 30, 2024 21:00 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4666 July 30, 2024 21:00 Inactive

Update lovely-days-march.md

cf00630

siddharthkp approved these changes Jul 31, 2024

View reviewed changes

siddharthkp deployed to github-pages July 31, 2024 13:04 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4666 July 31, 2024 13:05 View deployment

TylerJDev added this pull request to the merge queue Jul 31, 2024

Merged via the queue into main with commit 04eac62 Jul 31, 2024
30 checks passed

TylerJDev deleted the tylerjdev/add-improved-labels-descriptions-actionlist branch July 31, 2024 13:25

primer bot mentioned this pull request Jul 31, 2024

Release tracking (37.0.0-rc.0) #4797

Merged

siddharthkp mentioned this pull request Sep 16, 2024

ActionList.Leading/TrailingVisual is invisible to screen readers #4908

Open

primer bot mentioned this pull request Oct 18, 2024

Release tracking (37.0.0) #5143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utilize `aria-describedby` on all `ActionList` descriptions #4666

Utilize `aria-describedby` on all `ActionList` descriptions #4666

TylerJDev commented Jun 11, 2024 •

edited

Loading

changeset-bot bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

TylerJDev commented Jul 12, 2024

siddharthkp left a comment

siddharthkp Jul 30, 2024 •

edited

Loading

TylerJDev Jul 30, 2024

TylerJDev Jul 30, 2024

siddharthkp Jul 31, 2024 •

edited

Loading

siddharthkp left a comment

siddharthkp Jul 31, 2024

TylerJDev Jul 31, 2024

siddharthkp Jul 31, 2024

TylerJDev Jul 31, 2024

Utilize aria-describedby on all ActionList descriptions #4666

Utilize aria-describedby on all ActionList descriptions #4666

Conversation

TylerJDev commented Jun 11, 2024 • edited Loading

Changelog

Changed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Jun 11, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 11, 2024 • edited Loading

size-limit report 📦

TylerJDev commented Jul 12, 2024

siddharthkp left a comment

Choose a reason for hiding this comment

siddharthkp Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

TylerJDev Jul 30, 2024

Choose a reason for hiding this comment

TylerJDev Jul 30, 2024

Choose a reason for hiding this comment

siddharthkp Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

siddharthkp left a comment

Choose a reason for hiding this comment

siddharthkp Jul 31, 2024

Choose a reason for hiding this comment

TylerJDev Jul 31, 2024

Choose a reason for hiding this comment

siddharthkp Jul 31, 2024

Choose a reason for hiding this comment

TylerJDev Jul 31, 2024

Choose a reason for hiding this comment

Utilize `aria-describedby` on all `ActionList` descriptions #4666

Utilize `aria-describedby` on all `ActionList` descriptions #4666

TylerJDev commented Jun 11, 2024 •

edited

Loading

changeset-bot bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

siddharthkp Jul 30, 2024 •

edited

Loading

siddharthkp Jul 31, 2024 •

edited

Loading