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

Fix UrlInput combobox to use the ARIA 1.0 pattern. #47148

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jan 13, 2023

What?

Fixes #47147
Safari and VoiceOver don't read the UrlInput combobox suggestions. To make the combobox implementation be broadly supported, there's the need to stick to the 'old' ARIA 1.0 pattern.

/Cc @tyxla @mirka

Why?

The combobox implementation needs to be consistent across all the codebase and use the most broadly supported ARIA pattern.

How?

Testing Instructions

  • edit a post
  • use Safari and VoiceOver
  • add a link to a word in a paragraph
  • type 'hello' in the URL input search to get some link suggestions
  • use the Down and Up keys to navigate through the suggestion options
  • observe VoiceOver does announce the options

Optionally test with the most common browser / screen reader combinations. I tested with:

  • macOS: Safari + VoiceOver
  • Windows: Firefox + NVDA
  • Windows: Chrome + NVDA
  • Windows: JAWS 2019 + Chrome
  • Windows: JAWS 2019 + Firefox

Update: note to clarify that Safari and VoiceOver are buggy anyways. When testing, you may notice some weirdness in the way the options are announced e.g.:

  • sometimes they are announced as {option name}, clickable
  • sometimes they are announced as {option name}, selected, (1 of n), completion selected (this is the expected behavior)
  • sometimes you may need to delete one of the typed characters and re-type it again to make VoiceOver announce the options

Testing Instructions for Keyboard

See above.

Screenshots or screencast

Screenshot 2023-01-13 at 17 09 33

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release [Package] Block editor /packages/block-editor labels Jan 13, 2023
@afercia afercia requested a review from ellatrix as a code owner January 13, 2023 16:13
@github-actions
Copy link

github-actions bot commented Jan 13, 2023

Size Change: -15.2 kB (-1%)

Total Size: 1.31 MB

