Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADR] docs: add live-regions adr #4611

Merged
merged 10 commits into from
Jun 28, 2024
Merged

[ADR] docs: add live-regions adr #4611

merged 10 commits into from
Jun 28, 2024

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented May 20, 2024

Live Regions

tl;dr this ADR will enable us in Primer React to conditionally render components that accurately announce to live regions 🥳 For example

function ExampleComponent() {
  const [loading, setLoading] = React.useState(true);
  if (loading) {
    return <AriaStatus>Loading</AriaStatus>
  }
  return <Content />
}

For more information about this ADR, check out the rendered view for the ADR.

Feedback

Would love to get your feedback on this approach and if it makes sense to try out more broadly in Primer React and GitHub 👀 This is definitely still experimental but it's been promising so far and we'd love to make use of it when building in Primer React and in GitHub directly.

Some broad questions to get things going:

  • What do you think of the usage of the custom element here? Does it make sense as a common interop point for Primer and GitHub?
  • Are there any alternatives that should be explored for accessible live region announcements?
  • What do you think of AriaStatus and AriaAlert? Are the semantics for these components clear along with how to use them?

Copy link

changeset-bot bot commented May 20, 2024

⚠️ No Changeset found

Latest commit: 176a9dd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot temporarily deployed to storybook-preview-4611 May 20, 2024 17:42 Inactive
@joshblack joshblack added the skip changeset This change does not need a changelog label May 20, 2024
Copy link
Contributor

github-actions bot commented May 20, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 91.03 KB (0%)
packages/react/dist/browser.umd.js 91.32 KB (0%)

Copy link
Contributor

@khiga8 khiga8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for writing this!

contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
joshblack and others added 2 commits May 20, 2024 15:38
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
contributor-docs/adrs/adr-020-live-regions.md Show resolved Hide resolved
Comment on lines +158 to +159
- What should happen if a component makes excessive announcements due to a
component that it renders? Is it possible to "group" announcements?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this one depends. Would we weigh this based on frequency of announcements, or repetitiveness? (e.g. announcing "loading" multiple times due to multiple spinners being rendered on the page at once).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great points, maybe both? Would be great to find a reference page that has this problem and see what we could do to make it obvious when something is firing a lot 😅

Thankfully some of these announcements will just be dropped if they are announced quickly but would be great to highlight the fundamental problem that is going on here.

One option down the road could be to create "grouping" live regions that could batch up messages. Basically something like:

<StatusGroup>
  <SomeComponent />
  <SomeComponent />
  <SomeComponent />
</StatusGroup>

Where SomeComponent renders Status. This grouping component could then collect messages and announce them together (or could expose an api to "summarize") but this is way down the line and may not even be useful 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's Rails, but if you want to see an example where this noise is pretty hard to cope with if you take a look at any PR on github/github with CI being processed it is incredibly noisy if you use MacOS and VoiceOver. This is because each Spinner has an aria-live region defined and when the spinner gets updated, because it's replaced in the DOM with a new spinner, it gets reannounced.

The only reason I can see that we're not getting lots of complaints about this is that I think it's much more common for screen reader users to be using Windows with NVDA or JAWS. Both of those screen readers only announce aria-live regions when they change but VoiceOver also announces on page load.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, scratch that! It looks like VoiceOver has fixed this so it behaves the same as other Screen Readers now 🤞

Although that does mean that the aria-live on these spinners is now doing nothing at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only places I've seen this pattern is with Spinner so I wonder if that's something that we can try to handle at that level 🤔

Vague idea for an API:

<Spinner announcement-id="my-id" announcement-template="{n} workflows loading" announcement-frequency="2000" />

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owenniblock's API suggestion is interesting. Would the props be the same for when we don't expect a bunch of announcements to be rendered? Or is this a special API for when consumers think we might build up a bunch of announcements?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mperrotti I hadn't really thought that far forward, just had an idea and threw it out there 😅

I guess those props would be optional and if there's no announcement-id then it assumes each item should be announced individually.

We could also split out the announcement into its own tag (something like):

<Announce  announcement-id="my-id" announcement-template="{n} workflows loading" announcement-frequency="2000">
   <Spinner />
</Announce>

Or maybe this API could form part of <AriaStatus> if that gains tractions.

@github-actions github-actions bot temporarily deployed to storybook-preview-4611 May 22, 2024 17:43 Inactive
@joshblack joshblack marked this pull request as ready for review May 22, 2024 17:46
@joshblack joshblack requested a review from a team as a code owner May 22, 2024 17:46
@joshblack
Copy link
Member Author

cc @primer/engineer-reviewers if you have a sec to check this out! Curious to hear what you all think about this approach 👀

contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Outdated Show resolved Hide resolved
Within `@primer/react`, we should lint against usage of `aria-live` and the
corresponding roles (if possible) and suggest using these alternatives instead.

> [!NOTE]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ Love this note! Great context.

Comment on lines +158 to +159
- What should happen if a component makes excessive announcements due to a
component that it renders? Is it possible to "group" announcements?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's Rails, but if you want to see an example where this noise is pretty hard to cope with if you take a look at any PR on github/github with CI being processed it is incredibly noisy if you use MacOS and VoiceOver. This is because each Spinner has an aria-live region defined and when the spinner gets updated, because it's replaced in the DOM with a new spinner, it gets reannounced.

The only reason I can see that we're not getting lots of complaints about this is that I think it's much more common for screen reader users to be using Windows with NVDA or JAWS. Both of those screen readers only announce aria-live regions when they change but VoiceOver also announces on page load.

Comment on lines +158 to +159
- What should happen if a component makes excessive announcements due to a
component that it renders? Is it possible to "group" announcements?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, scratch that! It looks like VoiceOver has fixed this so it behaves the same as other Screen Readers now 🤞

Although that does mean that the aria-live on these spinners is now doing nothing at all.

Comment on lines +158 to +159
- What should happen if a component makes excessive announcements due to a
component that it renders? Is it possible to "group" announcements?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only places I've seen this pattern is with Spinner so I wonder if that's something that we can try to handle at that level 🤔

Vague idea for an API:

<Spinner announcement-id="my-id" announcement-template="{n} workflows loading" announcement-frequency="2000" />

Co-authored-by: Owen Niblock <owenniblock@github.com>
Copy link
Contributor

@mperrotti mperrotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like the direction this is headed. I'm a little stumped on the best way to handle announcement grouping, but I'd be happy to participate in a pairing session to help us figure it out.


In order to have a common interop point for live region announcements, Primer React will
make use of a `live-region` custom element from `@primer/live-region-element`.
This package will be included and published from the `primer/react` repo.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're getting rid of @primer/live-region-element and moving it to this repo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically this would mean that it wouldn't live in it's own repo and instead would be under packages/live-region-element as another package in the monorepo 👀 It would still be published as the @primer/live-region-element package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that PVC consumers could also pull this element into their projects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! Definitely @khiga8

contributor-docs/adrs/adr-020-live-regions.md Show resolved Hide resolved
contributor-docs/adrs/adr-020-live-regions.md Show resolved Hide resolved
Co-authored-by: Mike Perrotti <mperrotti@github.com>
Copy link
Contributor

@khiga8 khiga8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! We can revisit the "Unresolved questions" as we learn along the way 🚀

@joshblack joshblack added this pull request to the merge queue Jun 28, 2024
Merged via the queue into main with commit 897033e Jun 28, 2024
30 checks passed
@joshblack joshblack deleted the docs/add-live-regions-adr branch June 28, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants