[WIP] Support prop to announce banner when it is shown #2583

khiga8 · 2024-02-09T20:44:41Z

This is a proof of concept, and has a pre-req to ship:

Add live-region-element package with a PVC wrapper #2625
- <live-region> should be part of the page layout of the consuming application. We probably want a PVC wrapper for this likePrimer::Alpha::LiveRegion, and have guidance that nudges consumers to add it to their page layout.

What are you trying to accomplish?

When banners are shown async to communicate feedback, we should either be focusing or announcing the banner content to draw attention to it. Read more at Banner docs.

This is a proof of concept to support a announce_on_show prop that, when set, announces the banner content upon it's visibility being updated by the consumer.

This should support a live region announcement being made across the following scenarios:

The banner is rendered as hidden. Then the consumer uses JS to unhide the banner. At this point, we want the banner to be announced when announce_on_show: true. (Test written ✅ )
A parent of the banner is set to hidden (e.g. an entire container). Then the consumer uses JS to unhide the container. At this point, we want the banner to be announced when announce_on_show: true is set. (Test written ✅ )
The banner gets dynamically injected into the DOM via JS. At this point, we want the banner to be announced when announce_on_show: true. (Needs test ❌ Any tips for how to go about dynamically injecting a view component in our system tests? )

IMPORTANT 🔈

Dynamically injected live region attributes don't work reliably so we cannot just set aria-live on the Banner and expect it to announce when the Banner appears. See Staff only: Challenges with dynamically injected live region.

This approach depends on hooking into live-region-element which is ideally already part of the page layout. As called out in the beginning, there is pre-req work necessary to support live-region. Consumer needs to add this to their page layout.

Integration

This depends on <live-region> to be part of the page layout of the repos it is called in.

List the issues that this change affects.

Relates to: https://github.com/github/accessibility/issues/5649

Risk Assessment

Low risk the change is small, highly observable, and easily rolled back.
Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2024-02-09T20:44:44Z

⚠️ No Changeset found

Latest commit: 0d4d517

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

app/components/primer/alpha/x_banner.ts

demo/app/views/layouts/component_preview.html.erb

khiga8 · 2024-02-09T21:50:21Z

app/components/primer/alpha/x_banner.ts

+          }
+        }
+      }
+      this.observer = new IntersectionObserver(callback, options)


Looked into IntersectionObserver because it was mentioned in #2576 (comment).

I am expecting this to detect when the Banner's visibility changes from hidden to being visible.

Close review would be appreciated.

github-actions · 2024-02-20T21:02:52Z

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

khiga8 · 2024-02-21T16:32:59Z

app/components/primer/alpha/banner.html.erb

-      <p class="Banner-title" data-target="<%= catalyst_target(field: "titleText") %>"><%= content %></p>
-      <% if @description %>
-        <p><%= @description %></p>
+    <span data-target="<%= catalyst_target(field: "contentToAnnounce") %>">


The dismiss button should not be announced.

khiga8 · 2024-02-21T16:34:18Z

app/components/primer/alpha/x_banner.ts

+// eslint-disable-next-line import/no-unresolved
+import '@primer/live-region-element/define'


How to fix this? Also, I noticed we don't need to make this call in other custom elements since it's exported as part of the index. Wonder if we should do the same for live-region?

khiga8 · 2024-02-21T17:10:28Z

test/system/alpha/banner_test.rb

+
+      assert_selector("#wrapper", visible: :visible)
+      assert_equal page.evaluate_script("document.querySelector('live-region').shadowRoot.querySelector('[aria-live=\"polite\"]').innerText.trim()"), "Hello."
+    end


I want to add a test for when Primer::Alpha::Banner is dynamically injected. How should I set this up?

khiga8 temporarily deployed to preview February 9, 2024 20:44 — with GitHub Actions Inactive

khiga8 commented Feb 9, 2024

View reviewed changes

app/components/primer/alpha/x_banner.ts Outdated Show resolved Hide resolved

khiga8 commented Feb 9, 2024

View reviewed changes

demo/app/views/layouts/component_preview.html.erb Outdated Show resolved Hide resolved

khiga8 temporarily deployed to preview February 9, 2024 21:34 — with GitHub Actions Inactive

khiga8 changed the title ~~[WIP] Proof of concept to announce banner once upon showing~~ [WIP] Support prop to announce banner when it is shown Feb 9, 2024

khiga8 requested review from joshblack and lindseywild February 9, 2024 21:42

khiga8 commented Feb 9, 2024

View reviewed changes