Filename Size Change
build/blob/index.min.js 483 B -4 B (-1%)
build/block-editor/content-rtl.css 4.11 kB +516 B (+14%) ⚠️
build/block-editor/content.css 4.1 kB +516 B (+14%) ⚠️
build/block-editor/index.min.js 192 kB +7.62 kB (+4%)
build/block-editor/style-rtl.css 14.3 kB +81 B (+1%)
build/block-editor/style.css 14.3 kB +81 B (+1%)
build/block-library/blocks/post-excerpt/style-rtl.css 134 B +65 B (+94%) 🆘
build/block-library/blocks/post-excerpt/style.css 134 B +65 B (+94%) 🆘
build/block-library/blocks/query/editor-rtl.css 458 B +18 B (+4%)
build/block-library/blocks/query/editor.css 457 B +17 B (+4%)
build/block-library/blocks/rss/editor-rtl.css 149 B -53 B (-26%) 🎉
build/block-library/blocks/rss/editor.css 149 B -55 B (-27%) 🎉
build/block-library/blocks/table/editor-rtl.css 433 B -24 B (-5%)
build/block-library/blocks/table/editor.css 433 B -24 B (-5%)
build/block-library/blocks/video/editor-rtl.css 552 B -139 B (-20%) 🎉
build/block-library/blocks/video/editor.css 555 B -139 B (-20%) 🎉
build/block-library/editor-rtl.css 11.5 kB -173 B (-1%)
build/block-library/editor.css 11.5 kB -173 B (-1%)
build/block-library/index.min.js 199 kB +353 B (0%)
build/block-library/style-rtl.css 12.5 kB +35 B (0%)
build/block-library/style.css 12.5 kB +35 B (0%)
build/blocks/index.min.js 50.4 kB -5 B (0%)
build/components/index.min.js 203 kB +228 B (0%)
build/components/style-rtl.css 11.6 kB +8 B (0%)
build/components/style.css 11.7 kB +8 B (0%)
build/compose/index.min.js 12.3 kB +38 B (0%)
build/core-data/index.min.js 15.9 kB -15 B (0%)
build/customize-widgets/index.min.js 11.8 kB +150 B (+1%)
build/data/index.min.js 8.06 kB +371 B (+5%) 🔍
build/edit-navigation/index.min.js 0 B -16.2 kB (removed) 🏆
build/edit-navigation/style-rtl.css 0 B -4.12 kB (removed) 🏆
build/edit-navigation/style.css 0 B -4.13 kB (removed) 🏆
build/edit-post/index.min.js 34.5 kB +81 B (0%)
build/edit-post/style-rtl.css 7.46 kB +15 B (0%)
build/edit-post/style.css 7.45 kB +15 B (0%)
build/edit-site/index.min.js 64.1 kB -2.28 kB (-3%)
build/edit-site/style-rtl.css 9.55 kB +251 B (+3%)
build/edit-site/style.css 9.55 kB +252 B (+3%)
build/edit-widgets/index.min.js 16.9 kB +132 B (+1%)
build/edit-widgets/style-rtl.css 4.49 kB +24 B (+1%)
build/edit-widgets/style.css 4.49 kB +25 B (+1%)
build/editor/index.min.js 45.5 kB +1.37 kB (+3%)
build/editor/style-rtl.css 3.68 kB -7 B (0%)
build/editor/style.css 3.67 kB -9 B (0%)
build/experiments/index.min.js 905 B +23 B (+3%)
build/format-library/index.min.js 7.23 kB +35 B (0%)
build/media-utils/index.min.js 2.99 kB +48 B (+2%)
build/priority-queue/index.min.js 1.52 kB -68 B (-4%)
build/reusable-blocks/index.min.js 2.26 kB +8 B (0%)
build/reusable-blocks/style-rtl.css 265 B -18 B (-6%)
build/reusable-blocks/style.css 265 B -18 B (-6%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 993 B
build/annotations/index.min.js 2.78 kB
build/api-fetch/index.min.js 2.27 kB
build/autop/index.min.js 2.15 kB
build/block-directory/index.min.js 7.16 kB
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
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 84 B
build/block-library/blocks/avatar/style.css 84 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 485 B
build/block-library/blocks/button/editor.css 485 B
build/block-library/blocks/button/style-rtl.css 532 B
build/block-library/blocks/button/style.css 532 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 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 100 B
build/block-library/blocks/categories/style.css 100 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 406 B
build/block-library/blocks/columns/style.css 406 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 612 B
build/block-library/blocks/cover/editor.css 613 B
build/block-library/blocks/cover/style-rtl.css 1.57 kB
build/block-library/blocks/cover/style.css 1.56 kB
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 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 353 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 984 B
build/block-library/blocks/gallery/editor.css 988 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 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 829 B
build/block-library/blocks/image/editor.css 828 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/latest-comments/style-rtl.css 298 B
build/block-library/blocks/latest-comments/style.css 298 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 507 B
build/block-library/blocks/media-text/style.css 505 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 716 B
build/block-library/blocks/navigation-link/editor.css 715 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 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.13 kB
build/block-library/blocks/navigation/editor.css 2.14 kB
build/block-library/blocks/navigation/style-rtl.css 2.22 kB
build/block-library/blocks/navigation/style.css 2.2 kB
build/block-library/blocks/navigation/view-modal.min.js 2.81 kB
build/block-library/blocks/navigation/view.min.js 447 B
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 376 B
build/block-library/blocks/page-list/editor.css 376 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 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 279 B
build/block-library/blocks/paragraph/style.css 281 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 501 B
build/block-library/blocks/post-comments-form/style.css 501 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 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-featured-image/editor-rtl.css 586 B
build/block-library/blocks/post-featured-image/editor.css 584 B
build/block-library/blocks/post-featured-image/style-rtl.css 318 B
build/block-library/blocks/post-featured-image/style.css 318 B
build/block-library/blocks/post-navigation-link/style-rtl.css 153 B
build/block-library/blocks/post-navigation-link/style.css 153 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 282 B
build/block-library/blocks/post-template/style.css 282 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-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 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 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 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/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 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/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 409 B
build/block-library/blocks/search/style.css 406 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 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 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 490 B
build/block-library/blocks/site-logo/editor.css 490 B
build/block-library/blocks/site-logo/style-rtl.css 203 B
build/block-library/blocks/site-logo/style.css 203 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 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/social-links/style-rtl.css 1.4 kB
build/block-library/blocks/social-links/style.css 1.39 kB
build/block-library/blocks/spacer/editor-rtl.css 332 B
build/block-library/blocks/spacer/editor.css 332 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/style-rtl.css 651 B
build/block-library/blocks/table/style.css 650 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 404 B
build/block-library/blocks/template-part/editor.css 404 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/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/style-rtl.css 179 B
build/block-library/blocks/video/style.css 179 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 162 B
build/block-library/classic.css 162 B
build/block-library/common-rtl.css 1.05 kB
build/block-library/common.css 1.05 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/theme-rtl.css 698 B
build/block-library/theme.css 703 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/customize-widgets/style-rtl.css 1.41 kB
build/customize-widgets/style.css 1.41 kB
build/data-controls/index.min.js 663 B
build/date/index.min.js 32.1 kB
build/deprecated/index.min.js 518 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.71 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/element/index.min.js 4.93 kB
build/escape-html/index.min.js 548 B
build/format-library/style-rtl.css 598 B
build/format-library/style.css 597 B
build/hooks/index.min.js 1.66 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.79 kB
build/is-shallow-equal/index.min.js 535 B
build/keyboard-shortcuts/index.min.js 1.79 kB
build/keycodes/index.min.js 1.88 kB
build/list-reusable-blocks/index.min.js 2.14 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/notices/index.min.js 977 B
build/plugins/index.min.js 1.95 kB
build/preferences-persistence/index.min.js 2.23 kB
build/preferences/index.min.js 1.35 kB
build/primitives/index.min.js 960 B
build/react-i18n/index.min.js 702 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.75 kB
build/rich-text/index.min.js 10.8 kB
build/server-side-render/index.min.js 2.09 kB
build/shortcode/index.min.js 1.52 kB
build/style-engine/index.min.js 1.53 kB
build/token-list/index.min.js 650 B
build/url/index.min.js 3.69 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 1.09 kB
build/warning/index.min.js 280 B
build/widgets/index.min.js 7.31 kB
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

@github-actions
Copy link

github-actions bot commented Jan 13, 2023

Flaky tests detected in e32866a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3931843625
📝 Reported issues:

Copy link
Contributor

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

Stupid Voiceover. This is why no one can ever move forward, Apple lags years behind in basic accessibility. It generally is not a problem in native applications but comes back to hurt us big time on the web. So annoying. This has also become much worse in IOS 16 for mobile devices. Wish Apple would bring back their old values.

My political comments out of the way, this looks good from a code perspective. I trust your judgement in testing @afercia, thanks for catching this.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks @afercia 🙌

I've just left a couple of suggestions on the tests, but nothing critical.

// Search Input UI.
const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

expect( searchInput ).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to change this assertion to use toBeVisible()? I've found this to be a bit more specific query, since toBeInTheDocument() will be truthy if the element is not accessible to the user at all.

Suggested change
expect( searchInput ).toBeInTheDocument();
expect( searchInput ).toBeVisible();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. There are a few more occurrences of toBeInTheDocument(). Some of them use not and they actually test for some elements to not be in the DOM. I guess the other ones should be changed to toBeVisible().

const searchInput = screen.getByRole( 'combobox', { name: 'URL' } );

expect( searchInput ).toBeInTheDocument();
expect( searchInput ).not.toHaveAttribute( 'aria-controls' );
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a good reason to have this assertion in place. I think we can remove it.

Suggested change
expect( searchInput ).not.toHaveAttribute( 'aria-controls' );

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'd like to make sure aria-selected is set only on the highlighted item and omitted from all the other items. The current usage is wrong and I'd like to to avoid future misuse. I'd agree it's best to move this to a separate test though.

Copy link
Member

Choose a reason for hiding this comment

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

I understand. thanks for clarifying. I guess it's worth adding some context as a comment to the test then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I see now you mentioned the assertion for aria-controls, not the one for aria-selected. Will add comments to clarify both.

@afercia
Copy link
Contributor Author

afercia commented Feb 1, 2023

Thanks everyone for the feedback. Everything is green, merging.

@afercia afercia merged commit 76f8b5f into trunk Feb 1, 2023
@afercia afercia deleted the fix/combobox-stick-to-aria-1-0-pattern branch February 1, 2023 15:13
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 1, 2023
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). [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UrlInput: Stick to the ARIA 1.0 pattern
3 participants