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

TabPanel: implement Ariakit internally #52133

Merged
merged 30 commits into from
Aug 2, 2023
Merged

TabPanel: implement Ariakit internally #52133

merged 30 commits into from
Aug 2, 2023

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Jun 29, 2023

⚠️ Do not backport to WP 6.3 ⚠️


Related/alternate to: #51551

This PR is a collaboration between myself and @ciampo, and also builds off of some previous work by @flootr.

What?

Introduce a Ariakit-powered version of TabPanel, named Tabs, maintaining the same functionality and API surface as the existing component. Eventually this will be used to ensure compatibility for existing TabPanel implementations as we move fully to a completely new component.

Initially, Radix-powered TabPanel alternative was explored, but as we continue to evaluate which library(ies) will be the best fit, we're also looking at things like this Ariakit implementation.

Why?

As we experiment with third-party libraries for certain UI patterns, TabPanel is a good candidate to explore, as there are time where additional functionality could be achieved with a more composable version of the component, which will come in future PRs.

How?

Tabs is a wrapper around a composed TabPanel utilizing the various Ariakit subcomponents. Tabs is written to accept all the same props as TabPanel, passing them to the right props for the various Ariakit subcomponents internally.

Notes

We've had to maintain the existing tab selection logic, both for initial tab selection and the handling of disabled tabs. While Ariakits's built in defaultSelectedId works well, there were a few elements of our existing component that we needed to to preserve our own custom logic to maintain.

Note: The new component does not yet have a README, as this isn't form that Tabs will take long-term. I'll make sure one is added in a future PR when the new component is nearer to completion.

Testing Instructions

  1. Open Storybook and experiment with the new Tabs component.
  2. Confirm that the controls work as expected.
  3. Confirm that each story behaves the same as its TabPanel counterpart.
  4. Specifically test keyboard interactions. Disabled tabs should be selectable via arrow keys, but not pointer events.

@chad1008 chad1008 requested a review from ajitbohra as a code owner June 29, 2023 16:51
@chad1008 chad1008 changed the title Try: Add @radix-ui/react-tabs-powered version of TabPanel Try: Add Ariakit-powered version of TabPanel Jun 29, 2023
@github-actions
Copy link

github-actions bot commented Jun 29, 2023

Size Change: +2.01 kB (0%)

Total Size: 1.44 MB

