Fixes issues with tab not able to edit tab title and also removes tab lists when tabs are toggled off - #173
Conversation
…ole attributes and add unit tests
…on/removal of tab list blocks in carousel edit
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR addresses carousel “tabs mode” usability and consistency by improving tab title editability, ensuring tab list blocks are inserted/removed when toggling tabs, and adding supporting tree utilities + tests.
Changes:
- Added
findAllBlocksDeeputility to locate all matching blocks in a nested block tree (with unit tests). - Updated carousel tab list edit markup to use ARIA tab roles and selection state.
- Updated carousel editor logic to avoid duplicate tab list insertion and to remove all tab list blocks when tabs are disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/blocks/carousel/types.ts | Adds findAllBlocksDeep DFS utility for matching blocks across the full tree. |
| src/blocks/carousel/edit.tsx | Uses new utility to manage tab list insertion/removal when toggling tabs mode. |
| src/blocks/carousel/carousel-tab-list/edit.tsx | Changes tab label RichText element to span with role="tab" and aria-selected. |
| src/blocks/carousel/carousel-tab-list/tests/edit.test.tsx | Adds unit test coverage for tab markup/ARIA attributes in the edit component. |
| src/blocks/carousel/tests/types.test.ts | Adds unit tests for findAllBlocksDeep. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…improve tab keyboard accessibility via tabIndex handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/blocks/carousel/carousel-tab-list/edit.tsx:183
- The ARIA tabs pattern typically requires each
role="tab"element to have a stableidand anaria-controlspointing at the associatedrole="tabpanel"element (and the tabpanel should reference the tab viaaria-labelledby). Consider adding these relationships here (or ensuring they exist in the surrounding markup) so screen readers can correctly associate tabs with their panels.
tagName="span"
role="tab"
aria-selected={ index === carousel.selectedIndex }
tabIndex={ index === carousel.selectedIndex ? 0 : -1 }
src/blocks/carousel/edit.tsx:369
innerBlockshere comes from the current render snapshot, which can be stale during rapid toggling (e.g., toggle ON inserts a tab list, then immediately toggle OFF before a rerender updatesinnerBlocks). In that case, the OFF path may fail to remove the just-inserted tab list. A more robust approach is to read the latest blocks from the block-editor store inside the handler (or move insert/remove logic into an effect keyed offuseTabs+ current blocks) so removal always targets the current tree.
} else {
setAttributes( { useTabs: false } );
const tabListBlocks = findAllBlocksDeep( innerBlocks, 'rt-carousel/carousel-tab-list' );
const tabListIds = tabListBlocks.map( ( b ) => b.clientId );
if ( tabListIds.length > 0 ) {
removeBlocks( tabListIds );
}
}
src/blocks/carousel/carousel-tab-list/tests/edit.test.tsx:53
- This
useSelectmock returns a constant for all selectors, which is brittle if the component adds anotheruseSelectcall or expects different return shapes. Consider mockinguseSelectto invoke the provided selector with a minimalselectfunction/object (similar to the carousel edit tests) so each selector can compute the intended value without coupling the test to the current number ofuseSelectinvocations.
jest.mock( '@wordpress/data', () => ( {
useSelect: jest.fn( () => 2 ),
} ) );
src/blocks/carousel/types.ts:109
result.push( ...findAllBlocksDeep(...) )allocates a new array for every subtree and then spreads it, which can add overhead for large trees. Consider implementing this with an accumulator parameter (or an iterative stack) to avoid repeated intermediate arrays.
const result: T[] = [];
for ( const block of blocks ) {
if ( block.name === name ) {
result.push( block );
}
if ( block.innerBlocks?.length ) {
result.push( ...findAllBlocksDeep( block.innerBlocks, name ) );
}
}
Summary
The main changes include adding a utility to find all matching blocks in a tree, updating the carousel tab list to use accessible markup, and ensuring tab lists are automatically inserted or removed based on settings. Comprehensive unit tests have also been added to cover these new behaviors.
Block tree utilities:
findAllBlocksDeeputility to recursively find all blocks of a given name in a block tree, with unit tests verifying its correctness. [1] [2] [3]Carousel tab list accessibility and behavior:
<span>elements with appropriate ARIA roles (role="tab",aria-selected) for accessibility, instead of buttons.Automatic tab list management:
removeBlocksaction. [1] [2] [3]Type of change
Related issue(s)
Closes #171, #172
What changed
Breaking changes
Does this introduce a breaking change? If yes, describe the impact and migration path below.
Testing
Describe how this was tested.
Test details:
Screenshots / recordings
If applicable, add screenshots or short recordings.
Checklist