Skip to content

Commit 8899bb5

Browse files
getdavegetdave
authored
Normalize the Navigation block appender behavior between canvas and list view contexts (#71163)
* refactor: eliminate prop drilling for directInsert in PrivateListView - Add directInsert to ListViewContext to eliminate prop drilling - Update ListViewBranch to use context instead of props - Update Appender to use context instead of props - Maintain directInsert prop at PrivateListView level to set context value - Cleaner component interfaces and centralized configuration * Use block list settings api * Use original const def * Access setting directly in appender * Revert branch component to match trunk * Revert another file to trunk * Pass entire block to callback * Revert changes to menu controls * Properly handle cleanup if no link selected * Allow for setting blocks inserterd by Add block Quick Inserter * fix: clean up original nav link block when selecting different block in list view When using the 'Add block' feature in the Navigation block's list view context, the original auto-inserted navigation link block was not being cleaned up when a different block was selected via the QuickInserter. This caused duplicate blocks to remain in the list view. The fix adds a wrapper function handleSetInsertedBlock that removes the original inserted block before setting the new selected block, ensuring only the newly selected block remains in the list view. This aligns the list view behavior with the canvas context behavior. * fix: prevent focus stealing when selecting blocks in Navigation list view When selecting a different block from the QuickInserter in the Navigation block's list view context, the original auto-inserted block was being removed with automatic block selection enabled. This caused focus stealing that closed the list view and switched to the canvas. The fix adds preventBlockSelection = false to removeBlock() calls to prevent automatic block selection, keeping the list view open and maintaining proper focus behavior. * refactor: rename setInsertedBlock prop to onBlockInsert for better clarity Rename the setInsertedBlock prop to onBlockInsert across the Navigation block components to make the prop name more generic and descriptive of its purpose as a callback function that's called when a block is inserted. This improves code readability and makes the prop name more reusable across different contexts while maintaining all existing functionality. * fix: add iframe-aware cleanup for LinkUI in Navigation block List View - Add useEditorCanvasCleanup hook to handle iframe interaction cleanup - Fixes issue where auto-inserted Navigation Link blocks weren't cleaned up when clicking on canvas blocks in List View context - Uses standardized iframe selector approach consistent with rest of codebase - Hook automatically manages event listener lifecycle based on LinkUI visibility * fix: add iframe-aware cleanup for LinkUI in Navigation block List View - Add useEditorCanvasCleanup hook to handle iframe interaction cleanup - Fixes issue where auto-inserted Navigation Link blocks weren't cleaned up when clicking on canvas blocks in List View context - Uses standardized iframe selector approach consistent with rest of codebase - Hook automatically manages event listener lifecycle based on LinkUI visibility * Fix test to expect directly inserted page link * Fix another test to expect direct inserted Page block * tweak comments * Remove hacky fix to onClose failure * Revert alteration to hook * Optimise useSelect calls * Simplify cleanup function to avoid passing functions around * Fix mess up from rebase
1 parent 37da295 commit 8899bb5

6 files changed

Lines changed: 68 additions & 66 deletions

File tree

‎packages/block-editor/src/components/inserter/index.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -386,9 +386,7 @@ export default compose( [
386386
);
387387

388388
if ( onSelectOrClose ) {
389-
onSelectOrClose( {
390-
clientId: blockToInsert?.clientId,
391-
} );
389+
onSelectOrClose( blockToInsert );
392390
}
393391

394392
const message = sprintf(

‎packages/block-editor/src/components/list-view/appender.js‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,20 @@ export const Appender = forwardRef(
2222
const { insertedBlock, setInsertedBlock } = useListViewContext();
2323

2424
const instanceId = useInstanceId( Appender );
25-
const hideInserter = useSelect(
25+
const { directInsert, hideInserter } = useSelect(
2626
( select ) => {
27-
const { getTemplateLock, isZoomOut } = unlock(
28-
select( blockEditorStore )
29-
);
27+
const { getBlockListSettings, getTemplateLock, isZoomOut } =
28+
unlock( select( blockEditorStore ) );
3029

31-
return !! getTemplateLock( clientId ) || isZoomOut();
30+
const settings = getBlockListSettings( clientId );
31+
const directInsertValue = settings?.directInsert || false;
32+
const hideInserterValue =
33+
!! getTemplateLock( clientId ) || isZoomOut();
34+
35+
return {
36+
directInsert: directInsertValue,
37+
hideInserter: hideInserterValue,
38+
};
3239
},
3340
[ clientId ]
3441
);
@@ -79,7 +86,7 @@ export const Appender = forwardRef(
7986
position="bottom right"
8087
isAppender
8188
selectBlockOnInsert={ false }
82-
shouldDirectInsert={ false }
89+
shouldDirectInsert={ directInsert }
8390
__experimentalIsQuick
8491
{ ...props }
8592
toggleProps={ { 'aria-describedby': descriptionId } }

‎packages/block-library/src/navigation-link/link-ui.js‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ export function getSuggestionsQuery( type, kind ) {
7575
}
7676
}
7777

78-
function LinkUIBlockInserter( { clientId, onBack } ) {
78+
function LinkUIBlockInserter( { clientId, onBack, onBlockInsert } ) {
7979
const { rootBlockClientId } = useSelect(
8080
( select ) => {
8181
const { getBlockRootClientId } = select( blockEditorStore );
@@ -135,7 +135,8 @@ function LinkUIBlockInserter( { clientId, onBack } ) {
135135
clientId={ clientId }
136136
isAppender={ false }
137137
prioritizePatterns={ false }
138-
selectBlockOnInsert
138+
selectBlockOnInsert={ ! onBlockInsert }
139+
onSelect={ onBlockInsert ? onBlockInsert : undefined }
139140
hasSearch={ false }
140141
/>
141142
</div>
@@ -250,6 +251,7 @@ function UnforwardedLinkUI( props, ref ) {
250251
setFocusAddBlockButton( true );
251252
setFocusAddPageButton( false );
252253
} }
254+
onBlockInsert={ props?.onBlockInsert }
253255
/>
254256
) }
255257

‎packages/block-library/src/navigation/edit/leaf-more-menu.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,6 @@ export default function LeafMoreMenu( props ) {
146146
<AddSubmenuItem
147147
block={ block }
148148
onClose={ onClose }
149-
expanded
150149
expandedState={ props.expandedState }
151150
expand={ props.expand }
152151
setInsertedBlock={ props.setInsertedBlock }

‎packages/block-library/src/navigation/edit/menu-inspector-controls.js‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ const BLOCKS_WITH_LINK_UI_SUPPORT = [
3535
const { PrivateListView } = unlock( blockEditorPrivateApis );
3636

3737
function AdditionalBlockContent( { block, insertedBlock, setInsertedBlock } ) {
38-
const { updateBlockAttributes } = useDispatch( blockEditorStore );
38+
const { updateBlockAttributes, removeBlock } =
39+
useDispatch( blockEditorStore );
3940

4041
const supportsLinkControls = BLOCKS_WITH_LINK_UI_SUPPORT?.includes(
4142
insertedBlock?.name
@@ -47,6 +48,27 @@ function AdditionalBlockContent( { block, insertedBlock, setInsertedBlock } ) {
4748
return null;
4849
}
4950

51+
/**
52+
* Cleanup function for auto-inserted Navigation Link blocks.
53+
*
54+
* Removes the block if it has no URL and clears the inserted block state.
55+
* This ensures consistent cleanup behavior across different contexts.
56+
*/
57+
const cleanupInsertedBlock = () => {
58+
// Prevent automatic block selection when removing blocks in list view context
59+
// This avoids focus stealing that would close the list view and switch to canvas
60+
const shouldAutoSelectBlock = false;
61+
62+
// Follows the exact same pattern as Navigation Link block's onClose handler
63+
// If there is no URL then remove the auto-inserted block to avoid empty blocks
64+
if ( ! insertedBlock?.attributes?.url && insertedBlock?.clientId ) {
65+
// Remove the block entirely to avoid poor UX
66+
// This matches the Navigation Link block's behavior
67+
removeBlock( insertedBlock.clientId, shouldAutoSelectBlock );
68+
}
69+
setInsertedBlock( null );
70+
};
71+
5072
const setInsertedBlockAttributes =
5173
( _insertedBlockClientId ) => ( _updatedAttributes ) => {
5274
if ( ! _insertedBlockClientId ) {
@@ -55,12 +77,28 @@ function AdditionalBlockContent( { block, insertedBlock, setInsertedBlock } ) {
5577
updateBlockAttributes( _insertedBlockClientId, _updatedAttributes );
5678
};
5779

80+
// Wrapper function to clean up original block when a new block is selected
81+
const handleSetInsertedBlock = ( newBlock ) => {
82+
// Prevent automatic block selection when removing blocks in list view context
83+
// This avoids focus stealing that would close the list view and switch to canvas
84+
const shouldAutoSelectBlock = false;
85+
86+
// If we have an existing inserted block and a new block is being set,
87+
// remove the original block to avoid duplicates
88+
if ( insertedBlock?.clientId && newBlock ) {
89+
removeBlock( insertedBlock.clientId, shouldAutoSelectBlock );
90+
}
91+
setInsertedBlock( newBlock );
92+
};
93+
5894
return (
5995
<LinkUI
6096
clientId={ insertedBlock?.clientId }
6197
link={ insertedBlock?.attributes }
98+
onBlockInsert={ handleSetInsertedBlock }
6299
onClose={ () => {
63-
setInsertedBlock( null );
100+
// Use cleanup function
101+
cleanupInsertedBlock();
64102
} }
65103
onChange={ ( updatedValue ) => {
66104
updateAttributes(
@@ -70,9 +108,6 @@ function AdditionalBlockContent( { block, insertedBlock, setInsertedBlock } ) {
70108
);
71109
setInsertedBlock( null );
72110
} }
73-
onCancel={ () => {
74-
setInsertedBlock( null );
75-
} }
76111
/>
77112
);
78113
}

‎test/e2e/specs/editor/blocks/navigation-list-view.spec.js‎

Lines changed: 10 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -171,31 +171,8 @@ test.describe( 'Navigation block - List view editing', () => {
171171

172172
await appender.click();
173173

174-
// Expect to see the block inserter.
175-
await expect(
176-
page.getByRole( 'searchbox', {
177-
name: 'Search',
178-
} )
179-
).toBeFocused();
180-
181-
const blockResults = page.getByRole( 'listbox', {
182-
name: 'Blocks',
183-
} );
184-
185-
await expect( blockResults ).toBeVisible();
186-
187-
const blockResultOptions = blockResults.getByRole( 'option' );
188-
189-
// Expect to see the Page Link and Custom Link blocks as the nth(0) and nth(1) results.
190-
// This is important for usability as the Page Link block is the most likely to be used.
191-
await expect( blockResultOptions.nth( 0 ) ).toHaveText( 'Page Link' );
192-
await expect( blockResultOptions.nth( 1 ) ).toHaveText( 'Custom Link' );
193-
194-
// Select the Page Link option.
195-
const customLinkResult = blockResultOptions.nth( 1 );
196-
await customLinkResult.click();
197-
198-
// Expect to see the Link creation UI be focused.
174+
// Expect a Navigation Link block to be inserted
175+
// and immediately trigger its Link UI.
199176
const linkUIInput = linkControl.getSearchInput();
200177

201178
// Coverage for bug whereby Link UI input would be incorrectly prepopulated.
@@ -215,17 +192,17 @@ test.describe( 'Navigation block - List view editing', () => {
215192
const thirdResult = await linkControl.getNthSearchResult( 2 );
216193

217194
const firstResultType =
218-
await linkControl.getSearchResultType( firstResult );
195+
await linkControl.getSearchResultText( firstResult );
219196

220197
const secondResultType =
221-
await linkControl.getSearchResultType( secondResult );
198+
await linkControl.getSearchResultText( secondResult );
222199

223200
const thirdResultType =
224-
await linkControl.getSearchResultType( thirdResult );
201+
await linkControl.getSearchResultText( thirdResult );
225202

226-
expect( firstResultType ).toBe( 'Page' );
227-
expect( secondResultType ).toBe( 'Page' );
228-
expect( thirdResultType ).toBe( 'Page' );
203+
expect( firstResultType ).toContain( 'Page' );
204+
expect( secondResultType ).toContain( 'Page' );
205+
expect( thirdResultType ).toContain( 'Page' );
229206

230207
// Grab the text from the first result so we can check (later on) that it was inserted.
231208
const firstResultText =
@@ -503,16 +480,8 @@ test.describe( 'Navigation block - List view editing', () => {
503480
} )
504481
.click();
505482

506-
const blockResults = page.getByRole( 'listbox', {
507-
name: 'Blocks',
508-
} );
509-
510-
await expect( blockResults ).toBeVisible();
511-
512-
const blockResultOptions = blockResults.getByRole( 'option' );
513-
514-
// Select the Page Link option.
515-
await blockResultOptions.nth( 0 ).click();
483+
// Expect the Link UI to be focused.
484+
await expect( linkControl.getSearchInput() ).toBeFocused();
516485

517486
// Immediately dismiss the Link UI thereby not populating the `url` attribute
518487
// of the block.
@@ -631,12 +600,4 @@ class LinkControl {
631600
.locator( '.components-menu-item__item' ) // this is the only way to get the label text without the URL.
632601
.innerText();
633602
}
634-
635-
async getSearchResultType( result ) {
636-
await expect( result ).toBeVisible();
637-
638-
return result
639-
.locator( '.components-menu-item__shortcut' ) // this is the only way to get the type text.
640-
.innerText();
641-
}
642603
}

0 commit comments

Comments
 (0)