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.

Sign up for GitHub

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

Custom select control: don't announce external value changes #22815

Merged
merged 8 commits into from
Jun 18, 2020
Merged

Custom select control: don't announce external value changes #22815

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e8909cf
Don't announce external value change
tellthemachines Jun 2, 2020
d8b5ed7
Correct aria attributes
tellthemachines Jun 3, 2020
6c6297e
Improve value equality checking.
tellthemachines Jun 12, 2020
cd87011
Selected item doesn't always exist
tellthemachines Jun 12, 2020
e1c1869
Check if value exists and add default
tellthemachines Jun 12, 2020
1ae478b
Fix initial selection
tellthemachines Jun 15, 2020
914d485
Memoize getSelectOptions instead
tellthemachines Jun 17, 2020
5e980b0
Switch to useMemo
tellthemachines Jun 18, 2020
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
31 changes: 18 additions & 13 deletions packages/components/src/custom-select-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ const stateReducer = (
{ selectedItem },
{ type, changes, props: { items } }
) => {
const selectedItemIndex = items.findIndex(
( item ) => item.key === selectedItem?.key
);
if ( changes.highlightedIndex < 0 ) {
changes.highlightedIndex = selectedItemIndex;
}
switch ( type ) {
case useSelect.stateChangeTypes.ToggleButtonKeyDownArrowDown:
// If we already have a selected item, try to select the next one,
Expand All @@ -31,7 +37,7 @@ const stateReducer = (
items[
selectedItem
? Math.min(
items.indexOf( selectedItem ) + 1,
selectedItemIndex + 1,
items.length - 1
)
: 0
Expand All @@ -44,7 +50,7 @@ const stateReducer = (
selectedItem:
items[
selectedItem
? Math.max( items.indexOf( selectedItem ) - 1, 0 )
? Math.max( selectedItemIndex - 1, 0 )
: items.length - 1
],
};
Expand All @@ -58,8 +64,9 @@ export default function CustomSelectControl( {
label,
options: items,
onChange: onSelectedItemChange,
value: _selectedItem,
value,
} ) {
const valueIndex = items.findIndex( ( item ) => item.key === value?.key );
const {
getLabelProps,
getToggleButtonProps,
Expand All @@ -69,14 +76,12 @@ export default function CustomSelectControl( {
highlightedIndex,
selectedItem,
} = useSelect( {
initialSelectedItem: items[ 0 ],
initialSelectedItem: items[ valueIndex >= 0 ? valueIndex : 0 ],
items,
itemToString,
onSelectedItemChange,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onSelectedItemChange,
onSelectedItemChange,
selectedItem: valueIndex >= 0 ? items[ valueIndex ] : undefined

Downshift needs to be aware when the item really does change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid that's the one thing we can't do, because it causes Downshift to announce changes to the selection whenever the component re-renders. Apart from the minor issue of the highlight not always matching the selected item (which we may be able to fix by leveraging :focus styles), is there anything else that won't work with these changes? Otherwise I think we'll have to use a different library, or implement our own solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the component rerenders and value is referentially equal, then it shouldn't be announcing anything, right? I think the issue is that the value is being recreated on render.

Have you tried memoizing getSelectOptions in the font size picker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I haven't. But that still won't solve the problem of changes being announced whenever the value changes outside of the component, e.g. if I select a random value in the font size number input, it announces "custom has been selected".

Copy link
Contributor

Choose a reason for hiding this comment

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

I select a random value in the font size number input, it announces "custom has been selected".

It would only happen once when the value is switched to custom. Is that still worth avoiding?

Not passing Downshift the value means that when it changes elsewhere, Downshift won't know about it. If I understand how it works correctly, this means that all the prop getters will return props based on an outdated selected item. E.g., entering a custom value would change the rendered label because you are getting the value manually for that. Still, all the prop getters won't know about it internally so that they can return the relevant props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand how it works correctly, this means that all the prop getters will return props based on an outdated selected item.

Yeah, that's why the highlighting isn't working correctly when the value is updated elsewhere. With all the other props, I've managed to work around.

I'll try memoizing and if it solves the main issue, it's probably good enough for now. But we should revisit this later, as having that unexpected announcement when using the number input isn't really great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a prop to "silence" the announcement in those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have anything specific in mind? There doesn't seem to be a way of silencing the announcement in Downshift, and I can only think of extremely dodgy solutions to do it on our side 😅 (like removing Downshift's aria-live container or trying to override it with an empty speak - not sure that'd even work btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: memoizing getSelectOptions works for the announcement when switching editor modes, so that's the main issue solved ✅

Copy link
Contributor

Choose a reason for hiding this comment

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

(like removing Downshift's aria-live container or trying to override it with an empty speak - not sure that'd even work btw)

That was my line of thinking as well, haha.

works for the announcement when switching editor modes, so that's the main issue solved

Awesome!

selectedItem: _selectedItem,
stateReducer,
} );

const menuProps = getMenuProps( {
className: 'components-custom-select-control__menu',
'aria-hidden': ! isOpen,
Expand All @@ -85,13 +90,12 @@ export default function CustomSelectControl( {
// fully ARIA compliant.
if (
menuProps[ 'aria-activedescendant' ] &&
menuProps[ 'aria-activedescendant' ].slice(
0,
'downshift-null'.length
) === 'downshift-null'
menuProps[ 'aria-hidden' ] === true
) {
delete menuProps[ 'aria-activedescendant' ];
}
const selectedItemValue = value ? value : selectedItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const selectedItemValue = value ? value : selectedItem;
const selectedItemValue = selectedItem;


return (
<div
className={ classnames(
Expand Down Expand Up @@ -122,7 +126,7 @@ export default function CustomSelectControl( {
isSmall: true,
} ) }
>
{ itemToString( selectedItem ) }
{ itemToString( selectedItemValue ) }
<Icon
icon={ chevronDown }
className="components-custom-select-control__button-icon"
Expand All @@ -131,7 +135,7 @@ export default function CustomSelectControl( {
<ul { ...menuProps }>
{ isOpen &&
items.map( ( item, index ) => (
// eslint-disable-next-line react/jsx-key
// eslint-disable-next-line react/jsx-key, jsx-a11y/role-supports-aria-props
<li
{ ...getItemProps( {
item,
Expand All @@ -146,9 +150,10 @@ export default function CustomSelectControl( {
}
),
style: item.style,
'aria-selected': index === valueIndex,
} ) }
>
{ item === selectedItem && (
{ item.key === selectedItemValue.key && (
<Icon
icon={ check }
className="components-custom-select-control__item-icon"
Expand Down