Autocomplete: Fix Voiceover not announcing suggestions #54902
Conversation
alexstine
added
[Focus] Accessibility (a11y)
| @@ -25,6 +25,7 @@ export type KeyedOption = { | |||
| key: string; | |||
| value: any; | |||
| label: OptionLabel; | |||
| textLabel: string; | |||
There was a problem hiding this comment.
Is there a better way to go about this? Had to add textLabel because label could contain a raw React object. Not sure if it is worth trying to figure out how to parse text from HTML but sounds difficult.
There was a problem hiding this comment.
Have we tried using label.textContent ?
We could extract the text by doing something like
const getNodeText = node => {
if (['string', 'number'].includes(typeof node)) return node
if (node instanceof Array) return node.map(getNodeText).join('')
if (typeof node === 'object' && node) return getNodeText(node.props.children)
}
const textLabel = getNodeText( label );(I haven't tested this myself, just a thought).
More in general, I'd avoid introducing new mandatory props (textLabel is marked as non-optional), since this will break all existing usages of the component
There was a problem hiding this comment.
@ciampo I found that same code in Stack Overflow. Might be worth a try.
More in general, I'd avoid introducing new mandatory props (textLabel is marked as non-optional), since this will break all existing usages of the component
In general, is it never allowed to have breaking changes? This is really important to get right for accessibility. This wasn't a problem before the iFrame and React portal but now its too late in the game to fix it? There's got to be a better way. I'll try the suggestion above but really interested in this point.
There was a problem hiding this comment.
Personally, I'd love to be able to introduce breaking changes, given that we do it sparingly and that we communicate them appropriately. But the WordPress community (and in particular some vocal members) are very much against this practice — in their opinion, once some code becomes part of a WordPress release, it should stick to the "no breaking changes" WordPress policy.
There are also different kinds of breaking changes. With the code currently proposed by this PR, for example, instances of Autocomplete not passing a textLabel for each of their options could incur in build errors (in case they're using TypeScript) and may incur in runtime error.
In case the approach that I suggested above was not viable, I would suggest to at least mark the textLabel prop as optional, and to tweak the code to call the speak() function only when a label is available
There was a problem hiding this comment.
Added a fallback. BTW, I did try label.textContent and TypeScript through an error about the property not existing.
There was a problem hiding this comment.
I did try label.textContent and TypeScript through an error about the property not existing.
I guess that's because label is a ReactNode, and therefore there could be instances in which it wouldn't have a textContent property.
Anyway, I see that you've update the code, at a first glance it looks good!
|
Size Change: +125 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Early notes:
- The keycodes package has a similar helper for detecting Apple devices -
isAppleOS. We can probably copy that. - The new helper can probably placed in
componentspackages instead of making it public API viaelement. - By the way, the
elementpackage already has aplatform.jsfile.
|
@Mamaduka Can the keycodes package also detect if IOS? I need both for this PR. I can add it to the components package but thought it might be worth exposing it in case there is a need to have a sane OS check that works. More work to come on this... |
I believe they do.
We can do that separately. Let's keep the scope of this PR small. It makes it easier to review and land. |
|
Okay, resolved all feedback so far. This is still working well. |
This comment was marked as off-topic.
This comment was marked as off-topic.
| @@ -129,7 +129,7 @@ export function getAutoCompleterUI( autocompleter: WPCompleter ) { | |||
| } ) => ( | |||
| <Component | |||
| id={ listBoxId } | |||
| role="listbox" | |||
| role="combobox" | |||
There was a problem hiding this comment.
Not sure how valid this is but it seems to actually work really well. Still need to test on Mac but it eliminates the re-rendering feeling. I did inspect with React Dev Tools and while there are some passed properties that do force some re-rendering, I think this issue was more related to the listbox role itself.
There was a problem hiding this comment.
Changing this to combobox doesn't seem to make any difference on Mac, I guess because VoiceOver has no idea what it's doing anyway! Either way, it's definitely much more vocal now, announcing each option as it's selected, and debouncing appropriately.
Academically speaking, I suppose it should be the controlling "paragraph" that temporarily gets a role change, while the popup should stay as a listbox. When I hacked that together very quickly, VoiceOver announced the following...
You are currently on a Menu pop-up combo box, inside a frame. Type text or, to display a list of choices, press Control-Option-Space.
That feels somehow more "correct", in terms of what it is semantically, though confusingly pressing Control+Option+Space doesn't do anything. Project for another time!
There was a problem hiding this comment.
@andrewhayward If I keep the role="listbox", every time I press Up or Down Arrow, it has this re-render effect. I think its because the passed selected index prop changes.
Voiceover, now that I think about it, won't work at all even with this change due to Apple not supporting aria-owns.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-owns
Warning: At the time of this writing, aria-owns is not supported on MacOS and iOS with VoiceOver.
I might try switching with aria-controls.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-controls
You should look at how we implemented the post author combo box if there are more than 25 users on the site. You can find that in the editor package, I think it uses Combobox component which wraps around part of FormTokenInput.
There was a problem hiding this comment.
Technically speaking, this change shouldn't affect how React re-renders items. As you said, there's probably a different change that triggers re-render.
There was a problem hiding this comment.
@andrewhayward Okay, after doing some more debugging, we could improve a little more. It would be possible to keep role="combobox" and role="listbox" on the correct elements if we could prevent listBox from re-rendering every time. Every time I press Up or Down Arrow keys, the AutocompleterUI triggers a re-render of the ListBox and React Dev Tools says the cause is:
Props changed: (selectedIndex, onChangeOptions, onSelect, reset)
Is it possible to useCallback or useMemo the onChangeOptions callback to only change if needed? Up or Down Arrow keys should only adjust the selectedIndex, not re-render the entire options tree.
There was a problem hiding this comment.
From what I can tell, the re-render is being caused by the changing props returned on the hook. Not sure how to go about fixing this so the suggestions list only re-renders when the filter is used. This doesn't appear to be a problem in the Combobox component. In theory, the props would simply update the existing component but it seems like the whole component is being replaced which also forces ListBox to re-render.
There was a problem hiding this comment.
Thank you for getting this important fix in for this release, @alexstine!
| @@ -39,6 +41,35 @@ import type { | |||
| WPCompleter, | |||
| } from './types'; | |||
|
|
|||
| const getNodeText = ( node: React.ReactNode ): string => { | |||
There was a problem hiding this comment.
This function is only used once, but it would be useful for it to be its own exported function so we can have some unit tests for it. Given the release is coming up so quickly, I think an extra day of manual testing it "in the wild" is more important than delaying this for unit tests.
| switch ( typeof node ) { | ||
| case 'string': | ||
| case 'number': | ||
| return node.toString(); | ||
| break; | ||
| case 'boolean': | ||
| return ''; | ||
| break; | ||
| case 'object': { | ||
| if ( node instanceof Array ) { | ||
| return node.map( getNodeText ).join( '' ); | ||
| } | ||
| if ( 'props' in node ) { | ||
| return getNodeText( node.props.children ); | ||
| } | ||
| break; | ||
| } | ||
| default: | ||
| return ''; | ||
| } | ||
|
|
||
| return ''; |
There was a problem hiding this comment.
We can remove the breaks that are preceded by a return since that code won't ever be run. The final return can be removed as well since the default will get hit first, I believe. Thanks for @ajlende for catching that!
We can do that after this is merged though. I'll open an issue about it.
| const getNodeText = ( node: React.ReactNode ): string => { | ||
| if ( node === null ) { | ||
| return ''; | ||
| } | ||
|
|
||
| switch ( typeof node ) { | ||
| case 'string': | ||
| case 'number': | ||
| return node.toString(); | ||
| break; | ||
| case 'boolean': | ||
| return ''; | ||
| break; | ||
| case 'object': { | ||
| if ( node instanceof Array ) { | ||
| return node.map( getNodeText ).join( '' ); | ||
| } | ||
| if ( 'props' in node ) { | ||
| return getNodeText( node.props.children ); | ||
| } | ||
| break; | ||
| } | ||
| default: | ||
| return ''; | ||
| } | ||
|
|
||
| return ''; | ||
| }; |
There was a problem hiding this comment.
This could simplify a bit.
| const getNodeText = ( node: React.ReactNode ): string => { | |
| if ( node === null ) { | |
| return ''; | |
| } | |
| switch ( typeof node ) { | |
| case 'string': | |
| case 'number': | |
| return node.toString(); | |
| break; | |
| case 'boolean': | |
| return ''; | |
| break; | |
| case 'object': { | |
| if ( node instanceof Array ) { | |
| return node.map( getNodeText ).join( '' ); | |
| } | |
| if ( 'props' in node ) { | |
| return getNodeText( node.props.children ); | |
| } | |
| break; | |
| } | |
| default: | |
| return ''; | |
| } | |
| return ''; | |
| }; | |
| const getNodeText = ( node: React.ReactNode ): string => { | |
| if ( node === null ) { | |
| return ''; | |
| } | |
| switch ( typeof node ) { | |
| case 'string': | |
| case 'number': | |
| return node.toString(); | |
| case 'object': { | |
| if ( node instanceof Array ) { | |
| return node.map( getNodeText ).join( '' ); | |
| } | |
| if ( 'props' in node ) { | |
| return getNodeText( node.props.children ); | |
| } | |
| return ''; | |
| } | |
| default: | |
| return ''; | |
| } | |
| }; |
|
I've triggered this to rerun the failed PHP jobs. I don't know why this PR would cause those to fail. |
…omplete-voiceover
|
I have already assigned the follow-up to myself and will get to work on it so it can go out in 6.4.1. |
SiobhyB
added
Backport to WP Minor Release
|
Following a discussion on Slack, I'm acknowledging the decision to include this fix in 6.4.1, rather than the main 6.4 release. Slack discussion for reference: https://wordpress.slack.com/archives/C02QB2JS7/p1697795881565639 |
* Add a function to return a text-only label. * Use already defined isAppleOS function from keycodes. Remove new OS functions from element. * Add fallback if textLabel is unavailable. * Try combobox role to get around annoying re-rendering type effect. * Changelog entry. * Revert Windows fix, too much scope creep. * Fix Changelog. * Remove diff artifact. * Remove mistaken files. * Add comment linking to PR. * Revert textLabel prop. # Conflicts: # packages/components/CHANGELOG.md
mikachan
removed
the
Backport to WP Minor Release
|
I just cherry-picked this PR to the 6.4-next-point-release branch to get it included in the next release: f2dea97 |
* Add a function to return a text-only label. * Use already defined isAppleOS function from keycodes. Remove new OS functions from element. * Add fallback if textLabel is unavailable. * Try combobox role to get around annoying re-rendering type effect. * Changelog entry. * Revert Windows fix, too much scope creep. * Fix Changelog. * Remove diff artifact. * Remove mistaken files. * Add comment linking to PR. * Revert textLabel prop. # Conflicts: # packages/components/CHANGELOG.md
This comment was marked as spam.
This comment was marked as spam.
What?
Fixes #47767
Why?
Stupid Voiceover still doesn't support
aria-activedescendant. Bugs going back for years, wish Apple would fix this.Inspired by: https://react-spectrum.adobe.com/blog/building-a-combobox.html
How?
speakfor IOS and Mac only.Testing Instructions
aria-liveregion.aria-live.Testing Instructions for Keyboard
Screenshots or screencast