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

bug: ensure Announce components work without pre-existing text node #5568

Merged
merged 6 commits into from
Jan 22, 2025

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Jan 17, 2025

Remaining TODOs:

  • add test coverage. is jest sufficient, or do we want storybook interactions? is there a way to thoroughly test live regions aside from seeing if element got populated? update: jest sufficient.
  • there's no guidance/docs page for this component. do we want one? (can follow-up separately)
  • add before/after videos with screen reader

relates to: #5566, https://github.com/github/accessibility-audits/issues/10031, https://github.com/github/accessibility-audits/issues/10017

Right now, announcements are not being triggered when the announce components (Announcement, AriaStatus, AriaAlert) go from being empty, to having text. See linked issue for broken examples.

The announcements do work in instances where these components have pre-existing text that gets updated.

Upon placing a debugger, I noticed we're not hitting the MutationObserver's callback in the failing scenario (where we go from empty to no text). This is because we need to set childList: true to detect when text nodes are added/removed. The current config, subtree: true and characterData: true, will only detect when existing text nodes are updated.

Set to true to monitor the target node (and, if subtree is true, its descendants) for the addition of new child nodes or removal of existing child nodes. The default value is false.

Changelog

Changed

  • Ensures that announcement are triggered when content is added to Announce, AriaAlert, AriaStatus without pre-existing text.

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

Copy link

changeset-bot bot commented Jan 17, 2025

🦋 Changeset detected

Latest commit: 83be114

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

Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jan 17, 2025
@github-actions github-actions bot temporarily deployed to storybook-preview-5568 January 17, 2025 22:21 Inactive
Copy link
Contributor

github-actions bot commented Jan 17, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 104.47 KB (-0.05% 🔽)
packages/react/dist/browser.umd.js 104.76 KB (-0.1% 🔽)

Copy link

@Pjrich1313 Pjrich1313 left a comment

Choose a reason for hiding this comment

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

Rerun debug run none

@github-actions github-actions bot temporarily deployed to storybook-preview-5568 January 21, 2025 22:03 Inactive
@khiga8 khiga8 marked this pull request as ready for review January 21, 2025 22:05
@khiga8 khiga8 requested a review from a team as a code owner January 21, 2025 22:05
@khiga8 khiga8 requested a review from francinelucca January 21, 2025 22:05
Copy link
Member

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

@khiga8 khiga8 added this pull request to the merge queue Jan 22, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 22, 2025
@khiga8 khiga8 enabled auto-merge January 22, 2025 16:55
@khiga8 khiga8 added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit 42c20dd Jan 22, 2025
42 checks passed
@khiga8 khiga8 deleted the kh-fix-5566 branch January 22, 2025 17:08
@primer primer bot mentioned this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants