Allow Banner component to receive focus, adds optional `id` parameter

[lindseywild]

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans. Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

Note: I am mostly looking for feedback on this approach before I test it fully 🙏🏻

On the Accessibility team, we've discovered a few audit issues are marked as blocked by Primer. After more investigation, we realized this is not the case, and that in some scenarios, we want to move focus to the Banner component when a user takes an action. We can do this in dotcom with JS, as most consumers have logic to show/hide the banners based on certain conditions.

This PR adds a focusBanner() method to allow consumers to hook into to focus the banner in certain scenarios. The element we want to focus on in a Banner is subject to change, so this approach allows that element to be managed at the Primer level. It also allows an optional id parameter to be passed in which is applied to the <x-banner> element (currently passing in id as a system arg applies it to the nested <div> under x-banner, so calling a method on the ID won't work).

Integration

No

List the issues that this change affects.

• https://github.com/github/accessibility-audits/issues/5108
• https://github.com/github/accessibility-audits/issues/5090
• https://github.com/github/accessibility-audits/issues/5492

Risk Assessment

• [x] 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.

What approach did you choose and why?

I chose to add a method to add the tabindex and .focus() functionality to focus the banner. @khiga8 and I considered adding a parameter to the ruby component, such as focus_on_show, that would hide the banner by default, and when shown, it would focus the banner.

The reasons we didn't go with this approach right now (it may be a good thing to discuss in the future!):

• Consumers currently handle the logic for hiding/showing the banner. We'd need to build this in at the component-level and update instances where a consumer is showing/hiding their banner
• It'd be more work to implement on the dotcom consumer end
• It'd be a larger time commitment on our end and Primer's (for testing, reviewing, validating, etc)
• It'd be a more risky change

Anything you want to highlight for special attention from reviewers?

A consumer could use this new functionality by calling the method this way:

document.querySelector("#banner_id").focusBanner();

Is this an anti-pattern? Should we invest more time in allowing a consumer to do something like:

<%= render(Primer::Alpha::Banner.new(hidden: :true, focus_on_show: :true)) do %>
  Please update your billing information in order to add a payment method.
<% end %>

and then the component would be responsible for showing/hiding itself? I think we'd still need to have an event listener present to know when to show/hide the component... Open to other ideas here!

Accessibility

Allows the banner to be focusable, which will ensure screen reader announcements are consistent in certain scenarios when relevant

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• [ ] Added/updated previews (Lookbook)
• [ ] Tested in Chrome
• [ ] Tested in Firefox
• [ ] Tested in Safari
• [ ] Tested in Edge

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[bot]]

⚠️ No Changeset found

Latest commit: 3f1bac2f34b0af28f821c0e3ed36f0cc25ae460f

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

[keithamus]

<%= render(Primer::Alpha::Banner.new(hidden: :true, focus_on_show: :true)) do %>
  Please update your billing information in order to add a payment method.
<% end %>

This seems like a better idea to me. With an IntersectionObserver it would be quite straightforward to implement, and would avoid adding new public API to the web components, which I'd like to avoid unless there was a very compelling reason to do so.

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.