Filename Size Change
build/components/index.min.js 243 kB +2.01 kB (+1%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 955 B
build/annotations/index.min.js 2.69 kB
build/api-fetch/index.min.js 2.28 kB
build/autop/index.min.js 2.1 kB
build/blob/index.min.js 451 B
build/block-directory/index.min.js 6.99 kB
build/block-directory/style-rtl.css 1.02 kB
build/block-directory/style.css 1.02 kB
build/block-editor/content-rtl.css 4.26 kB
build/block-editor/content.css 4.25 kB
build/block-editor/default-editor-styles-rtl.css 381 B
build/block-editor/default-editor-styles.css 381 B
build/block-editor/index.min.js 210 kB
build/block-editor/style-rtl.css 14.8 kB
build/block-editor/style.css 14.8 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 126 B
build/block-library/blocks/audio/theme.css 126 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 584 B
build/block-library/blocks/button/editor.css 582 B
build/block-library/blocks/button/style-rtl.css 624 B
build/block-library/blocks/button/style.css 623 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 409 B
build/block-library/blocks/columns/style.css 409 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.61 kB
build/block-library/blocks/cover/style.css 1.6 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 178 B
build/block-library/blocks/details/style.css 178 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 126 B
build/block-library/blocks/embed/theme.css 126 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 269 B
build/block-library/blocks/file/style.css 270 B
build/block-library/blocks/file/view-interactivity.min.js 317 B
build/block-library/blocks/file/view.min.js 375 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/freeform/editor-rtl.css 2.58 kB
build/block-library/blocks/freeform/editor.css 2.58 kB
build/block-library/blocks/gallery/editor-rtl.css 947 B
build/block-library/blocks/gallery/editor.css 952 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 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 336 B
build/block-library/blocks/html/editor.css 337 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.42 kB
build/block-library/blocks/image/style.css 1.42 kB
build/block-library/blocks/image/theme-rtl.css 126 B
build/block-library/blocks/image/theme.css 126 B
build/block-library/blocks/image/view-interactivity.min.js 1.46 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 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 712 B
build/block-library/blocks/navigation-link/editor.css 711 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 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 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.23 kB
build/block-library/blocks/navigation/style.css 2.22 kB
build/block-library/blocks/navigation/view-interactivity.min.js 988 B
build/block-library/blocks/navigation/view-modal.min.js 2.85 kB
build/block-library/blocks/navigation/view.min.js 469 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 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 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 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 319 B
build/block-library/blocks/post-featured-image/style.css 319 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 314 B
build/block-library/blocks/post-template/style.css 314 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 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 302 B
build/block-library/blocks/query-pagination/style.css 299 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 450 B
build/block-library/blocks/query/editor.css 449 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 178 B
build/block-library/blocks/search/editor.css 178 B
build/block-library/blocks/search/style-rtl.css 587 B
build/block-library/blocks/search/style.css 584 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 631 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 323 B
build/block-library/blocks/shortcode/editor.css 323 B
build/block-library/blocks/site-logo/editor-rtl.css 754 B
build/block-library/blocks/site-logo/editor.css 754 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.44 kB
build/block-library/blocks/social-links/style.css 1.43 kB
build/block-library/blocks/spacer/editor-rtl.css 348 B
build/block-library/blocks/spacer/editor.css 348 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 433 B
build/block-library/blocks/table/editor.css 433 B
build/block-library/blocks/table/style-rtl.css 645 B
build/block-library/blocks/table/style.css 644 B
build/block-library/blocks/table/theme-rtl.css 146 B
build/block-library/blocks/table/theme.css 146 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 185 B
build/block-library/blocks/video/style.css 185 B
build/block-library/blocks/video/theme-rtl.css 126 B
build/block-library/blocks/video/theme.css 126 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.1 kB
build/block-library/common.css 1.1 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.1 kB
build/block-library/editor.css 12.1 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/index.min.js 202 kB
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-library/style-rtl.css 13.7 kB
build/block-library/style.css 13.8 kB
build/block-library/theme-rtl.css 686 B
build/block-library/theme.css 691 B
build/block-serialization-default-parser/index.min.js 1.12 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/blocks/index.min.js 51 kB
build/commands/index.min.js 15.1 kB
build/commands/style-rtl.css 835 B
build/commands/style.css 834 B
build/components/style-rtl.css 11.8 kB
build/components/style.css 11.8 kB
build/compose/index.min.js 12.1 kB
build/core-commands/index.min.js 2.44 kB
build/core-data/index.min.js 16.4 kB
build/customize-widgets/index.min.js 12 kB
build/customize-widgets/style-rtl.css 1.46 kB
build/customize-widgets/style.css 1.45 kB
build/data-controls/index.min.js 640 B
build/data/index.min.js 8.28 kB
build/date/index.min.js 17.8 kB
build/deprecated/index.min.js 451 B
build/dom-ready/index.min.js 324 B
build/dom/index.min.js 4.63 kB
build/edit-post/classic-rtl.css 544 B
build/edit-post/classic.css 545 B
build/edit-post/index.min.js 35.6 kB
build/edit-post/style-rtl.css 7.58 kB
build/edit-post/style.css 7.58 kB
build/edit-site/index.min.js 90.3 kB
build/edit-site/style-rtl.css 13.2 kB
build/edit-site/style.css 13.2 kB
build/edit-widgets/index.min.js 16.9 kB
build/edit-widgets/style-rtl.css 4.52 kB
build/edit-widgets/style.css 4.52 kB
build/editor/index.min.js 45.3 kB
build/editor/style-rtl.css 3.54 kB
build/editor/style.css 3.53 kB
build/element/index.min.js 4.8 kB
build/escape-html/index.min.js 537 B
build/format-library/index.min.js 7.59 kB
build/format-library/style-rtl.css 554 B
build/format-library/style.css 553 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.58 kB
build/interactivity/index.min.js 10.4 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.64 kB
build/keycodes/index.min.js 1.84 kB
build/list-reusable-blocks/index.min.js 2.18 kB
build/list-reusable-blocks/style-rtl.css 836 B
build/list-reusable-blocks/style.css 836 B
build/media-utils/index.min.js 2.9 kB
build/notices/index.min.js 948 B
build/nux/index.min.js 1.99 kB
build/nux/style-rtl.css 735 B
build/nux/style.css 732 B
build/plugins/index.min.js 1.77 kB
build/preferences-persistence/index.min.js 1.84 kB
build/preferences/index.min.js 1.24 kB
build/primitives/index.min.js 943 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 951 B
build/react-i18n/index.min.js 615 B
build/react-refresh-entry/index.min.js 9.47 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/redux-routine/index.min.js 2.7 kB
build/reusable-blocks/index.min.js 2.71 kB
build/reusable-blocks/style-rtl.css 243 B
build/reusable-blocks/style.css 243 B
build/rich-text/index.min.js 11 kB
build/router/index.min.js 1.77 kB
build/server-side-render/index.min.js 1.94 kB
build/shortcode/index.min.js 1.39 kB
build/style-engine/index.min.js 1.83 kB
build/token-list/index.min.js 582 B
build/url/index.min.js 3.57 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 958 B
build/warning/index.min.js 268 B
build/widgets/index.min.js 7.16 kB
build/widgets/style-rtl.css 1.15 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.02 kB

compressed-size-action

@chad1008
Copy link
Contributor Author

👋 @diegohaz while I was working on this with @ciampo we actually found we had a question for you! A number of our unit tests began failing when we implemented Ariakit, and it turned out that the elements or text the test checked for didn't render immediately. The tests began passing when we updated them to run async with findby instead of getby.

We suspect this is due to re-renders caused by the different useEffect/useLayoutEffect calls we needed to preserve, but were curious if had any thoughts on a better approach. For this PR, (similar to the previous Radix attempt) it's important that we preserve all the functionality and behavior of the existing component. That'll be less critical in future PRs as we build out a more composable version.

@chad1008 chad1008 requested a review from mirka June 29, 2023 17:18
@chad1008 chad1008 self-assigned this Jun 29, 2023
@chad1008 chad1008 requested a review from diegohaz June 29, 2023 17:19
@chad1008 chad1008 added [Package] Components /packages/components [Feature] Component System WordPress component system labels Jun 29, 2023
@github-actions
Copy link

github-actions bot commented Jun 29, 2023

Flaky tests detected in d938bd3.
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/5672378046
📝 Reported issues:

@diegohaz
Copy link
Member

👋 @diegohaz while I was working on this with @ciampo we actually found we had a question for you! A number of our unit tests began failing when we implemented Ariakit, and it turned out that the elements or text the test checked for didn't render immediately. The tests began passing when we updated them to run async with findby instead of getby.

We suspect this is due to re-renders caused by the different useEffect/useLayoutEffect calls we needed to preserve, but were curious if had any thoughts on a better approach. For this PR, (similar to the previous Radix attempt) it's important that we preserve all the functionality and behavior of the existing component. That'll be less critical in future PRs as we build out a more composable version.

If you don't provide defaultSelectedId, Ariakit will automatically select the first tab. For performance reasons, this happens in a requestAnimationFrame callback, which is not automatically flushed by JSDOM/React Testing Library.

In theory, you could do something like this right after render() or rerender():

await act(async () => {
  await new Promise(requestAnimationFrame);
});

This would flush the pending animation frame callbacks triggered after render and let you use getBy functions.

But this is an implementation detail. There may be cases where requestAnimationFrame, queueMicrotask, or even requestIdleCallback are used (when available) at different times. These are all flushed automatically by browsers but not by JSDOM/RTL.

In Ariakit, we use an internal testing package that automatically flushes those timers in JSDOM in a similar way that browsers would do, so our JSDOM tests don't include implementation details.

I'd say findBy is okay as a temporary solution. But I'd highly recommend using a real browser to test those components, especially the ones that rely on third-party libraries. Playwright works really well for this. As I said, you may find other cases where you'll have to manually flush timers between actions. Also, Ariakit (and other libraries) may introduce non-breaking changes that add those timers as an implementation detail, which could make those JSDOM tests fail.

@chad1008
Copy link
Contributor Author

I'd say findBy is okay as a temporary solution. But I'd highly recommend using a real browser to test those components, especially the ones that rely on third-party libraries. Playwright works really well for this.

Excellent, thanks so much for all of that insight @diegohaz!

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

This is looking very nice!

The one difference I noticed with keyboard navigation is that the tab content (tabpanel) now gets a tab stop, which I believe is the standard and thus desirable behavior.

A bit more open to debate is what happens when there's a focusable element (like a button) in the tab content. The ARIA guidance is somewhat unclear, and there's variance in the main library implementations. Ariakit and Radix still retain the tab stop on tabpanel, while React Aria will go straight to the focusable element. I think this is fine to ship as an enhancement and not a breaking change. (cc @alexstine @andrewhayward)

package.json Outdated Show resolved Hide resolved
packages/components/package.json Outdated Show resolved Hide resolved
packages/components/src/tabs/index.tsx Outdated Show resolved Hide resolved
packages/components/src/tabs/index.tsx Outdated Show resolved Hide resolved
packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
@diegohaz
Copy link
Member

diegohaz commented Jul 4, 2023

A bit more open to debate is what happens when there's a focusable element (like a button) in the tab content. The ARIA guidance is somewhat unclear, and there's variance in the main library implementations. Ariakit and Radix still retain the tab stop on tabpanel, while React Aria will go straight to the focusable element.

Good catch! Ariakit should only focus on the tab panel element if there are no tabbable elements inside it. But it's currently not able to identify tabbable elements inside the tab panel after the first render, so it only works for the tab panel that's initially visible (using the defaultSelectedId prop). This should be addressed in future versions.

If that's a requirement now, passing tabIndex={-1} or focusable={false} to TabPanel (if there are tabbable elements inside it) should do it.

@chad1008
Copy link
Contributor Author

chad1008 commented Aug 2, 2023

Thanks for all of the time and advice on this one @mirka. Final round of changes addressed, just waiting on CI 🚀

@chad1008 chad1008 enabled auto-merge (squash) August 2, 2023 15:02
@chad1008 chad1008 merged commit 072a09d into trunk Aug 2, 2023
@chad1008 chad1008 deleted the add/ariakit-tabs branch August 2, 2023 15:37
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Aug 2, 2023
@jsnajdr
Copy link
Member

jsnajdr commented Aug 16, 2023

Seems that this PR caused that some unit tests started failing intermittently, because a button element is sometimes rendered with tabindex attribute, sometimes not.

 FAIL  packages/edit-post/src/components/preferences-modal/test/index.js
  ● EditPostPreferencesModal › should match snapshot when the modal is active › large viewports

    expect(received).toMatchSnapshot()

    Snapshot name: `EditPostPreferencesModal should match snapshot when the modal is active large viewports 1`

    - Snapshot  - 0
    + Received  + 2

    @@ -125,10 +125,11 @@
                  aria-selected="false"
                  class="components-button components-tab-panel__tabs-item"
                  data-command=""
                  id="tab-panel-0-blocks"
                  role="tab"
    +             tabindex="-1"
                  type="button"
                >
                  Blocks
                </button>
                <button
    @@ -136,10 +137,11 @@
                  aria-selected="false"
                  class="components-button components-tab-panel__tabs-item"
                  data-command=""
                  id="tab-panel-0-panels"
                  role="tab"
    +             tabindex="-1"
                  type="button"
                >
                  Panels
                </button>
              </div>

      27 | 			expect(
      28 | 				await screen.findByRole( 'dialog', { name: 'Preferences' } )
    > 29 | 			).toMatchSnapshot();
         | 			  ^
      30 | 		} );
      31 | 		it( 'small viewports', async () => {
      32 | 			useSelect.mockImplementation( () => [ true, true, false ] );

      at Object.toMatchSnapshot (packages/edit-post/src/components/preferences-modal/test/index.js:29:6)

It's a test that uses React Testing Library and JSDOM. @diegohaz is it possible that the tabindex prop is not present on initial render, but added only after on a second render triggered by some useEffect? Then we'd have to use .findBy or waitFor to wait until things settle down.

@diegohaz
Copy link
Member

It's a test that uses React Testing Library and JSDOM. @diegohaz is it possible that the tabindex prop is not present on initial render, but added only after on a second render triggered by some useEffect? Then we'd have to use .findBy or waitFor to wait until things settle down.

Yes, tabindex={-1} is added only after hydration to allow keyboard users to navigate through tabs using the Tab key before JavaScript can manage arrow keys. Unfortunately, it's a tradeoff when testing these components with an emulated DOM. I'm developing testing utilities that automatically handle all these timers, but they're not quite ready for public use yet. However, we've been using them to test components within Ariakit for years. You can see an example here: https://github.com/ariakit/ariakit/blob/0eacd26eb42fb5c54208dc31b71ef675306f940f/examples/tab/test.ts.

@jsnajdr
Copy link
Member

jsnajdr commented Aug 18, 2023

Yes, tabindex={-1} is added only after hydration to allow keyboard users to navigate through tabs using the Tab key before JavaScript can manage arrow keys.

Is it the disableFocus function that does this? By setting the tabindex attribute directly on the DOM element, altering markup that should be owned by React?

And how is it scheduled relative to the initial render? Is there useEffect, or requestAnimationFrame, or setTimeout?

I'm developing testing utilities that automatically handle all these timers, but they're not quite ready for public use yet.

I see, there are utilities that do await act(), and creating a promise that resolves after a microtask tick, or after rAF, or after timeout... But that's an antipattern we're trying to avoid in Gutenberg, because it requires too much implementation knowledge from the test author.

The preferred way is to use findBy or waitFor to detect state after all scheduled tasks are flushed. That's how we'll need to fix the failing EditPostPreferencesModal.

@diegohaz
Copy link
Member

Is it the disableFocus function that does this? By setting the tabindex attribute directly on the DOM element, altering markup that should be owned by React?

And how is it scheduled relative to the initial render? Is there useEffect, or requestAnimationFrame, or setTimeout?

tabIndex is not set directly. It's managed by React. It uses requestAnimationFrame, but this is an implementation detail.

I see, there are utilities that do await act(), and creating a promise that resolves after a microtask tick, or after rAF, or after timeout... But that's an antipattern we're trying to avoid in Gutenberg, because it requires too much implementation knowledge from the test author.

I'm not sure what utilities you're talking about, but the functions we use are like await press.Tab(), await press.ArrowLeft(), await click(). Internally, those functions mimic what the browser does, including flushing microtasks between related events, animation frames, and timeouts when necessary to replicate an actual browser behavior (for example, unlike tap(), a real click should have a timeout between mouse/pointer down and mouse/pointer up). The fact that it handles the implementation details in the library is just a consequence of mimicking the browser behavior.

This is the opposite of implementation knowledge. Unless you're talking about browser implementation knowledge, which we had to study a lot to build those functions, but that's entirely abstracted into the testing library.

The preferred way is to use findBy or waitFor to detect state after all scheduled tasks are flushed. That's how we'll need to fix the failing EditPostPreferencesModal.

Since @ariakit/test is not ready for public use, I usually recommend using Playwright because it'll guarantee tests that don't use findBy or waitFor today won't fail if Ariakit or another UI library changes the implementation. Also, sometimes it's not obvious what you need to waitFor, or you'll end up waiting for some unrelated state just to get rid of an act warning or something else.

But I understand Playwright may be slower and less convenient than JSDOM. So that's an okay solution, I guess.

@jsnajdr
Copy link
Member

jsnajdr commented Aug 18, 2023

tabIndex is not set directly. It's managed by React.

Yes, I found the relevant code in the useCompositeItem hook 👍

the functions we use are like await press.Tab(), await press.ArrowLeft(), await click(). Internally, those functions mimic what the browser does,

Yes, I was talking about the utility functions that the helpers like press.Tab() use.

If it was only about mimicking browser behavior, that would be fine. But in that case Ariakit wouldn't need its own helpers, the helpers in @testing-library/user-event already mimick the browser behavior very well, including timeouts between the simulated events etc.

But unfortunately there is more. A call like:

await act( () => new Promise( r => requestAnimationFrame( r ) ) );

isn't there just to mimick browser behavior. If it was, it wouldn't need the React act() call. It's there mainly to prevent act() warnings on code like this:

function TabButton() {
  const [ tabIndex, setTabIndex ] = useState( undefined );
  useEffect( () => {
    requestAnimationFrame( () => {
      setTabIndex( -1 );
    } );
  }, [] );

  return <button tabIndex={ tabIndex } />;
}

If you didn't call that await act( requestAnimationFrame ), you'd get an act() warning, because that delayed setTabIndex call is scheduled asynchronously and executed outside the act environment's control.

So, when user is testing their React component that uses some library that somewhere deep down does the useEffect + rAF + setState combo, the deep implementation detail leaks very visibly out of the library into the user's test. Because it's the user's test that must do a matching await act() call.

We've seen many instances of that when migrating Gutenberg to React 18 and newer versions of Testing Library. floating-ui has async effects that leak out, and now Ariakit has them, too.

The solution is to:

  • carefully figure out what user-visible change does that async effect produce
  • wait for that user-visible change in the unit test

@diegohaz
Copy link
Member

If it was only about mimicking browser behavior, that would be fine. But in that case Ariakit wouldn't need its own helpers, the helpers in @testing-library/user-event already mimick the browser behavior very well, including timeouts between the simulated events etc.

The act function is specifically there to replicate browser behavior when using React, where effects are naturally flushed on timers. @ariakit/test doesn't have specific knowledge about @ariakit/react, floating-ui, framer-motion, react-router, and so on. However, it effectively works with all these libraries.

When I last experimented with user-event, it failed to correctly flush microtasks between events. This was the main reason we couldn't use it with Ariakit. The absence of act wasn't the only issue. This was years ago, and I see that it's greatly improved now. Nevertheless, to most accurately replicate browser behavior in a React application, it should either invoke act on timers or at least allow users to wrap their internal timers with a custom function. This way, users can pass React's act to it.

So, when user is testing their React component that uses some library that somewhere deep down does the useEffect + rAF + setState combo, the deep implementation detail leaks very visibly out of the library into the user's test. Because it's the user's test that must do a matching await act() call.

I respectfully disagree with your last statement. In almost all cases, a rAF within an effect or a queued microtask within an event handler should not leak into the user's test. If it does, this should not be deemed as a preferred solution.

Take your example, if a library invokes setTabIndex(-1), and then starts embedding it in rAF in a minor or patch version, your tests should not fail. As a user, you shouldn't have to spend time deciphering this and waitFor something just because of that. It's not required in the browser, it's not necessary with Playwright, and it's not needed with @ariakit/test. That's essentially what I'm trying to convey.

I realize that the current state of unit testing doesn't provide many solutions to this issue. But that is precisely what @ariakit/test aims to address. It's the complete opposite of "unfortunate."

@jsnajdr
Copy link
Member

jsnajdr commented Aug 21, 2023

if a library invokes setTabIndex(-1), and then starts embedding it in rAF in a minor or patch version, your tests should not fail.

The tests shouldn't fail, but, if they wait for specific fine-grained events inside the event loop, they will fail. It's very likely that if Ariakit's internal implementation changed, the @ariakit/test/click implementation would also have to change: the carefully placed await queuedMicrotasks() and await sleep() calls would need to placed differently. Because the individual React updates are now scheduled differently in the event loop.

Nevertheless, to most accurately replicate browser behavior in a React application, it should either invoke act on timers or at least allow users to wrap their internal timers with a custom function.

To really replicate accurate browser behavior in a React application, the tests would simply need to stop using act. Because the entire point of act is to disable the "native" scheduling of React updates, and instead schedule the updates on an artificial "act" queue. This queue can then be reliably flushed after the act callback returns. This lets us write synchronous tests for React components, even for code that runs useEffect, which is natively asynchronous.

Recent versions of React Testing Library completely disable the act environment when waitFor is active. Because that's the only way how to reliably run all the async updates the same way how browser would run them. To get Playwright style unit tests, Testing Library would need to run React without act, and do all checks asynchronously, i.e., with findBy. getBy becomes obsolete because we no longer can guarantee that anything happens sync.

@diegohaz
Copy link
Member

diegohaz commented Aug 21, 2023

The tests shouldn't fail, but, if they wait for specific fine-grained events inside the event loop, they will fail. It's very likely that if Ariakit's internal implementation changed, the @ariakit/test/click implementation would also have to change: the carefully placed await queuedMicrotasks() and await sleep() calls would need to placed differently. Because the individual React updates are now scheduled differently in the event loop.

queuedMicrotasks and sleep are carefully placed to mimic a real user action in the browser, where microtasks are flushed after individual dispatched events and some tasks might have a user delay between them. So, no, if Ariakit's internal implementation changes, the implementation of @ariakit/test shouldn't change. The only reason to modify @ariakit/test is if it's not executing actions the same way they would occur in the browser.

Recent versions of React Testing Library completely disable the act environment when waitFor is active. Because that's the only way how to reliably run all the async updates the same way how browser would run them. To get Playwright style unit tests, Testing Library would need to run React without act, and do all checks asynchronously, i.e., with findBy. getBy becomes obsolete because we no longer can guarantee that anything happens sync.

I wasn't aware you could completely disable the act environment. I'll take a look into that. We can definitely remove act from queuedMicrotasks and sleep if it's possible. Do you know if it also works with React 17?

@jsnajdr
Copy link
Member

jsnajdr commented Aug 23, 2023

I wasn't aware you could completely disable the act environment. [...] Do you know if it also works with React 17?

It's a matter of setting the global.IS_REACT_ACT_ENVIRONMENT to false, and not calling act. It's supported only since React 18, and is described very well in this discussion: reactwg/react-18#102. The "act is not the right API for all tests" section therein is particularly relevant.

I think that React Testing Library wants to maintain full control over the IS_REACT_ACT_ENVIRONMENT value, and it won't be easy to wrestle it away and set it reliably yourself. Might require some patches to support that.

Once you disable the act environment, React rendering and effects become fully async, scheduled with setImmediate, and you always need to poll until the expected DOM appears on the screen.

@diegohaz
Copy link
Member

It's a matter of setting the global.IS_REACT_ACT_ENVIRONMENT to false, and not calling act. It's supported only since React 18, and is described very well in this discussion: reactwg/react-18#102. The "act is not the right API for all tests" section therein is particularly relevant.

I think that React Testing Library wants to maintain full control over the IS_REACT_ACT_ENVIRONMENT value, and it won't be easy to wrestle it away and set it reliably yourself. Might require some patches to support that.

Once you disable the act environment, React rendering and effects become fully async, scheduled with setImmediate, and you always need to poll until the expected DOM appears on the screen.

Thanks! I tried setting this global value to false in beforeAll and removed all act from @ariakit/test. Interestingly, 2 out of 400 tests failed on @ariakit/react, but those might have been using getBy or something similar. So, it seems we can certainly do away with act.

[ instanceId ]
);

const tabStore = Ariakit.useTabStore( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just having a look at this PR — is there a reason why we imported * from Ariakit instead of importing only the needed components / functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually. Importing just the needed items would have been better. I'll plan a followup.

Copy link
Member

Choose a reason for hiding this comment

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

Just in case this is a factor, besides code style/aesthetics, there's no difference between importing the namespace and components individually. In the docs, we sometimes use the namespace import. The reasons are described in this section of the docs: https://ariakit.org/guide/coding-guidelines#import-namespace

diegohaz added a commit to ariakit/ariakit that referenced this pull request Oct 1, 2023
This PR removes the need for calling React's `act` when performing
asynchronous tasks using `@ariakit/test`. More details on
reactwg/react-18#102 and
WordPress/gutenberg#52133 (comment)

Some breaking changes have been introduced to the `@ariakit/test`
package. Refer to the changeset files for more details.
@properlypurple properlypurple mentioned this pull request Oct 11, 2023
10 tasks
@ciampo
Copy link
Contributor

ciampo commented Oct 12, 2023

See #48440 for the combined dev note

@ciampo ciampo added the has dev note when dev note is done (for upcoming WordPress release) label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system has dev note when dev note is done (for upcoming WordPress release) [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants