[WB-1891]: Integrating Announcer into SingleSelect and MultiSelect #2495
Conversation
…r ARIA Live Regions
…announcements in SingleSelect and MultiSelect
🦋 Changeset detectedLatest commit: bf3193a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Size Change: +257 B (+0.26%) Total Size: 101 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (8504deb) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2495"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2495 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-dqbwvbivds.chromatic.com/ Chromatic results:
|
| @@ -159,7 +159,7 @@ describe("Announcer.announceMessage", () => { | |||
| expect(message1Region).toHaveTextContent(message1); | |||
|
|
|||
| expect(setTimeout).toHaveBeenNthCalledWith( | |||
| 1, | |||
| 2, | |||
There was a problem hiding this comment.
Is there a reason why setTimeout is called twice now? Would be helpful to add a comment here so we remember why!
There was a problem hiding this comment.
Yes, I'll add a comment! This was due to the leading edge / trailing edge change in the debounce function. I think the first setTimeout is the initial one for Safari timing, so this configurable one in the test with specific duration/etc. comes second.
| @@ -161,6 +161,7 @@ describe("Announcer class", () => { | |||
| const waitThreshold = 1000; | |||
|
|
|||
| // Act | |||
| // The second call will win out in the trailing edge implementation | |||
There was a problem hiding this comment.
Great job setting up these tests with the initial implementation! Makes it easy to see what's changed with the updated behaviour 😄
There was a problem hiding this comment.
Would it be helpful to document the announcer behaviour for SingleSelect and MultiSelect? We have accessibility pages for both!
| @@ -316,6 +324,10 @@ const SingleSelect = (props: Props) => { | |||
| }); | |||
| const hasError = error || !!errorMessage; | |||
|
|
|||
| React.useEffect(() => { | |||
| initAnnouncer(); | |||
There was a problem hiding this comment.
This is great that we get to test out how the announcer is used by other things! I have a few questions 😄
- Do all of the consumers of the announcer have to initialize the announcer?
- Would there be any issues if a consumer didn't initialize the announcer? Would there be any issues if the announcer is initialized multiple times? (left a related comment in the initAnnouncer function!)
- Could we call
initAnnouncerwhenannounceMessageis used when the announcer isn't initialized yet? I'm wondering ifinitAnnouncershould be private to the announcer package so consumers don't have to worry about setting it up!- If we need to initialize the announcer even earlier, could we initialize the announcer in a central place so that we don't have to initialize it when we use it? Maybe in something like
RenderStateRootor something similar? I'm not sure the best place to put it! I was thinking ofRenderStateRootbecause it's high up in the render tree (webapp example)!
- If we need to initialize the announcer even earlier, could we initialize the announcer in a central place so that we don't have to initialize it when we use it? Maybe in something like
There was a problem hiding this comment.
Good questions! All consumers don't have to initialize the Announcer, as it already initializes itself when you call announceMessage. There were two reasons I added this initAnnouncer option:
- To make the debounceThreshold (and potentially other options) configurable in tests when you aren't handling the public Announcer functions directly (like in
SingleSelecttests, where it's called internally) - To improve reliability on the first call in VoiceOver and Safari -- which I should definitely document if we keep it in there! It injects the live regions earlier so that screen readers can pick them up every time.
The initAnnouncer function would have to be public in order for consumers to call it.
Initializing it in a central place on page load also makes a lot of sense to me, assuming it doesn't have a negative impact on performance or something. I can look into initializing it in RenderStateRoot in my webapp integration work! It would always return the same Announcer instance no matter how it's initialized, so we could probably do both.
There was a problem hiding this comment.
Sounds good! For context, RenderStateRoot is a WB Core component! We have a storybook decorator to wrap our stories with the RenderStateRoot component. Looking at the RenderStateRoot implementation more, maybe we should have a different component to initialize the announcer so we can separate the concerns! Or we can call initAnnouncer directly when Storybook or the app first mounts!
There was a problem hiding this comment.
Ooh, thanks for this! I actually might need it for some target node customization I want to do in Storybook. I need to inject the Announcer into a more precise place within the docs examples so it doesn't overlap story content when shown visually. It's also helping me look more closely at the lifecycle of the initAnnouncer function, as it appears to be called much later than I intended. I'll check this out!
| export function initAnnouncer(props?: InitAnnouncerProps): Announcer { | ||
| const announcer = Announcer.getInstance(); | ||
| if (props?.debounceThreshold !== undefined) { | ||
| announcer.updateWaitThreshold(props?.debounceThreshold); |
There was a problem hiding this comment.
Since there can only be one instance of the announcer, could it cause issues if different consumers called initAnnouncer with different debounceThreshold values? The last thing to call initAnnouncer would override the previous value
There was a problem hiding this comment.
Yes, that's a good point. It could change on every call.. but I think that is what you would want. Maybe we could play with it more in webapp and see if that becomes an issue?
packages/wonder-blocks-dropdown/src/components/__tests__/multi-select.test.tsx
Outdated
Show resolved
Hide resolved
| @@ -1724,9 +1745,15 @@ describe("MultiSelect", () => { | |||
| await userEvent.click(textbox); | |||
| await userEvent.paste("ear"); | |||
|
|
|||
| // wait to avoid getting caught in the Announcer debounce | |||
| jest.advanceTimersByTime(250); | |||
There was a problem hiding this comment.
Is 250 a specific number that needs to be used? I see we also advance timers by 10!
I wonder if it would be helpful to add some docs around how to write unit tests for things using the announcer so other teams know how to create tests for it! Things like how to look for the announcer in the DOM, how long to advance timers, and any other internal details that would impact how it gets tested. Or maybe we could provide a testing utility to encapsulate these details? Just a thought!
There was a problem hiding this comment.
I love that idea! The 250 number is the default debounceThreshold. The 10 number came up when I was using a 0 value in tests... setTimeouts with a value of 0 weren't quite immediate in the JavaScript event loop so I had to advance Jest timers 10! I can write a doc on testing this so the testing path is more well-trodden.
There was a problem hiding this comment.
Thinking about this more, do want consumers of the announcer to mock out the announcer instead so they don't have to worry about the implementation details? Their tests could be around making sure the announcer is called with the correct message! Then the announcer's unit tests are what makes sure it's putting the content in the live region correctly. What do you think?
There was a problem hiding this comment.
That's a great suggestion. I'd love to make the tests easier to work with, and mocking could be a really helpful way to do that!
|
|
||
| if (openerStringValue) { | ||
| // opener value changed, so let's announce it | ||
| handleAnnouncement(openerStringValue); |
There was a problem hiding this comment.
If I'm understanding correctly, this will announce the message on every render, even when the opener value hasn't changed (for example, every time the dropdown is opened/closed). Would it make sense to only announce it when the opener value changes? We could do that by calling the announcer in a useEffect with selectedValues, or we could call it whenever we trigger the onChange event prop for the MultiSelect! What do you think?
There was a problem hiding this comment.
I debated on whether to gate this with opener value changes, and ultimately kept it this way because of the bugs in Safari and VoiceOver. It will always push an announcement of this particular value to counteract the buggy clipped and stale value that is announced in VO regardless of the visual state of it!
I also played with setting state for these values and using useEffect and got thrown into an infinite loop. This was the only way I could get it to work with the amount of re-rendering going on.
| const menuContent = getMenuTextOrNode(allChildren); | ||
| const menuTextOrNode = getMenuTextOrNode(allChildren); | ||
| const [openerStringValue, openerContent] = | ||
| maybeExtractStringFromNode(menuTextOrNode); |
There was a problem hiding this comment.
To simplify things, could we use the getLabel utility directly to get the announcement message? It looks like we updated getMenuTextOrNode and added maybeExtractStringFromNode to get the openerStringValue, which looks to always be the return value from getLabel!
Let me know if I'm missing something though! It always takes me longer to understand the menu opener with all the flexibility it supports 😅
There was a problem hiding this comment.
I wish we could... unless I'm missing something, getLabel returns a node sometimes that is the wrong type for a live region (which requires a string). I needed a string label consistently, so that's why I had to maybeExtractStringFromNode.
There was a problem hiding this comment.
getLabel will always return a string 😄
wonder-blocks/packages/wonder-blocks-dropdown/src/util/helpers.ts
Lines 62 to 72 in 78c3535
getSelectOpenerLabel returns a string or node!
There was a problem hiding this comment.
Oh yes, you're right! Drowning in complexity a bit here... I'll test it out! Simplification is always a good idea when we can do it. That empty string case at the end might be an issue, but I think it was also foundational in my more complicated logic anyway!
There was a problem hiding this comment.
Ah, I remember now... it's the complexity from getMenuTextOrNode in MultiSelect that made this difficult, as there are multiple cases for selected items in a switch statement that return a string OR a node. I wanted to leverage that logic without having to iterate through it all again!
There was a problem hiding this comment.
Ahh gotcha, that makes sense!
| @@ -607,6 +631,10 @@ const MultiSelect = (props: Props) => { | |||
| const filteredItems = getMenuItems(allChildren); | |||
| const isDisabled = numEnabledOptions === 0 || disabled; | |||
|
|
|||
| if (open && isFilterable) { | |||
| handleAnnouncement(labels.someSelected(filteredItems.length)); | |||
There was a problem hiding this comment.
Would it make sense to trigger this announcement when an event occurs or when the data changes?
There was a problem hiding this comment.
I looked into that too...this code was the biggest culprit in triggering infinite loops as it's in the main component thread. The filtered items aren't tracked with state, so I'd have to find a safe way to do that. The change event for this doesn't do any filtering on children. I could try moving it back into dropdown-core, but I hoped to keep all announcement handling centralized in SingleSelect and MultiSelect, respectively. I'm open to suggestions as this was really tricky!
There was a problem hiding this comment.
Ahh I see, this is what you were mentioning the other day! I'll poke around and get back to you 😄
There was a problem hiding this comment.
Tinkered around a bit in SingleSelect and was able to reduce the announcement to only when the selectedValue/children changes and when the filtered items length changes! https://github.com/Khan/wonder-blocks/compare/combobox-announcer-v2...combobox-tinkering?expand=1 Let me know what you think :)
(Note: I duplicated some logic to figure out the selectedOption within the useEffect to avoid additional renders. We could memoize that logic since other things use that logic too!)
There was a problem hiding this comment.
Thanks so much for this Bea!! I appreciate you taking the time to tinker with it. I'm testing it out in Safari and VO and it is working so far... I'll keep playing with it and a MultiSelect version.
Summary:
Integrating WB Announcer into SingleSelect and MultiSelect for updating combobox values. This work is intended to work around VoiceOver/Safari bugs where the value isn't read out properly in an opener.
Issue: WB-1891
Test plan: