-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Autocomplete: Fix Voiceover not announcing suggestions #54902
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
Merged
Merged
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 3c82605
Use already defined isAppleOS function from keycodes. Remove new OS f…
alexstine 700bb1c
Add fallback if textLabel is unavailable.
alexstine a6b45ab
Try combobox role to get around annoying re-rendering type effect.
alexstine 98bbc1f
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine b43d95e
Changelog entry.
alexstine 6372f1c
Code quality and refresh.
alexstine 96464c6
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine 7ecf361
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine 7972225
Revert Windows fix, too much scope creep.
alexstine 7681b88
Fix Changelog.
alexstine 1e24a70
Remove diff artifact.
alexstine facbe98
Fix conflict.
alexstine 8e7ba56
Merge branch 'fix/autocomplete-voiceover' of github.com:WordPress/gut…
alexstine 307b78e
Merge branch 'trunk' into fix/autocomplete-voiceover
alexstine 98a845b
Remove mistaken files.
alexstine 6e91679
Add comment linking to PR.
alexstine fe36a90
Revert textLabel prop.
alexstine 607c455
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/autoc…
alexstine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Try combobox role to get around annoying re-rendering type effect.
- Loading branch information
commit a6b45abdec956d86220d70e1192410d5e2418056
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to
comboboxdoesn'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...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.
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
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
Comboboxcomponent which wraps around part ofFormTokenInput.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"androle="listbox"on the correct elements if we could preventlistBoxfrom re-rendering every time. Every time I press Up or Down Arrow keys, theAutocompleterUItriggers a re-render of theListBoxand React Dev Tools says the cause is:Props changed: (selectedIndex, onChangeOptions, onSelect, reset)Is it possible to
useCallbackoruseMemotheonChangeOptionscallback to only change if needed? Up or Down Arrow keys should only adjust theselectedIndex, not re-render the entire options tree.There was a problem hiding this comment.
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
Comboboxcomponent. In theory, the props would simply update the existing component but it seems like the whole component is being replaced which also forcesListBoxto re-render.