Utilize aria-describedby on all ActionList descriptions
#4666
Conversation
…o `TrailingVisual`
🦋 Changeset detectedLatest commit: cf00630 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
TylerJDev
added
the
update snapshots
github-actions
bot
removed
the
update snapshots
|
@primer/engineer-reviewers - Could I get a review on this PR? 👀 |
| 'aria-describedby': slots.blockDescription | ||
| ? [blockDescriptionId, inactiveWarningId].join(' ') | ||
| : inactiveWarningId, | ||
| 'aria-labelledby': `${labelId} ${slots.trailingVisual ? trailingVisualId : ''}`, |
There was a problem hiding this comment.
- 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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Should now only include the proposed changed for TrailingVisual!
There was a problem hiding this comment.
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)
Co-authored-by: Siddharth Kshetrapal <[email protected]>
…ved-labels-descriptions-actionlist' into tylerjdev/add-improved-labels-descriptions-actionlist
| [slots.blockDescription ? blockDescriptionId : undefined, inactiveWarningId ?? undefined] | ||
| .filter(String) | ||
| .join(' ') | ||
| .trim() || undefined, |
There was a problem hiding this comment.
Optional: Don't think we need trim now that we are filtering strings
There was a problem hiding this comment.
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 🤔
| <a | ||
| aria-current="page" | ||
| aria-labelledby=":r2:--label " | ||
| aria-labelledby=":r2:--label " |
There was a problem hiding this comment.
Could me make the filtering string on aria-labelledby as well so that we don't get these extra spaces 😅
There was a problem hiding this comment.
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!
TylerJDev
deleted the
tylerjdev/add-improved-labels-descriptions-actionlist
branch
* 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 <[email protected]> * changes from PR feedback * Update .changeset/lovely-days-march.md * Update snapshots * Update lovely-days-march.md --------- Co-authored-by: Siddharth Kshetrapal <[email protected]>
* 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 <[email protected]> * 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 <[email protected]> * chore: add package version numbers (#4796) Co-authored-by: Josh Black <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * changes from PR feedback * Update .changeset/lovely-days-march.md * Update snapshots * Update lovely-days-march.md --------- Co-authored-by: Siddharth Kshetrapal <[email protected]> * Wrap `header` and `footer` with `<Box>` * Move `<Box>` * Add back focus to story * Update behaviors package * Move back `useFocusTrap` --------- Co-authored-by: Siddharth Kshetrapal <[email protected]> Co-authored-by: Josh Black <[email protected]> Co-authored-by: Josh Black <[email protected]> Co-authored-by: Katie Langerman <[email protected]> Co-authored-by: langermank <[email protected]>
Closes https://github.com/github/primer/issues/2501, https://github.com/github/primer/issues/3471
Changelog
Changed
aria-labelledbytoActionList.TrailingVisualRollout strategy
Testing & Reviewing
Merge checklist