Skip to content
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

Autocomplete: Fix Voiceover not announcing suggestions #54902

Merged
merged 19 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/autocompleters/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ function createBlockCompleter() {
return {
key: `block-${ blockItem.id }`,
value: blockItem,
textLabel: title,
label: (
<>
<BlockIcon
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
- `SlotFill`: Pass `Component` instance to unregisterSlot ([#54765](https://github.com/WordPress/gutenberg/pull/54765)).
- `Button`: Remove `aria-selected` CSS selector from styling 'active' buttons ([#54931](https://github.com/WordPress/gutenberg/pull/54931)).
- `Popover`: Apply the CSS in JS styles properly for components used within popovers. ([#54912](https://github.com/WordPress/gutenberg/pull/54912))
- `Autocomplete`: Add `aria-live` announcements for Mac and IOS Voiceover to fix lack of support for `aria-owns` ([#54902](https://github.com/WordPress/gutenberg/pull/54902)).

### Internal

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/autocomplete/autocompleter-ui.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export function getAutoCompleterUI( autocompleter: WPCompleter ) {
} ) => (
<Component
id={ listBoxId }
role="listbox"
role="combobox"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

className="components-autocomplete__results"
>
{ items.map( ( option, index ) => (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ export default function getDefaultUseItems( autocompleter: WPCompleter ) {
label: autocompleter.getOptionLabel(
optionData
),
textLabel:
autocompleter.getOptionTextLabel(
optionData
),
keywords: autocompleter.getOptionKeywords
? autocompleter.getOptionKeywords(
optionData
Expand Down
59 changes: 53 additions & 6 deletions packages/components/src/autocomplete/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import {
isCollapsed,
getTextContent,
} from '@wordpress/rich-text';
import { speak } from '@wordpress/a11y';
import { isAppleOS } from '@wordpress/keycodes';

/**
* Internal dependencies
Expand All @@ -39,6 +41,35 @@ import type {
WPCompleter,
} from './types';

const getNodeText = ( node: React.ReactNode ): string => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 '';
Comment on lines +49 to +70
Copy link
Contributor

@jeryj jeryj Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
Comment on lines +44 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could simplify a bit.

Suggested change
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 '';
}
};


const EMPTY_FILTERED_OPTIONS: KeyedOption[] = [];

export function useAutocomplete( {
Expand Down Expand Up @@ -163,19 +194,35 @@ export function useAutocomplete( {
) {
return;
}

let newIndex = 0;
alexstine marked this conversation as resolved.
Show resolved Hide resolved

switch ( event.key ) {
case 'ArrowUp':
setSelectedIndex(
newIndex =
( selectedIndex === 0
? filteredOptions.length
: selectedIndex ) - 1
);
: selectedIndex ) - 1;
setSelectedIndex( newIndex );
if ( isAppleOS() ) {
speak(
filteredOptions[ newIndex ].textLabel ||
getNodeText( filteredOptions[ newIndex ].label ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a place where the textLabel and getNodeText( filteredOptions[ newIndex ].label ) differed. Could we rely only on getNodeText() and remove the textLabel option from the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeryj We could try but trying to parse text out of objects I thought could be tricky. I knew this would work fine for Core but third-party usages? Thoughts?

'assertive'
);
}
break;

case 'ArrowDown':
setSelectedIndex(
( selectedIndex + 1 ) % filteredOptions.length
);
newIndex = ( selectedIndex + 1 ) % filteredOptions.length;
setSelectedIndex( newIndex );
if ( isAppleOS() ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to have a link to this PR and a brief comment explaining why we need to use speak on appleOS. Let's do it in a follow-up though, so we don't need to retrigger all the tests since they're currently passing.

speak(
filteredOptions[ newIndex ].textLabel ||
getNodeText( filteredOptions[ newIndex ].label ),
'assertive'
);
}
break;

case 'Escape':
Expand Down
5 changes: 5 additions & 0 deletions packages/components/src/autocomplete/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ describe( 'AutocompleterUI', () => {
{ option.name }
</span>
),
getOptionTextLabel: ( option: FruitOption ) => {
return option.name;
},
// Mock useItems function to return a autocomplete item.
useItems: ( filterValue: string ) => {
const options = autocompleter.options;
Expand All @@ -46,6 +49,8 @@ describe( 'AutocompleterUI', () => {
key: `${ autocompleter.name }-${ optionIndex }`,
value: optionData,
label: autocompleter.getOptionLabel( optionData ),
textLabel:
autocompleter.getOptionTextLabel( optionData ),
keywords: [],
isDisabled: false,
} )
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/autocomplete/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export type KeyedOption = {
key: string;
value: any;
label: OptionLabel;
textLabel?: string;
keywords: Array< string >;
isDisabled: boolean;
};
Expand Down Expand Up @@ -68,6 +69,11 @@ export type WPCompleter< TCompleterOption = any > = {
* string or a mixed array of strings, elements, and components.
*/
getOptionLabel: ( option: TCompleterOption ) => OptionLabel;
/**
* A function that returns the text label for a given option. A text label may be a
* string only.
*/
getOptionTextLabel: ( option: TCompleterOption ) => string;
/**
* A function that takes a Range before and a Range after the autocomplete
* trigger and query text and returns a boolean indicating whether the
Expand Down
Loading