khiga8 temporarily deployed to preview February 10, 2024 01:04 — with GitHub Actions Inactive

khiga8 mentioned this pull request Feb 20, 2024

Docs: update accessibility section of PVC Banner #2620

Merged

10 tasks

khiga8 added 6 commits February 20, 2024 17:50

Proof of concept to announce banner once upon showing

c7d4b10

Add live region to previews

3393a97

Add comments

05d0b4d

Don't announce the dismiss button

bace12a

Uncomment live-region-element

4c9c5ff

Add live-region-element package

89f1c73

khiga8 force-pushed the kh-banner-live-region branch from 7882d28 to 89f1c73 Compare February 20, 2024 17:52

khiga8 had a problem deploying to preview February 20, 2024 17:52 — with GitHub Actions Error

Uncomment require

8e5214c

khiga8 had a problem deploying to preview February 20, 2024 17:56 — with GitHub Actions Failure

Install dependency

6afb573

khiga8 temporarily deployed to preview February 20, 2024 20:56 — with GitHub Actions Inactive

khiga8 force-pushed the kh-banner-live-region branch from bc05bc6 to ecb54a9 Compare February 21, 2024 14:32

khiga8 temporarily deployed to preview February 21, 2024 14:32 — with GitHub Actions Inactive

khiga8 force-pushed the kh-banner-live-region branch from 26f8f67 to aeb1eba Compare February 21, 2024 15:26

khiga8 had a problem deploying to preview February 21, 2024 15:26 — with GitHub Actions Error

khiga8 had a problem deploying to preview February 21, 2024 15:27 — with GitHub Actions Error

Update examples

8c0e6d1

khiga8 force-pushed the kh-banner-live-region branch from 3021350 to 8c0e6d1 Compare February 21, 2024 15:29

khiga8 temporarily deployed to preview February 21, 2024 15:30 — with GitHub Actions Inactive

update tests

4f6ee15

khiga8 force-pushed the kh-banner-live-region branch from cb0eec6 to 4f6ee15 Compare February 21, 2024 16:28

khiga8 had a problem deploying to preview February 21, 2024 16:28 — with GitHub Actions Error

khiga8 temporarily deployed to preview February 21, 2024 16:30 — with GitHub Actions Inactive

khiga8 commented Feb 21, 2024

View reviewed changes

Fix regressed visual test

2c0b982

khiga8 force-pushed the kh-banner-live-region branch from 9702766 to 2c0b982 Compare February 21, 2024 17:07

khiga8 temporarily deployed to preview February 21, 2024 17:07 — with GitHub Actions Inactive

khiga8 commented Feb 21, 2024

View reviewed changes

khiga8 and others added 2 commits February 21, 2024 17:11

Generating component snapshots

e1a5a70

Merge branch 'main' into kh-banner-live-region

0d4d517

khiga8 deployed to preview February 21, 2024 17:19 — with GitHub Actions View deployment

khiga8 mentioned this pull request Feb 21, 2024

Add live-region-element package with a PVC wrapper #2625

Closed

10 tasks

joshblack removed their request for review April 12, 2024 20:03

khiga8 closed this May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Support prop to announce banner when it is shown #2583

[WIP] Support prop to announce banner when it is shown #2583

khiga8 commented Feb 9, 2024 •

edited

Loading

changeset-bot bot commented Feb 9, 2024 •

edited

Loading

khiga8 Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024 •

edited

Loading

khiga8 Feb 21, 2024

khiga8 Feb 21, 2024

khiga8 Feb 21, 2024

		// eslint-disable-next-line import/no-unresolved
		import '@primer/live-region-element/define'

[WIP] Support prop to announce banner when it is shown #2583

[WIP] Support prop to announce banner when it is shown #2583

Conversation

khiga8 commented Feb 9, 2024 • edited Loading

What are you trying to accomplish?

IMPORTANT 🔈

Integration

List the issues that this change affects.

Risk Assessment

Merge checklist

changeset-bot bot commented Feb 9, 2024 • edited Loading

⚠️ No Changeset found

khiga8 Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2024 • edited Loading

⚠️ Visual differences found

khiga8 Feb 21, 2024

Choose a reason for hiding this comment

khiga8 Feb 21, 2024

Choose a reason for hiding this comment

khiga8 Feb 21, 2024

Choose a reason for hiding this comment

khiga8 commented Feb 9, 2024 •

edited

Loading

changeset-bot bot commented Feb 9, 2024 •

edited

Loading

khiga8 Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024 •

edited

Loading