Components: Add autoFocus prop to AlignmentMatrixControl, use for Cover block toolbar - #79354
Components: Add autoFocus prop to AlignmentMatrixControl, use for Cover block toolbar#79354aduth wants to merge 4 commits into
Conversation
Ariakit Composite initialization is asynchronous via requestAnimationFrame, because its roving tabindex can only be computed once all cell items are rendered. This asynchronous initialization can conflict with other autofocus behaviors which themselves may be asynchronous in different ways (e.g. useFocusOnMount scheduling via `setTimeout(0)`). In the browser this is usually fine, but it's not a strong guarantee, and breaks in environments like JSDOM which define their own unique behavior for scheduling (rAF as a 16ms timeout). If useFocusOnMount runs before composite initializes, all composite items are tabbable, and the first cell is selected, but the expected behavior would be that focus is only assigned once initialized, since it may not be the first cell that's the active one.
… in block toolbar
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +56 B (0%) Total Size: 7.5 MB 📦 View Changed
|
| return ( | ||
| <Dropdown | ||
| popoverProps={ { placement: 'bottom-start' } } | ||
| popoverProps={ { placement: 'bottom-start', focusOnMount: false } } |
There was a problem hiding this comment.
It appears we can't really do this because disabling focusOnMount opts us out from some necessary keyboard navigation behaviors:
- Focus trapping when tabbing within the popover.
- Returning focus to the toolbar item after closing the popover with Escape.
There was a problem hiding this comment.
Good catch, though I find it a bit surprising that those behaviors are linked to a prop which advertises itself as controlling an "on mount" behavior. I'll give this a think. And the absence of failing tests on this regression is separately concerning.
There was a problem hiding this comment.
We need to hook it up with getWpCompatOverlaySlot() first, but this may be a good place to try swapping the Dropdown with the new Popover and see if the behavior is any better.
|
As a less elegant (but simpler) alternative, could we programmatically focus the correct DOM element in the I also agree with @mirka's suggestion to test the new |
What?
Adds a new
autoFocusprop to theAlignmentMatrixControlcomponent and updates the Cover block to use it in place of relying on DropdownfocusOnMount.Why?
Seeks to avoid a possible race condition between
AlignmentMatrixControl's asynchronous initialization andfocusOnMount's asynchronous detection of focusable elements.The extended commit description has more details:
This was motivated by #79353, which had to work around this issue by introducing a test utility overriding
requestAnimationFrameto emulate the browser behavior. But the browser behavior is not a strong guarantee, and there is a real risk of a race condition.Testing Instructions
Verify that Cover block tests pass:
npm run test:unit packages/block-library/src/cover/test/edit.js(note that Cover block is the only consumer of this component)Verify that focus shifts to the alignment matrix center cell and does not dirty post in doing so:
Screenshots or screencast
Use of AI Tools
There was a lot of deep research here between Cursor Composer and Claude Opus 4.8 models. Implementations were initially authored by AI, with further refinement and iterations based on continued research. This was split from work that started in #79353.