Custom select control: don't announce external value changes #22815
Conversation
tellthemachines
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta and
youknowriad
as code owners
tellthemachines
requested review from
epiqueras,
noisysocks,
talldan and
enriquesanchez
|
Size Change: -7.85 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
| } = useSelect( { | ||
| initialSelectedItem: items[ 0 ], | ||
| items, | ||
| itemToString, | ||
| onSelectedItemChange, | ||
| selectedItem: _selectedItem, | ||
| stateReducer, | ||
| } ); | ||
|
|
There was a problem hiding this comment.
Aren't there other a11y props that need to be set on the selected item? This won't let the hook do it for the initial state when using this in a controlled manner. It will think items[ 0 ] is always the selected item.
There was a problem hiding this comment.
Thanks for spotting that! I've fixed it so that all the attributes are in the correct places again, and all keyboard functionality is restored. The one thing I'm not super happy with is that, if a new value is set elsewhere, immediately reopening the dropdown will show the previous value incorrectly highlighted. I couldn't find a good workaround for that, but it seems to me less of an issue than what this PR is trying to fix 😅
Ultimately I think the best solution would be for Downshift to introduce an option to disable its aria-live announcements altogether, so we can use our custom ones and avoid clashes with other components (cc @silviuaavram would be great to have your opinion on this). Otherwise, if we find ourselves having to reimplement a ton of logic on our side, it might be easier to build our own. Happy to hear other ideas!
| @@ -22,6 +22,12 @@ const stateReducer = ( | |||
| { selectedItem }, | |||
| { type, changes, props: { items } } | |||
| ) => { | |||
| const selectedItemIndex = items.findIndex( | |||
| ( item ) => item.key === selectedItem.key | |||
There was a problem hiding this comment.
When are the items here not referentially equal? I ask because other code relies on it below.
There was a problem hiding this comment.
I haven't really looked at what Downshift is doing, but suspect they are creating a selectedItem object at some point, because these stopped being referentially equal when I removed _selectedItem from the arguments passed to useSelect. Where do you reckon this could cause breakage?
There was a problem hiding this comment.
There, for example. We should stick to one way of comparing values to avoid inconsistencies.
There was a problem hiding this comment.
Got it, fixed!
|
Ahh good catch @tellthemachines thanks! 🙂 Yeah, unnecessary re-renderings can produce very odd consequences for keyboard users and assistive technology. That's the reason why the accessibility team recommended since day 1 to avoid them as much as possible. The first recommendation dates to the day where a JS library for Gutenberg was being discussed more than 3 years ago 😄 |
There was a problem hiding this comment.
Hi @tellthemachines!
Tested on Safari + VO and it works great. I was able to hear the current mode announcement on several types of blocks, including Headings and Parahgraphs 👍
| } ) { | ||
| const valueIndex = items.findIndex( ( item ) => item.key === value.key ); |
There was a problem hiding this comment.
This will throw if the input is not controlled. value wouldn't be defined.
There was a problem hiding this comment.
Whoopsies, forgot about that!
| @@ -122,7 +124,7 @@ export default function CustomSelectControl( { | |||
| isSmall: true, | |||
| } ) } | |||
| > | |||
| { itemToString( selectedItem ) } | |||
| { itemToString( value ) } | |||
There was a problem hiding this comment.
This won't work if the value prop is not used.
| @@ -146,8 +148,9 @@ export default function CustomSelectControl( { | |||
| ), | |||
| style: item.style, | |||
| } ) } | |||
| aria-selected={ index === valueIndex } | |||
There was a problem hiding this comment.
I think we should pass this to getItemProps.
| > | ||
| { item === selectedItem && ( | ||
| { item.key === value.key && ( |
There was a problem hiding this comment.
Same as the other two usages of value. We need to use the selectedItem returned from useSelect.
There was a problem hiding this comment.
Hrm, because we're no longer telling useSelect about value updates, its selectedItem won't reflect the most recent value if that value is change elsewhere (as in the case of the multiple font-size setting options), so instead I'm checking whether a value has been set, and selecting the first item on the list if not (which I think would be expected behaviour in the case of an uncontrolled component).
There was a problem hiding this comment.
Ok, I've pushed my changes now 😄
There was a problem hiding this comment.
If the uncontrolled component updates, we can't keep selecting the first item in the list. We need to let Downshift tell us which item is selected.
There was a problem hiding this comment.
Oh, good catch. It should be working now (tested with the storybook component).
tellthemachines
force-pushed
the
fix/custom-select-voiceover-issue
branch
from
b7af0bc
to
6c6297e
Compare
| @@ -69,14 +76,12 @@ export default function CustomSelectControl( { | |||
| highlightedIndex, | |||
| selectedItem, | |||
| } = useSelect( { | |||
| initialSelectedItem: items[ 0 ], | |||
| initialSelectedItem: items[ valueIndex >= 0 ? valueIndex : 0 ], | |||
| items, | |||
| itemToString, | |||
| onSelectedItemChange, | |||
There was a problem hiding this comment.
| onSelectedItemChange, | |
| onSelectedItemChange, | |
| selectedItem: valueIndex >= 0 ? items[ valueIndex ] : undefined |
Downshift needs to be aware when the item really does change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe we can add a prop to "silence" the announcement in those cases?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Update: memoizing getSelectOptions works for the announcement when switching editor modes, so that's the main issue solved ✅
There was a problem hiding this comment.
(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!
| ) { | ||
| delete menuProps[ 'aria-activedescendant' ]; | ||
| } | ||
| const selectedItemValue = value ? value : selectedItem; |
There was a problem hiding this comment.
| const selectedItemValue = value ? value : selectedItem; | |
| const selectedItemValue = selectedItem; |
I didn't do that because there's a condition right before |
|
Yeah, definitely. Especially because this component can be used in multiple places. |
|
I am probably late to the party, but couldn't this issue be fixed by controlling The default messaged generated provide information such as the number of options / which option has been selected in the select, so I am trusting these are important from the a11y point of view. I did not follow the whole discussion however, but if I can help let me know! |
|
Thanks @silviuaavram ! What we need to do is disable the message whenever the update to |
Description
While testing #22453 I found that announcements of switch between Navigation and Edit modes are not being read out by VoiceOver on Paragraph and Heading blocks. They're working as expected on other block types.
The issue is with the
useSelecthook inCustomSelectControlbeing passed the current value of the control.useSelectgenerates anaria-liveannouncement every time the value changes, or every timeCustomSelectControlre-renders. Because the switch between modes causes the sidebar to re-render, that announcement is triggered forFontSizePicker, and it overrides the mode switch announcement.My change here is to bypass
useSelectand use the current value prop directly for populating the toggle button and styling the selected item. This changes nothing in the visual functionality, and has the extra benefit of stopping change announcements for the Preset size field whenever the Custom font size field or the Reset buttons are used. (Currently, changing the size in the Custom field with the up and down arrows causes "Custom has been selected" to be announced at every change, which can become quite annoying.)How has this been tested?
Tested across browsers; especially with Safari + VoiceOver on mac OS, and Firefox + NVDA on Windows.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: