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

Conversation

alexstine
Copy link
Contributor

What?

Fixes #47767

Why?

Stupid Voiceover still doesn't support aria-activedescendant. Bugs going back for years, wish Apple would fix this.

Inspired by: https://react-spectrum.adobe.com/blog/building-a-combobox.html

How?

  • Adds a speak for IOS and Mac only.
  • Allows autocomplete to take a text-only label.
  • Adds a package to determine OS.

Testing Instructions

  1. On Mac, enable Voiceover.
  2. Open a post or page.
  3. Type a title.
  4. Press Enter to create your first block.
  5. Press slash "/" and type "tab" or another search phrase.
  6. Use Up and Down Arrow keys and notice how Voiceover announces each block in an aria-live region.
  7. Try the same on Windows and notice how there is no screen reader announcement via aria-live.

Testing Instructions for Keyboard

Screenshots or screencast

@alexstine alexstine added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Element /packages/element [Package] Components /packages/components Needs Accessibility Feedback Need input from accessibility [Package] Block editor /packages/block-editor labels Sep 28, 2023
@alexstine alexstine self-assigned this Sep 28, 2023
Copy link
Contributor Author

@alexstine alexstine left a comment

Choose a reason for hiding this comment

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

I am not pleased with this solution but due to the lack of support from Apple, it is what it is. One note below.

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

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Size Change: +125 B (0%)

Total Size: 1.66 MB

Filename Size Change
build/block-editor/index.min.js 219 kB +4 B (0%)
build/components/index.min.js 251 kB +121 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 964 B
build/annotations/index.min.js 2.71 kB
build/api-fetch/index.min.js 2.29 kB
build/autop/index.min.js 2.11 kB
build/blob/index.min.js 461 B
build/block-directory/index.min.js 7.25 kB
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 4.28 kB
build/block-editor/content.css 4.27 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
build/block-editor/style-rtl.css 15.6 kB
build/block-editor/style.css 15.6 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 138 B
build/block-library/blocks/audio/theme.css 138 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 587 B
build/block-library/blocks/button/editor.css 587 B
build/block-library/blocks/button/style-rtl.css 633 B
build/block-library/blocks/button/style.css 632 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 421 B
build/block-library/blocks/columns/style.css 421 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.7 kB
build/block-library/blocks/cover/style.css 1.69 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 138 B
build/block-library/blocks/embed/theme.css 138 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 311 B
build/block-library/blocks/file/style.css 312 B
build/block-library/blocks/file/view.min.js 321 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/form-input/editor-rtl.css 229 B
build/block-library/blocks/form-input/editor.css 228 B
build/block-library/blocks/form-input/style-rtl.css 343 B
build/block-library/blocks/form-input/style.css 343 B
build/block-library/blocks/form-submission-notification/editor-rtl.css 343 B
build/block-library/blocks/form-submission-notification/editor.css 342 B
build/block-library/blocks/form-submit-button/style-rtl.css 69 B
build/block-library/blocks/form-submit-button/style.css 69 B
build/block-library/blocks/form/view.min.js 452 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 957 B
build/block-library/blocks/gallery/editor.css 962 B
build/block-library/blocks/gallery/style-rtl.css 1.55 kB
build/block-library/blocks/gallery/style.css 1.55 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 189 B
build/block-library/blocks/heading/style.css 189 B
build/block-library/blocks/html/editor-rtl.css 340 B
build/block-library/blocks/html/editor.css 341 B
build/block-library/blocks/image/editor-rtl.css 834 B
build/block-library/blocks/image/editor.css 833 B
build/block-library/blocks/image/style-rtl.css 1.54 kB
build/block-library/blocks/image/style.css 1.54 kB
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/image/view.min.js 2 kB
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 505 B
build/block-library/blocks/media-text/style.css 503 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 671 B
build/block-library/blocks/navigation-link/editor.css 672 B
build/block-library/blocks/navigation-link/style-rtl.css 103 B
build/block-library/blocks/navigation-link/style.css 103 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation/editor-rtl.css 2.26 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/style-rtl.css 2.26 kB
build/block-library/blocks/navigation/style.css 2.25 kB
build/block-library/blocks/navigation/view.min.js 1.07 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 401 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-featured-image/style-rtl.css 345 B
build/block-library/blocks/post-featured-image/style.css 345 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 409 B
build/block-library/blocks/post-template/style.css 408 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 335 B
build/block-library/blocks/pullquote/style.css 335 B
build/block-library/blocks/pullquote/theme-rtl.css 168 B
build/block-library/blocks/pullquote/theme.css 168 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/style-rtl.css 312 B
build/block-library/blocks/query/style.css 308 B
build/block-library/blocks/query/view.min.js 609 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 184 B
build/block-library/blocks/search/editor.css 184 B
build/block-library/blocks/search/style-rtl.css 613 B
build/block-library/blocks/search/style.css 613 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 471 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 329 B
build/block-library/blocks/shortcode/editor.css 329 B
build/block-library/blocks/site-logo/editor-rtl.css 760 B
build/block-library/blocks/site-logo/editor.css 760 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/social-links/style-rtl.css 1.45 kB
build/block-library/blocks/social-links/style.css 1.45 kB
build/block-library/blocks/spacer/editor-rtl.css 359 B
build/block-library/blocks/spacer/editor.css 359 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 432 B
build/block-library/blocks/table/editor.css 432 B
build/block-library/blocks/table/style-rtl.css 646 B
build/block-library/blocks/table/style.css 645 B
build/block-library/blocks/table/theme-rtl.css 157 B
build/block-library/blocks/table/theme.css 157 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 191 B
build/block-library/blocks/video/style.css 191 B
build/block-library/blocks/video/theme-rtl.css 139 B
build/block-library/blocks/video/theme.css 139 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.11 kB
build/block-library/common.css 1.11 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.4 kB
build/block-library/editor.css 12.4 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 211 kB
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/style-rtl.css 14.4 kB
build/block-library/style.css 14.4 kB
build/block-library/theme-rtl.css 700 B
build/block-library/theme.css 705 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/blocks/index.min.js 51.5 kB
build/commands/index.min.js 15.5 kB
build/commands/style-rtl.css 947 B
build/commands/style.css 942 B
build/components/style-rtl.css 11.9 kB
build/components/style.css 11.9 kB
build/compose/index.min.js 12.7 kB
build/core-commands/index.min.js 2.72 kB
build/core-data/index.min.js 71.7 kB
build/customize-widgets/index.min.js 12 kB
build/customize-widgets/style-rtl.css 1.51 kB
build/customize-widgets/style.css 1.5 kB
build/data-controls/index.min.js 651 B
build/data/index.min.js 8.87 kB
build/date/index.min.js 17.9 kB
build/deprecated/index.min.js 462 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.68 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-post/index.min.js 35.7 kB
build/edit-post/style-rtl.css 7.88 kB
build/edit-post/style.css 7.88 kB
build/edit-site/index.min.js 206 kB
build/edit-site/style-rtl.css 14.3 kB
build/edit-site/style.css 14.3 kB
build/edit-widgets/index.min.js 17 kB
build/edit-widgets/style-rtl.css 4.85 kB
build/edit-widgets/style.css 4.84 kB
build/editor/index.min.js 45.9 kB
build/editor/style-rtl.css 3.58 kB
build/editor/style.css 3.58 kB
build/element/index.min.js 4.87 kB
build/escape-html/index.min.js 548 B
build/format-library/index.min.js 7.82 kB
build/format-library/style-rtl.css 577 B
build/format-library/style.css 577 B
build/hooks/index.min.js 1.57 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.61 kB
build/interactivity/index.min.js 11.4 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.76 kB
build/keycodes/index.min.js 1.9 kB
build/list-reusable-blocks/index.min.js 2.21 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 964 B
build/nux/index.min.js 2.01 kB
build/nux/style-rtl.css 775 B
build/nux/style.css 771 B
build/patterns/index.min.js 4.51 kB
build/patterns/style-rtl.css 518 B
build/patterns/style.css 517 B
build/plugins/index.min.js 1.81 kB
build/preferences-persistence/index.min.js 1.85 kB
build/preferences/index.min.js 1.26 kB
build/primitives/index.min.js 994 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 988 B
build/react-i18n/index.min.js 631 B
build/react-refresh-entry/index.min.js 9.46 kB
build/react-refresh-runtime/index.min.js 6.78 kB
build/redux-routine/index.min.js 2.71 kB
build/reusable-blocks/index.min.js 2.73 kB
build/reusable-blocks/style-rtl.css 265 B
build/reusable-blocks/style.css 265 B
build/rich-text/index.min.js 10.2 kB
build/router/index.min.js 1.79 kB
build/server-side-render/index.min.js 1.96 kB
build/shortcode/index.min.js 1.4 kB
build/style-engine/index.min.js 1.98 kB
build/token-list/index.min.js 587 B
build/url/index.min.js 3.83 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/viewport/index.min.js 967 B
build/warning/index.min.js 259 B
build/widgets/index.min.js 7.18 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.03 kB

compressed-size-action

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Early notes:

  • The keycodes package has a similar helper for detecting Apple devices - isAppleOS. We can probably copy that.
  • The new helper can probably placed in components packages instead of making it public API via element.
  • By the way, the element package already has a platform.js file.

@alexstine
Copy link
Contributor Author

@Mamaduka Can the keycodes package also detect if IOS? I need both for this PR. I can add it to the components package but thought it might be worth exposing it in case there is a need to have a sane OS check that works.

More work to come on this...

@Mamaduka
Copy link
Member

Can the keycodes package also detect if IOS?

I believe they do.

it might be worth exposing it in case there is a need to have a sane OS check that works.

We can do that separately. Let's keep the scope of this PR small. It makes it easier to review and land.

@alexstine
Copy link
Contributor Author

Okay, resolved all feedback so far. This is still working well.

@alexstine alexstine removed the [Package] Element /packages/element label Sep 28, 2023
@Kishlol

This comment was marked as off-topic.

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

@alexstine alexstine removed the [Package] Block editor /packages/block-editor label Oct 19, 2023
Copy link
Contributor

@jeryj jeryj left a comment

Choose a reason for hiding this comment

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

Thank you for getting this important fix in for this release, @alexstine!

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

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

Copy link
Contributor

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

Had one suggestion for the getNodeText. Otherwise, this looks good to me.

Comment on lines +44 to +71
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 '';
};
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 '';
}
};

@jeryj
Copy link
Contributor

jeryj commented Oct 19, 2023

I've triggered this to rerun the failed PHP jobs. I don't know why this PR would cause those to fail.

@alexstine alexstine merged commit d74d9e1 into trunk Oct 19, 2023
50 checks passed
@alexstine alexstine deleted the fix/autocomplete-voiceover branch October 19, 2023 21:15
@github-actions github-actions bot added this to the Gutenberg 17.0 milestone Oct 19, 2023
@alexstine
Copy link
Contributor Author

I have already assigned the follow-up to myself and will get to work on it so it can go out in 6.4.1.

@SiobhyB SiobhyB added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 23, 2023
@SiobhyB
Copy link
Contributor

SiobhyB commented Oct 23, 2023

Following a discussion on Slack, I'm acknowledging the decision to include this fix in 6.4.1, rather than the main 6.4 release. Slack discussion for reference: https://wordpress.slack.com/archives/C02QB2JS7/p1697795881565639

mikachan pushed a commit that referenced this pull request Nov 9, 2023
* Add a function to return a text-only label.

* Use already defined isAppleOS function from keycodes. Remove new OS functions from element.

* Add fallback if textLabel is unavailable.

* Try combobox role to get around annoying re-rendering type effect.

* Changelog entry.

* Revert Windows fix, too much scope creep.

* Fix Changelog.

* Remove diff artifact.

* Remove mistaken files.

* Add comment linking to PR.

* Revert textLabel prop.
# Conflicts:
#	packages/components/CHANGELOG.md
@mikachan mikachan removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 9, 2023
@mikachan
Copy link
Member

mikachan commented Nov 9, 2023

I just cherry-picked this PR to the 6.4-next-point-release branch to get it included in the next release: f2dea97

mikachan pushed a commit that referenced this pull request Nov 13, 2023
* Add a function to return a text-only label.

* Use already defined isAppleOS function from keycodes. Remove new OS functions from element.

* Add fallback if textLabel is unavailable.

* Try combobox role to get around annoying re-rendering type effect.

* Changelog entry.

* Revert Windows fix, too much scope creep.

* Fix Changelog.

* Remove diff artifact.

* Remove mistaken files.

* Add comment linking to PR.

* Revert textLabel prop.
# Conflicts:
#	packages/components/CHANGELOG.md
@Kishlol

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Package] Components /packages/components [Type] Regression Related to a regression in the latest release
Projects
Status: Done 🎉
Status: Punted to 6.4.1
Development

Successfully merging this pull request may close these issues.

Autocomplete: Must move inside iFrame for slash block insert
10 participants