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

Composite: make items tabbable if active element gets removed #65720

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 28, 2024

What?

May fix #65283

This PR tweaks the behavior of the Composite component so that the composite widget can continue to receive keyboard focus even when its active ID doesn't correspond to an element rendered in the DOM.

Why?

The fact that Composite can end up in a situation where the whole widget is not focusable is bad for assistive technology, and we should try to fix it.

How?

Following a suggestion from the Ariakit maintainers, this PR makes all composite items tabbable if the element associated with the active ID is not connected to the store.

In this case, when the focus is moved back to the composite widget, the items will be able to receive focus. As soon as one item receives focus, all composite items will stop being tabbable and revert back to the standard behavior of a composite widget.

Review suggestions

Review commit-by-commit. The actual fix is applied in the first commit. The rest of the commits are accessories to the main fix.

Testing Instructions

In Storybook:

  • Load the "Remove active item" example
  • Focus the composite widget, and with the arrow keys move focus on the third item
  • Tab to the "Toggle third item button" and click it
  • Tab back to the composite widget: the second item should receive focus and become active
  • Moving arrow keys should work as expected in the composite widget
Edit: it looks like the UI has changed and it's impossible to reproduce the bug in the editor as of now

In the editor:

Screenshots or screencast

Before (trunk) After (this PR)
Kapture.2024-09-30.at.14.18.39.mp4
Kapture.2024-09-30.at.14.22.18.mp4

@ciampo ciampo self-assigned this Sep 28, 2024
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Sep 30, 2024
@ciampo ciampo requested review from afercia and a team September 30, 2024 14:46
@ciampo ciampo marked this pull request as ready for review September 30, 2024 14:46
Copy link

github-actions bot commented Sep 30, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: diegohaz <hazdiego@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably add a suite of tests similar to the legacy version. I can do that in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a plan

Copy link

github-actions bot commented Sep 30, 2024

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

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.

Looks good, works well 👍

Thanks for adding tests and a special storybook story to manually test it in 🙇‍♂️

Not approving just yet since we'll need to clean up after merging #65823 and #65821.

packages/components/src/composite/item.tsx Show resolved Hide resolved
packages/components/src/composite/stories/index.story.tsx Outdated Show resolved Hide resolved
packages/components/src/composite/test/index.tsx Outdated Show resolved Hide resolved
@ciampo
Copy link
Contributor Author

ciampo commented Oct 2, 2024

Rebased to include changes from #65823 and #65821 (which were merged in the meantime), and addressed other pending feedback

@tyxla this is ready for another round of review

@ciampo ciampo requested a review from tyxla October 2, 2024 15:39
@ciampo ciampo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 2, 2024
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 🚀

@ciampo ciampo enabled auto-merge (squash) October 3, 2024 10:56
@ciampo ciampo removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 3, 2024
@ciampo
Copy link
Contributor Author

ciampo commented Oct 3, 2024

I'll enable backporting once #65853 gets merged

@ciampo ciampo merged commit edcd976 into trunk Oct 3, 2024
64 checks passed
@ciampo ciampo deleted the fix/composite-focus-loss branch October 3, 2024 11:32
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Oct 3, 2024
@ciampo ciampo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 3, 2024
gutenbergplugin pushed a commit that referenced this pull request Oct 3, 2024
* Composite: make items tabbable when the active element is disconnected

* Add unit test

* Better test name

* CHANGELOG

---

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: diegohaz <hazdiego@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
@github-actions github-actions bot added Backported to WP Core Pull request that has been successfully merged into WP Core 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 3, 2024
Copy link

github-actions bot commented Oct 3, 2024

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 54ee0ee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options in the CircularOptionPicker may be not operable with the keyboard
3 participants