-
Notifications
You must be signed in to change notification settings - Fork 582
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
feat(live-region): add support for <live-region> element #4174
Conversation
|
size-limit report 📦
|
Note: looking into the deploy preview problem now to see what we can do. Unfortunately we directly wire things up to |
…er/react into feat/add-live-region-custom-element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, excellent work! I want to "approve", but I (unfortunately) don't have much experience with custom elements yet, and would love to get feedback from folks who do.
src/internal/components/LiveRegion/live-region-element/live-region-element.d.ts
Outdated
Show resolved
Hide resolved
It is clear and I've run into the challenges identified multiple times.
I love this approach. We could also explore React portals, but I think this is better.
Before looking at the source, my impression was that the hook's API design is very straightforward and intuitive. However, I found the following things to be a little confusing about the components:
After reviewing the PR, I think I can answer all of those questions. However, I still have these questions:
I feel good about it assuming that we're going to do some screen reader testing. I really think we should add some usage examples to Storybook. |
@mperrotti thanks so much for the review! Appreciate it a ton. Will respond to some of the questions below 👇
Hopefully never 😅 This is in place just so DataTable keeps working.
We could definitely use this style if we expose it in our public API. Right now it's an internal component which doesn't typically use that convention.
Definitely! I think it'd be incredibly helpful. Depending on how the feedback goes for this I would love to bring it to accessibility and see if this would be a good interop point for announcements at GitHub 🤞 Some of the other ones that I thought were great questions and will add them to the questions section in the PR 🔥
It can be helpful to think of Totally get that this is confusing naming-wise and we can definitely change that, maybe to
I think
LiveRegion is used to make sure that the live region element exists on the page before making any announcements to it. Otherwise, one will be dynamically created and added to the DOM. A lot of this framing definitely comes from the idea that we want the live region in the DOM before making announcements to it. Hopefully for most people they just interact with
Surprisingly no, it seems like it will work with a dynamic live region plus buffered announcements but will need more testing to make sure 🤞 |
Yea, that name change makes things a lot clearer 🙂 |
@mperrotti awesome, done! ✨ |
One quick update, the For more info, checkout the Status, Alert section above. |
@joshblack I like this, feels more "React-y" that what we're using already. We should discuss this with the accessibility team if we haven't already since it'll presumably replace the existing |
Closing as this work has moved out into: |
Overview
This is a proof of concept for a
live-region
custom element that is used in coordination with our components to provide accessible announcements in live regions.tl;dr
We'd love to make announcements to live regions without them being dropped. This PR adds in components, hooks, and a custom element to make the following API possible:
Problem
When making announcements through live regions, the container for a live region must exist in the document before the announcement in made. When this is not the case, the announcement will be dropped.
The core challenge for making announcements is when a component is rendered conditionally. In these scenarios, we cannot rely on a stable live region within the component. Instead, we would require one of the following approaches:
This PR introduces a direction for both approaches through a combination of components and hooks.
Approach
This PR introduces components, hooks, and a
<live-region>
custom element in order to solve the problems mentioned above.Note
This PR includes a change to the
Spinner
component. This change is illustrative of theStatus
API specifically and is not a proposal for the final API of this component.Components
LiveRegionProvider
andLiveRegion
are two internal components that are used to render and provide a<live-region>
for a tree of React components. These components can be used at the top-level of an application or can be used within a component.In the example above,
LiveRegionProvider
acts as a context provider that provideschildren
with access to alive-region
element rendered byLiveRegion
.The
Status
andAlert
components correspond therole="status"
androle="alert"
live regions. However, instead of injecting these containers into the DOM, these components announce their text content as a message in the corresponding live region.Hooks
useLiveRegion
anduseAnnounce
are two internal hooks used to either get or interact with thelive-region
element in context.useLiveRegion(): LiveRegionElement
provides access to thelive-region
in contextlive-region
element if one does not exist in contextuseAnnounce()
provides anannounce()
method to directly announce the messageuseEffect()
hooks<live-region>
custom elementThe
live-region
custom element is an attempt at using a Custom Element to provide a common interop point between Primer and github/github. This custom element mirror methods used upstream in github/github, specificallyannounce()
andannounceFromElement()
, and provides them from the element class directly.Detailed design
LiveRegionProvider, LiveRegion
The
LiveRegionProvider
component is used to provide alive-region
custom element tochildren
. TheLiveRegion
component renders thelive-region
and sets the context value to thatlive-region
.Within
LiveRegion
, we use declarative shadow DOM to make sure that the structure is present in the document before JS hydrates.Status, Alert
The
Status
andAlert
components are used for one-off messages when a component first renders. They correspond torole="status"
androle="alert"
regions and will create an announcement based on the text content of thechildren
passed to this component.At the moment, these components will not create announcements if their contents change.
These components accept some of the options from
AnnounceOptions
onLiveRegionElement
but do not allow changing thepoliteness
setting. By default,politeness
will map to the corresponding defaults for the role.useLiveRegion
This hook provides a reference to a
live-region
element. With this hook, components can interact with thelive-region
directly. However, there is an important practice to follow when using this hook. Specifically, the value of this hook should not be present inuseEffect()
or similar dependency arrays as these effects should not synchronize with changes to the identity of this element.Instead, the identity of
liveRegion
should be saved to a ref using the following pattern:This pattern de-couples the effect firing from the identity of
liveRegion
changing.As a result, it is preferable to use hooks like
useAnnounce
that abstracts this pattern.useAnnounce
The
useAnnounce()
hook provides a stableannounce()
method that mirrors the function fromLiveRegionElement#announce
. This stable reference makes it reliable within effects and, as a result, should not be listed in dependency arrays.<live-region>
designlive-region
is a custom element,LiveRegionElement
, that has the following shadow DOM structure:It provides two public methods,
announce()
andannounceFromElement()
, for live region announcements. The idea behind using a custom element is that it could provide a common interop point across Primer and GitHub since the element would be namedlive-region
and would expose consistent methods for announcements.Buffering
One technical note is that this element implements announcement buffering. Specifically, if
announce()
orannounceFromElement
are called before the element is connected (in the DOM) then they will wait until the element is in the DOM before being announced.This is a technical detail in order to support our conditional rendering challenge above. Specifically, we may call
announce()
on an element before it is technically in the DOM. Implementing this buffer allows us to make sure these messages do not get dropped.Bundling
We need to shim certain DOM globals that are used when authoring custom elements in a Node.js environment. As a result, this PR updates our rollup config to add
node
specific entrypoints that uses@lit-labs/ssr-dom-shim
to safely call this code during SSR.Feedback
This PR is a lot and for that I am sorry 😅 Trying to address this problem lead me to go very deep in this problem space. Let me know if there would be a better way to present this information!
I would love the following feedback on this PR:
Also feel free to leave other feedback if there is something missing above 😄
Questions
When should someone use
delayMs
? What are valid values for it?