Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
766855b
Add a function to return a text-only label.
alexstine Sep 28, 2023
3c82605
Use already defined isAppleOS function from keycodes. Remove new OS f…
alexstine Sep 28, 2023
700bb1c
Add fallback if textLabel is unavailable.
alexstine Sep 28, 2023
a6b45ab
Try combobox role to get around annoying re-rendering type effect.
alexstine Sep 29, 2023
98bbc1f
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine Oct 2, 2023
b43d95e
Changelog entry.
alexstine Oct 2, 2023
6372f1c
Code quality and refresh.
alexstine Oct 10, 2023
96464c6
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine Oct 14, 2023
7ecf361
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine Oct 18, 2023
7972225
Revert Windows fix, too much scope creep.
alexstine Oct 18, 2023
7681b88
Fix Changelog.
alexstine Oct 18, 2023
1e24a70
Remove diff artifact.
alexstine Oct 18, 2023
facbe98
Fix conflict.
alexstine Oct 19, 2023
8e7ba56
Merge branch 'fix/autocomplete-voiceover' of github.com:WordPress/gut…
alexstine Oct 19, 2023
307b78e
Merge branch 'trunk' into fix/autocomplete-voiceover
alexstine Oct 19, 2023
98a845b
Remove mistaken files.
alexstine Oct 19, 2023
6e91679
Add comment linking to PR.
alexstine Oct 19, 2023
fe36a90
Revert textLabel prop.
alexstine Oct 19, 2023
607c455
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine Oct 19, 2023
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
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
22 changes: 16 additions & 6 deletions packages/components/src/autocomplete/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
useState,
useRef,
useMemo,
isAppleDevice,
} from '@wordpress/element';
import { __, _n } from '@wordpress/i18n';
import { useInstanceId, useMergeRefs, useRefEffect } from '@wordpress/compose';
Expand All @@ -22,6 +23,7 @@ import {
isCollapsed,
getTextContent,
} from '@wordpress/rich-text';
import { speak } from '@wordpress/a11y';

/**
* Internal dependencies
Expand Down Expand Up @@ -163,19 +165,27 @@ export function useAutocomplete( {
) {
return;
}

let newIndex = 0;

switch ( event.key ) {
case 'ArrowUp':
setSelectedIndex(
newIndex =
( selectedIndex === 0
? filteredOptions.length
: selectedIndex ) - 1
);
: selectedIndex ) - 1;
setSelectedIndex( newIndex );
if ( isAppleDevice() ) {
speak( filteredOptions[ newIndex ].textLabel, 'assertive' );
}
break;

case 'ArrowDown':
setSelectedIndex(
( selectedIndex + 1 ) % filteredOptions.length
);
newIndex = ( selectedIndex + 1 ) % filteredOptions.length;
setSelectedIndex( newIndex );
if ( isAppleDevice() ) {
speak( filteredOptions[ newIndex ].textLabel, '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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ciampo ciampo Sep 28, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a fallback. BTW, I did try label.textContent and TypeScript through an error about the property not existing.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

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
20 changes: 20 additions & 0 deletions packages/element/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,10 @@ _Changelog_

`6.2.0` Introduced in WordPress core.

### isAppleDevice

Undocumented declaration.

### isEmptyElement

Checks if the provided WP element is empty.
Expand All @@ -280,6 +284,22 @@ _Returns_

- `boolean`: True when an element is considered empty.

### isIOS

Undocumented declaration.

### isIPad

Undocumented declaration.

### isIPhone

Undocumented declaration.

### isMac

Undocumented declaration.

### isValidElement

Checks if an object is a valid WPElement.
Expand Down
1 change: 1 addition & 0 deletions packages/element/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@ export * from './react';
export * from './react-platform';
export * from './utils';
export { default as Platform } from './platform';
export * from './platform-os';
export { default as renderToString } from './serialize';
export { default as RawHTML } from './raw-html';
45 changes: 45 additions & 0 deletions packages/element/src/platform-os.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright 2020 Adobe. All rights reserved.
* This file is licensed to you under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. You may obtain a copy
* of the License at http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under
* the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/

function testPlatform( re: RegExp ) {
return typeof window !== 'undefined' && window.navigator !== null
? re.test(
// eslint-disable-next-line dot-notation
window.navigator[ 'userAgentData' ]?.platform ||
window.navigator.platform
)
: false;
}

export function isMac() {
return testPlatform( /^Mac/i );
}

export function isIPhone() {
return testPlatform( /^iPhone/i );
}

export function isIPad() {
return (
testPlatform( /^iPad/i ) ||
// iPadOS 13 lies and says it's a Mac, but we can distinguish by detecting touch support.
( isMac() && navigator.maxTouchPoints > 1 )
);
}

export function isIOS() {
return isIPhone() || isIPad();
}

export function isAppleDevice() {
return isMac() || isIOS();
}