From 96f01f3a6b627b59b1cf854cf07407587597b39a Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Wed, 18 Dec 2024 14:52:07 -0600 Subject: [PATCH 01/14] Move linkUI to own custom appender in navigation block --- .../components/block-list-appender/index.js | 2 +- .../src/navigation/edit/inner-blocks.js | 59 ++++++++++++++++--- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/packages/block-editor/src/components/block-list-appender/index.js b/packages/block-editor/src/components/block-list-appender/index.js index 4c65ead6b4e62c..b6576ad4fd829e 100644 --- a/packages/block-editor/src/components/block-list-appender/index.js +++ b/packages/block-editor/src/components/block-list-appender/index.js @@ -94,7 +94,7 @@ export default function BlockListAppender( { data-block > { CustomAppender ? ( - + ) : ( ) } diff --git a/packages/block-library/src/navigation/edit/inner-blocks.js b/packages/block-library/src/navigation/edit/inner-blocks.js index 6fe3dd8347a33e..aabc628164902c 100644 --- a/packages/block-library/src/navigation/edit/inner-blocks.js +++ b/packages/block-library/src/navigation/edit/inner-blocks.js @@ -4,17 +4,59 @@ import { useEntityBlockEditor } from '@wordpress/core-data'; import { useInnerBlocksProps, - InnerBlocks, store as blockEditorStore, } from '@wordpress/block-editor'; import { useSelect } from '@wordpress/data'; -import { useMemo } from '@wordpress/element'; - +import { useMemo, useState, useRef } from '@wordpress/element'; +import { Icon, plus } from '@wordpress/icons'; +import { Button } from '@wordpress/components'; /** * Internal dependencies */ import PlaceholderPreview from './placeholder/placeholder-preview'; -import { DEFAULT_BLOCK, PRIORITIZED_INSERTER_BLOCKS } from '../constants'; +import { PRIORITIZED_INSERTER_BLOCKS } from '../constants'; +import { LinkUI } from '../../navigation-link/link-ui'; + +function AddLinkAppender( { rootClientId } ) { + const [ isLinkOpen, setIsLinkOpen ] = useState( false ); + const [ popoverAnchor, setPopoverAnchor ] = useState( null ); + const linkUIref = useRef(); + + return ( + <> + + { isLinkOpen && ( + { + setIsLinkOpen( false ); + } } + anchor={ popoverAnchor } + /> + ) } + + ); +} export default function NavigationInnerBlocks( { clientId, @@ -82,7 +124,6 @@ export default function NavigationInnerBlocks( { onInput, onChange, prioritizedInserterBlocks: PRIORITIZED_INSERTER_BLOCKS, - defaultBlock: DEFAULT_BLOCK, directInsert: true, orientation, templateLock, @@ -98,7 +139,7 @@ export default function NavigationInnerBlocks( { ! selectedBlockHasChildren ) || // Show the appender while dragging to allow inserting element between item and the appender. parentOrChildHasSelection - ? InnerBlocks.ButtonBlockAppender + ? AddLinkAppender : false, placeholder: showPlaceholder ? placeholder : undefined, __experimentalCaptureToolbars: true, @@ -106,5 +147,9 @@ export default function NavigationInnerBlocks( { } ); - return
; + return ( + <> +
+ + ); } From 8ff71d7c469843c3caa8f50558737e20e2c6fae1 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Wed, 18 Dec 2024 16:15:37 -0600 Subject: [PATCH 02/14] Create link or block from NavigationLinkAppender --- .../block-library/src/navigation-link/edit.js | 42 +------------------ .../src/navigation-link/link-ui.js | 17 +------- .../src/navigation/edit/inner-blocks.js | 18 ++++++-- 3 files changed, 18 insertions(+), 59 deletions(-) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index 39073b848d3ca8..c1a64a0f696497 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -42,8 +42,6 @@ import { LinkUI } from './link-ui'; import { updateAttributes } from './update-attributes'; import { getColors } from '../navigation/edit/utils'; -const DEFAULT_BLOCK = { name: 'core/navigation-link' }; - /** * A React hook to determine if it's dragging within the target element. * @@ -248,12 +246,10 @@ export default function NavigationLinkEdit( { replaceBlock, __unstableMarkNextChangeAsNotPersistent, selectBlock, - selectPreviousBlock, } = useDispatch( blockEditorStore ); // Have the link editing ui open on mount when lacking a url and selected. const [ isLinkOpen, setIsLinkOpen ] = useState( isSelected && ! url ); // Store what element opened the popover, so we know where to return focus to (toolbar button vs navigation link text) - const [ openedBy, setOpenedBy ] = useState( null ); // Use internal state instead of a ref to make sure that the component // re-renders when the popover's anchor updates. const [ popoverAnchor, setPopoverAnchor ] = useState( null ); @@ -398,7 +394,6 @@ export default function NavigationLinkEdit( { // If this link is a child of a parent submenu item, the parent submenu item event will also open, closing this popover event.stopPropagation(); setIsLinkOpen( true ); - setOpenedBy( ref.current ); } } @@ -428,7 +423,6 @@ export default function NavigationLinkEdit( { className: 'remove-outline', // Remove the outline from the inner blocks container. }, { - defaultBlock: DEFAULT_BLOCK, directInsert: true, renderAppender: false, } @@ -437,7 +431,6 @@ export default function NavigationLinkEdit( { if ( ! url || isInvalid || isDraft ) { blockProps.onClick = () => { setIsLinkOpen( true ); - setOpenedBy( ref.current ); }; } @@ -464,9 +457,8 @@ export default function NavigationLinkEdit( { icon={ linkIcon } title={ __( 'Link' ) } shortcut={ displayShortcut.primary( 'k' ) } - onClick={ ( event ) => { + onClick={ () => { setIsLinkOpen( true ); - setOpenedBy( event.currentTarget ); } } /> { ! isAtMaxNesting && ( @@ -569,39 +561,7 @@ export default function NavigationLinkEdit( { clientId={ clientId } link={ attributes } onClose={ () => { - // If there is no link then remove the auto-inserted block. - // This avoids empty blocks which can provided a poor UX. - if ( ! url ) { - // Fixes https://github.com/WordPress/gutenberg/issues/61361 - // There's a chance we're closing due to the user selecting the browse all button. - // Only move focus if the focus is still within the popover ui. If it's not within - // the popover, it's because something has taken the focus from the popover, and - // we don't want to steal it back. - if ( - linkUIref.current.contains( - window.document.activeElement - ) - ) { - // Select the previous block to keep focus nearby - selectPreviousBlock( clientId, true ); - } - - // Remove the link. - onReplace( [] ); - return; - } - setIsLinkOpen( false ); - if ( openedBy ) { - openedBy.focus(); - setOpenedBy( null ); - } else if ( ref.current ) { - // select the ref when adding a new link - ref.current.focus(); - } else { - // Fallback - selectPreviousBlock( clientId, true ); - } } } anchor={ popoverAnchor } onRemove={ removeLink } diff --git a/packages/block-library/src/navigation-link/link-ui.js b/packages/block-library/src/navigation-link/link-ui.js index 52db034c6f980c..a57097e39966ed 100644 --- a/packages/block-library/src/navigation-link/link-ui.js +++ b/packages/block-library/src/navigation-link/link-ui.js @@ -11,7 +11,6 @@ import { import { __, sprintf, isRTL } from '@wordpress/i18n'; import { LinkControl, - store as blockEditorStore, privateApis as blockEditorPrivateApis, } from '@wordpress/block-editor'; import { @@ -27,7 +26,7 @@ import { useResourcePermissions, } from '@wordpress/core-data'; import { decodeEntities } from '@wordpress/html-entities'; -import { useSelect, useDispatch } from '@wordpress/data'; +import { useDispatch } from '@wordpress/data'; import { chevronLeftSmall, chevronRightSmall, plus } from '@wordpress/icons'; import { useInstanceId, useFocusOnMount } from '@wordpress/compose'; @@ -79,17 +78,6 @@ export function getSuggestionsQuery( type, kind ) { } function LinkUIBlockInserter( { clientId, onBack, onSelectBlock } ) { - const { rootBlockClientId } = useSelect( - ( select ) => { - const { getBlockRootClientId } = select( blockEditorStore ); - - return { - rootBlockClientId: getBlockRootClientId( clientId ), - }; - }, - [ clientId ] - ); - const focusOnMountRef = useFocusOnMount( 'firstElement' ); const dialogTitleId = useInstanceId( @@ -134,8 +122,7 @@ function LinkUIBlockInserter( { clientId, onBack, onSelectBlock } ) { { + const block = createBlock( 'core/navigation-link', attributes ); + + insertBlock( block, undefined, rootClientId ); + }; + return ( <> + { isLinkOpen && ( + { + setIsLinkOpen( false ); + } } + onChange={ ( updatedValue ) => { + updateAttributes( updatedValue, createNewLink ); + } } + anchor={ popoverAnchor } + /> + ) } + + ); +} diff --git a/packages/block-library/src/navigation-submenu/edit.js b/packages/block-library/src/navigation-submenu/edit.js index dbdbd23b13b2f6..01dd72546d5838 100644 --- a/packages/block-library/src/navigation-submenu/edit.js +++ b/packages/block-library/src/navigation-submenu/edit.js @@ -19,7 +19,6 @@ import { displayShortcut, isKeyboardEvent } from '@wordpress/keycodes'; import { __ } from '@wordpress/i18n'; import { BlockControls, - InnerBlocks, useInnerBlocksProps, InspectorControls, RichText, @@ -39,6 +38,7 @@ import { useMergeRefs, usePrevious } from '@wordpress/compose'; */ import { ItemSubmenuIcon } from './icons'; import { LinkUI } from '../navigation-link/link-ui'; +import NavigationLinkAppender from '../navigation-link/appender'; import { updateAttributes } from '../navigation-link/update-attributes'; import { getColors, @@ -330,7 +330,7 @@ export default function NavigationSubmenuEdit( { ! selectedBlockHasChildren ) || // Show the appender while dragging to allow inserting element between item and the appender. hasChildren - ? InnerBlocks.ButtonBlockAppender + ? NavigationLinkAppender : false, } ); diff --git a/packages/block-library/src/navigation/edit/inner-blocks.js b/packages/block-library/src/navigation/edit/inner-blocks.js index c851650bdc34c3..32c36522355399 100644 --- a/packages/block-library/src/navigation/edit/inner-blocks.js +++ b/packages/block-library/src/navigation/edit/inner-blocks.js @@ -6,69 +6,15 @@ import { useInnerBlocksProps, store as blockEditorStore, } from '@wordpress/block-editor'; -import { useDispatch, useSelect } from '@wordpress/data'; -import { useMemo, useState, useRef } from '@wordpress/element'; -import { Icon, plus } from '@wordpress/icons'; -import { Button } from '@wordpress/components'; -import { createBlock } from '@wordpress/blocks'; +import { useSelect } from '@wordpress/data'; +import { useMemo } from '@wordpress/element'; + /** * Internal dependencies */ import PlaceholderPreview from './placeholder/placeholder-preview'; import { PRIORITIZED_INSERTER_BLOCKS } from '../constants'; -import { LinkUI } from '../../navigation-link/link-ui'; -import { updateAttributes } from '../../navigation-link/update-attributes'; - -function NavigationLinkAppender( { rootClientId } ) { - const { insertBlock } = useDispatch( blockEditorStore ); - const [ isLinkOpen, setIsLinkOpen ] = useState( false ); - const [ popoverAnchor, setPopoverAnchor ] = useState( null ); - const linkUIref = useRef(); - - const createNewLink = ( attributes ) => { - const block = createBlock( 'core/navigation-link', attributes ); - - insertBlock( block, undefined, rootClientId ); - }; - - return ( - <> - - { isLinkOpen && ( - { - setIsLinkOpen( false ); - } } - onChange={ ( updatedValue ) => { - updateAttributes( updatedValue, createNewLink ); - } } - anchor={ popoverAnchor } - /> - ) } - - ); -} +import NavigationLinkAppender from '../../navigation-link/appender'; export default function NavigationInnerBlocks( { clientId, From f0bf3610571d18146c1d3f2f793ea8d43c0633b5 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Wed, 18 Dec 2024 16:29:54 -0600 Subject: [PATCH 04/14] Remove openedBy ref from submenus --- .../src/navigation-submenu/edit.js | 20 +++---------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/packages/block-library/src/navigation-submenu/edit.js b/packages/block-library/src/navigation-submenu/edit.js index 01dd72546d5838..7720f390ce8850 100644 --- a/packages/block-library/src/navigation-submenu/edit.js +++ b/packages/block-library/src/navigation-submenu/edit.js @@ -138,14 +138,9 @@ export default function NavigationSubmenuEdit( { const { showSubmenuIcon, maxNestingLevel, openSubmenusOnClick } = context; - const { - __unstableMarkNextChangeAsNotPersistent, - replaceBlock, - selectBlock, - } = useDispatch( blockEditorStore ); + const { __unstableMarkNextChangeAsNotPersistent, replaceBlock } = + useDispatch( blockEditorStore ); const [ isLinkOpen, setIsLinkOpen ] = useState( false ); - // Store what element opened the popover, so we know where to return focus to (toolbar button vs navigation link text) - const [ openedBy, setOpenedBy ] = useState( null ); // Use internal state instead of a ref to make sure that the component // re-renders when the popover's anchor updates. const [ popoverAnchor, setPopoverAnchor ] = useState( null ); @@ -277,7 +272,6 @@ export default function NavigationSubmenuEdit( { // If we don't stop propogation, this event bubbles up to the parent submenu item event.stopPropagation(); setIsLinkOpen( true ); - setOpenedBy( ref.current ); } } @@ -364,9 +358,8 @@ export default function NavigationSubmenuEdit( { icon={ linkIcon } title={ __( 'Link' ) } shortcut={ displayShortcut.primary( 'k' ) } - onClick={ ( event ) => { + onClick={ () => { setIsLinkOpen( true ); - setOpenedBy( event.currentTarget ); } } /> ) } @@ -517,7 +510,6 @@ export default function NavigationSubmenuEdit( { onClick={ () => { if ( ! openSubmenusOnClick && ! url ) { setIsLinkOpen( true ); - setOpenedBy( ref.current ); } } } /> @@ -532,12 +524,6 @@ export default function NavigationSubmenuEdit( { link={ attributes } onClose={ () => { setIsLinkOpen( false ); - if ( openedBy ) { - openedBy.focus(); - setOpenedBy( null ); - } else { - selectBlock( clientId ); - } } } anchor={ popoverAnchor } onRemove={ () => { From 3d6c8a80c1a760dfb64d112a7d3dd6f3f2502db3 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Wed, 18 Dec 2024 21:59:54 -0600 Subject: [PATCH 05/14] Move navigation block tests to steps --- .../specs/editor/blocks/navigation.spec.js | 313 +++++++++--------- 1 file changed, 153 insertions(+), 160 deletions(-) diff --git a/test/e2e/specs/editor/blocks/navigation.spec.js b/test/e2e/specs/editor/blocks/navigation.spec.js index 83e95a08c0f6a2..f7217ef09606b6 100644 --- a/test/e2e/specs/editor/blocks/navigation.spec.js +++ b/test/e2e/specs/editor/blocks/navigation.spec.js @@ -338,190 +338,183 @@ test.describe( 'Navigation block', () => { // Wait until the nav block inserter is visible before we continue. Otherwise the navigation block may not have finished being created. await expect( navBlockInserter ).toBeVisible(); - /** - * Test: We don't lose focus when using the navigation link appender - */ - await pageUtils.pressKeys( 'ArrowDown' ); - await navigation.useBlockInserter(); - await navigation.addLinkClose(); - /** - * TODO: This is not desired behavior. Ideally the - * Appender should be focused again since it opened - * the link control. - * IMPORTANT: This check is not to enforce this behavior, - * but to make sure focus is kept nearby until we are able - * to send focus to the appender. - */ - await expect( navBlock ).toBeFocused(); + await test.step( 'should not lose focus when using the navigation link appender', async () => { + await pageUtils.pressKeys( 'ArrowDown' ); + await navigation.useBlockInserter(); + await navigation.addLinkClose(); + /** + * TODO: This is not desired behavior. Ideally the + * Appender should be focused again since it opened + * the link control. + * IMPORTANT: This check is not to enforce this behavior, + * but to make sure focus is kept nearby until we are able + * to send focus to the appender. + */ + await expect( navBlock ).toBeFocused(); + } ); - /** - * Test: Creating a link sends focus to the newly created navigation link item - */ - await pageUtils.pressKeys( 'ArrowDown' ); + await test.step( 'should send focus to the newly created navigation link item when creating a link', async () => { + await pageUtils.pressKeys( 'ArrowDown' ); - await navigation.useBlockInserter(); - await navigation.addPage( 'Cat' ); - /** - * Test: We can open and close the preview with the keyboard and escape - * buttons from a top-level nav item using both the shortcut and toolbar - */ - await navigation.useLinkShortcut(); - await navigation.previewIsOpenAndCloses(); - await navigation.checkLabelFocus( 'Cat' ); + await navigation.useBlockInserter(); + await navigation.addPage( 'Cat' ); + } ); - await navigation.canUseToolbarLink(); + await test.step( 'should open and close the preview with the keyboard and escape buttons from a top-level nav item using the shortcut', async () => { + await navigation.useLinkShortcut(); + await navigation.previewIsOpenAndCloses(); + await navigation.checkLabelFocus( 'Cat' ); + } ); - /** - * Test: Creating a link from a url-string (https://www.example.com) returns - * focus to the newly created link with the text selected - */ - // Move focus to the Add Block Appender. - await page.keyboard.press( 'Escape' ); - await pageUtils.pressKeys( 'ArrowDown' ); - await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); + await test.step( 'should open and close the preview with the keyboard and escape buttons from a top-level nav item using the toolbar button', async () => { + await navigation.canUseToolbarLink(); + } ); - await navigation.useBlockInserter(); - await navigation.addCustomURL( 'https://example.com' ); - await navigation.expectToHaveTextSelected( 'example.com' ); + await test.step( 'should send focus to the newly created navigation link item with text selected when creating a link from a url-like label', async () => { + await page.keyboard.press( 'Escape' ); + await pageUtils.pressKeys( 'ArrowDown' ); + await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); - /** - * Test: We can open and close the preview with the keyboard and escape - * buttons from a top-level nav link with a url-like label using - * both the shortcut and toolbar - */ - await pageUtils.pressKeys( 'ArrowLeft' ); - await navigation.useLinkShortcut(); - await navigation.previewIsOpenAndCloses(); - await navigation.checkLabelFocus( 'example.com' ); + await navigation.useBlockInserter(); + await navigation.addCustomURL( 'https://example.com' ); + await navigation.expectToHaveTextSelected( 'example.com' ); + } ); - await navigation.canUseToolbarLink(); + await test.step( 'should open and close the preview with the keyboard and escape buttons from a top-level nav item with a url-like label using the shortcut', async () => { + await pageUtils.pressKeys( 'ArrowLeft' ); + await navigation.useLinkShortcut(); + await navigation.previewIsOpenAndCloses(); + await navigation.checkLabelFocus( 'example.com' ); + } ); - /** - * Test: Can add submenu item using the keyboard - */ - navigation.useToolbarButton( 'Add submenu' ); + await test.step( 'should open and close the preview with the keyboard and escape buttons from a top-level nav item with a url-like label using the toolbar button', async () => { + await navigation.canUseToolbarLink(); + } ); - // Expect the submenu Add link to be present - await expect( - editor.canvas.locator( 'a' ).filter( { hasText: 'Add link' } ) - ).toBeVisible(); + await test.step( 'should be able to add submenu item using the keyboard', async () => { + navigation.useToolbarButton( 'Add submenu' ); - await pageUtils.pressKeys( 'ArrowDown' ); - // There is a bug that won't allow us to press Enter to add the link: https://github.com/WordPress/gutenberg/issues/60051 - // TODO: Use Enter after that bug is resolved - await navigation.useLinkShortcut(); + // Expect the submenu Add link to be present + await expect( + editor.canvas.locator( 'a' ).filter( { hasText: 'Add link' } ) + ).toBeVisible(); - await navigation.addPage( 'Dog' ); + await pageUtils.pressKeys( 'ArrowDown' ); + // There is a bug that won't allow us to press Enter to add the link: https://github.com/WordPress/gutenberg/issues/60051 + // TODO: Use Enter after that bug is resolved + await navigation.useLinkShortcut(); - /** - * Test: We can open and close the preview with the keyboard and escape - * buttons from a submenu nav item using both the shortcut and toolbar - */ - await navigation.useLinkShortcut(); - await navigation.previewIsOpenAndCloses(); - await navigation.checkLabelFocus( 'Dog' ); + await navigation.addPage( 'Dog' ); + } ); - await navigation.canUseToolbarLink(); + await test.step( 'should open and close the preview with the keyboard and escape buttons from a submenu nav item using the shortcut', async () => { + await navigation.useLinkShortcut(); + await navigation.previewIsOpenAndCloses(); + await navigation.checkLabelFocus( 'Dog' ); + } ); - // Return to nav label from toolbar - await page.keyboard.press( 'Escape' ); + await test.step( 'should open and close the preview with the keyboard and escape buttons from a submenu nav item using the toolbar button', async () => { + await navigation.canUseToolbarLink(); - // We should be at the first position on the label - await navigation.checkLabelFocus( 'Dog' ); + // Return to nav label from toolbar + await page.keyboard.press( 'Escape' ); - /** - * Test: We don't lose focus when closing the submenu appender - */ + // We should be at the first position on the label + await navigation.checkLabelFocus( 'Dog' ); + } ); - // Move focus to the submenu navigation appender - await page.keyboard.press( 'End' ); - await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); - await navigation.useBlockInserter(); - await navigation.addLinkClose(); - /** - * TODO: This is not desired behavior. Ideally the - * Appender should be focused again since it opened - * the link control. - * IMPORTANT: This check is not to enforce this behavior, - * but to make sure focus is kept nearby until we are able - * to send focus to the appender. It is falling back to the previous sibling. - */ - await navigation.checkLabelFocus( 'Dog' ); + await test.step( 'should not lose focus when closing the submenu appender', async () => { + // Move focus to the submenu navigation appender + await page.keyboard.press( 'End' ); + await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); + await navigation.useBlockInserter(); + await navigation.addLinkClose(); + /** + * TODO: This is not desired behavior. Ideally the + * Appender should be focused again since it opened + * the link control. + * IMPORTANT: This check is not to enforce this behavior, + * but to make sure focus is kept nearby until we are able + * to send focus to the appender. It is falling back to the previous sibling. + */ + await navigation.checkLabelFocus( 'Dog' ); + } ); - /** - * Test: Use the submenu nav item appender to add a custom link - */ - await page.keyboard.press( 'End' ); - await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); - await navigation.useBlockInserter(); - await navigation.addCustomURL( 'https://wordpress.org' ); - await navigation.expectToHaveTextSelected( 'wordpress.org' ); + await test.step( 'should be able to add a custom link to a submenu nav item using the nav item appender', async () => { + await page.keyboard.press( 'End' ); + await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); + await navigation.useBlockInserter(); + await navigation.addCustomURL( 'https://wordpress.org' ); + await navigation.expectToHaveTextSelected( 'wordpress.org' ); + } ); - /** - * Test: We can open and close the preview with the keyboard and escape - * both the shortcut and toolbar - */ - await pageUtils.pressKeys( 'ArrowLeft' ); - await navigation.useLinkShortcut(); - await navigation.previewIsOpenAndCloses(); - await navigation.checkLabelFocus( 'wordpress.org' ); - await navigation.canUseToolbarLink(); + await test.step( 'should close the preview with escape key from the shortcut', async () => { + await pageUtils.pressKeys( 'ArrowLeft' ); + await navigation.useLinkShortcut(); + await navigation.previewIsOpenAndCloses(); + await navigation.checkLabelFocus( 'wordpress.org' ); + } ); - /** - * Test: We can open and close the preview from a submenu navigation block (the top-level parent of a submenu) - * using both the shortcut and toolbar - */ - // Exit the toolbar - await page.keyboard.press( 'Escape' ); - // Move to the submenu item - await pageUtils.pressKeys( 'ArrowUp', { times: 4 } ); - await page.keyboard.press( 'Home' ); - - // Check we're on our submenu link - await navigation.checkLabelFocus( 'example.com' ); - // Test the shortcut - await navigation.useLinkShortcut(); - await navigation.previewIsOpenAndCloses(); - await navigation.checkLabelFocus( 'example.com' ); - // Test the toolbar - await navigation.canUseToolbarLink(); - await page.keyboard.press( 'Escape' ); - await navigation.checkLabelFocus( 'example.com' ); + await test.step( 'should close the preview with escape key from the toolbar button', async () => { + await navigation.canUseToolbarLink(); + } ); + await test.step( 'should open and close open and close the preview from a submenu navigation block (the top-level parent of a submenu) using the shortcut', async () => { + // Exit the toolbar + await page.keyboard.press( 'Escape' ); + // Move to the submenu item + await pageUtils.pressKeys( 'ArrowUp', { times: 4 } ); + await page.keyboard.press( 'Home' ); + + // Check we're on our submenu link + await navigation.checkLabelFocus( 'example.com' ); + // Test the shortcut + await navigation.useLinkShortcut(); + await navigation.previewIsOpenAndCloses(); + await navigation.checkLabelFocus( 'example.com' ); + } ); + + await test.step( 'should open and close open and close the preview from a submenu navigation block (the top-level parent of a submenu) using the toolbar button', async () => { + await navigation.canUseToolbarLink(); + await page.keyboard.press( 'Escape' ); + await navigation.checkLabelFocus( 'example.com' ); + } ); /** * Deleting returns items focus to its sibling */ - await pageUtils.pressKeys( 'ArrowDown', { times: 4 } ); - await navigation.checkLabelFocus( 'wordpress.org' ); - // Delete the nav link - await pageUtils.pressKeys( 'access+z' ); - // Focus moved to sibling - await navigation.checkLabelFocus( 'Dog' ); - // Add a link back so we can delete the first submenu link and see if focus returns to the parent submenu item - await page.keyboard.press( 'End' ); - await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); - await navigation.useBlockInserter(); - await navigation.addCustomURL( 'https://wordpress.org' ); - await navigation.expectToHaveTextSelected( 'wordpress.org' ); - - await pageUtils.pressKeys( 'ArrowUp', { times: 2 } ); - await navigation.checkLabelFocus( 'Dog' ); - // Delete the nav link - await pageUtils.pressKeys( 'access+z' ); - await pageUtils.pressKeys( 'ArrowDown' ); - // Focus moved to parent submenu item - await navigation.checkLabelFocus( 'example.com' ); - // Deleting this should move focus to the sibling item - await pageUtils.pressKeys( 'access+z' ); - await navigation.checkLabelFocus( 'Cat' ); - // Deleting with no more siblings should focus the navigation block again - await pageUtils.pressKeys( 'access+z' ); - await expect( navBlock ).toBeFocused(); - // Wait until the nav block inserter is visible before we continue. - await expect( navBlockInserter ).toBeVisible(); - // Now the appender should be visible and reachable with an arrow down - await pageUtils.pressKeys( 'ArrowDown' ); - await expect( navBlockInserter ).toBeFocused(); + await test.step( 'should return focus to sibling when deleting an item', async () => { + await pageUtils.pressKeys( 'ArrowDown', { times: 4 } ); + await navigation.checkLabelFocus( 'wordpress.org' ); + // Delete the nav link + await pageUtils.pressKeys( 'access+z' ); + // Focus moved to sibling + await navigation.checkLabelFocus( 'Dog' ); + // Add a link back so we can delete the first submenu link and see if focus returns to the parent submenu item + await page.keyboard.press( 'End' ); + await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); + await navigation.useBlockInserter(); + await navigation.addCustomURL( 'https://wordpress.org' ); + await navigation.expectToHaveTextSelected( 'wordpress.org' ); + + await pageUtils.pressKeys( 'ArrowUp', { times: 2 } ); + await navigation.checkLabelFocus( 'Dog' ); + // Delete the nav link + await pageUtils.pressKeys( 'access+z' ); + await pageUtils.pressKeys( 'ArrowDown' ); + // Focus moved to parent submenu item + await navigation.checkLabelFocus( 'example.com' ); + // Deleting this should move focus to the sibling item + await pageUtils.pressKeys( 'access+z' ); + await navigation.checkLabelFocus( 'Cat' ); + // Deleting with no more siblings should focus the navigation block again + await pageUtils.pressKeys( 'access+z' ); + await expect( navBlock ).toBeFocused(); + // Wait until the nav block inserter is visible before we continue. + await expect( navBlockInserter ).toBeVisible(); + // Now the appender should be visible and reachable with an arrow down + await pageUtils.pressKeys( 'ArrowDown' ); + await expect( navBlockInserter ).toBeFocused(); + } ); } ); test( 'Adding new links to a navigation block with existing inner blocks triggers creation of a single Navigation Menu', async ( { From d328bad68f7778997f7eeea3237e87ebe7575b98 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Thu, 19 Dec 2024 08:21:31 -0600 Subject: [PATCH 06/14] Add back in functionality to remove empty links --- .../block-library/src/navigation-link/edit.js | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index c1a64a0f696497..4e19ebfb8ecb69 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -246,6 +246,7 @@ export default function NavigationLinkEdit( { replaceBlock, __unstableMarkNextChangeAsNotPersistent, selectBlock, + selectPreviousBlock, } = useDispatch( blockEditorStore ); // Have the link editing ui open on mount when lacking a url and selected. const [ isLinkOpen, setIsLinkOpen ] = useState( isSelected && ! url ); @@ -561,6 +562,32 @@ export default function NavigationLinkEdit( { clientId={ clientId } link={ attributes } onClose={ () => { + // If there is no link then remove the block. + // This avoids empty blocks which can provided a poor UX. + // This should not happen often. The main route is through adding a custom link block. + // An alternative to this would be to open the custom link block + // directly to the edit link popover with text and url fields, rather than the LinkUI + // where you can select a block as well. + if ( ! url ) { + // Fixes https://github.com/WordPress/gutenberg/issues/61361 + // There's a chance we're closing due to the user selecting the browse all button. + // Only move focus if the focus is still within the popover ui. If it's not within + // the popover, it's because something has taken the focus from the popover, and + // we don't want to steal it back. + if ( + linkUIref.current.contains( + window.document.activeElement + ) + ) { + // Select the previous block to keep focus nearby + selectPreviousBlock( clientId, true ); + } + + // Remove the link. + onReplace( [] ); + return; + } + setIsLinkOpen( false ); } } anchor={ popoverAnchor } From 6b8867b59e7d643f0546f693cd9429d273b7253a Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Thu, 19 Dec 2024 11:35:09 -0600 Subject: [PATCH 07/14] Pass navigation block id as the root client Id so the right blocks show in the quick inserter --- .../src/navigation-link/link-ui.js | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/navigation-link/link-ui.js b/packages/block-library/src/navigation-link/link-ui.js index a57097e39966ed..4127c4322da88e 100644 --- a/packages/block-library/src/navigation-link/link-ui.js +++ b/packages/block-library/src/navigation-link/link-ui.js @@ -12,6 +12,7 @@ import { __, sprintf, isRTL } from '@wordpress/i18n'; import { LinkControl, privateApis as blockEditorPrivateApis, + store as blockEditorStore, } from '@wordpress/block-editor'; import { createInterpolateElement, @@ -26,7 +27,7 @@ import { useResourcePermissions, } from '@wordpress/core-data'; import { decodeEntities } from '@wordpress/html-entities'; -import { useDispatch } from '@wordpress/data'; +import { useDispatch, useSelect } from '@wordpress/data'; import { chevronLeftSmall, chevronRightSmall, plus } from '@wordpress/icons'; import { useInstanceId, useFocusOnMount } from '@wordpress/compose'; @@ -78,6 +79,25 @@ export function getSuggestionsQuery( type, kind ) { } function LinkUIBlockInserter( { clientId, onBack, onSelectBlock } ) { + const { navigationBlockId } = useSelect( + ( select ) => { + const { getBlockRootClientId, getBlockName } = + select( blockEditorStore ); + + const rootClientId = getBlockRootClientId( clientId ); + + // We need the core/navigation block to be the ID that gets passed to the as the rootClientId + // so the right block results show up in the quick inserter. + return { + navigationBlockId: + getBlockName( rootClientId ) === 'core/navigation' + ? rootClientId + : clientId, + }; + }, + [ clientId ] + ); + const focusOnMountRef = useFocusOnMount( 'firstElement' ); const dialogTitleId = useInstanceId( @@ -122,7 +142,8 @@ function LinkUIBlockInserter( { clientId, onBack, onSelectBlock } ) { Date: Thu, 19 Dec 2024 13:06:55 -0600 Subject: [PATCH 08/14] Fix appender insertion order via quick inserter --- .../src/navigation-link/link-ui.js | 27 +++---------------- 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/packages/block-library/src/navigation-link/link-ui.js b/packages/block-library/src/navigation-link/link-ui.js index 4127c4322da88e..b453d52a427c58 100644 --- a/packages/block-library/src/navigation-link/link-ui.js +++ b/packages/block-library/src/navigation-link/link-ui.js @@ -12,7 +12,6 @@ import { __, sprintf, isRTL } from '@wordpress/i18n'; import { LinkControl, privateApis as blockEditorPrivateApis, - store as blockEditorStore, } from '@wordpress/block-editor'; import { createInterpolateElement, @@ -27,7 +26,7 @@ import { useResourcePermissions, } from '@wordpress/core-data'; import { decodeEntities } from '@wordpress/html-entities'; -import { useDispatch, useSelect } from '@wordpress/data'; +import { useDispatch } from '@wordpress/data'; import { chevronLeftSmall, chevronRightSmall, plus } from '@wordpress/icons'; import { useInstanceId, useFocusOnMount } from '@wordpress/compose'; @@ -79,25 +78,6 @@ export function getSuggestionsQuery( type, kind ) { } function LinkUIBlockInserter( { clientId, onBack, onSelectBlock } ) { - const { navigationBlockId } = useSelect( - ( select ) => { - const { getBlockRootClientId, getBlockName } = - select( blockEditorStore ); - - const rootClientId = getBlockRootClientId( clientId ); - - // We need the core/navigation block to be the ID that gets passed to the as the rootClientId - // so the right block results show up in the quick inserter. - return { - navigationBlockId: - getBlockName( rootClientId ) === 'core/navigation' - ? rootClientId - : clientId, - }; - }, - [ clientId ] - ); - const focusOnMountRef = useFocusOnMount( 'firstElement' ); const dialogTitleId = useInstanceId( @@ -142,9 +122,8 @@ function LinkUIBlockInserter( { clientId, onBack, onSelectBlock } ) { Date: Thu, 19 Dec 2024 13:38:16 -0600 Subject: [PATCH 09/14] Update tests for closing link overlay to go straight to appender --- .../specs/editor/blocks/navigation.spec.js | 24 ++----------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/test/e2e/specs/editor/blocks/navigation.spec.js b/test/e2e/specs/editor/blocks/navigation.spec.js index f7217ef09606b6..409950556798bb 100644 --- a/test/e2e/specs/editor/blocks/navigation.spec.js +++ b/test/e2e/specs/editor/blocks/navigation.spec.js @@ -342,20 +342,10 @@ test.describe( 'Navigation block', () => { await pageUtils.pressKeys( 'ArrowDown' ); await navigation.useBlockInserter(); await navigation.addLinkClose(); - /** - * TODO: This is not desired behavior. Ideally the - * Appender should be focused again since it opened - * the link control. - * IMPORTANT: This check is not to enforce this behavior, - * but to make sure focus is kept nearby until we are able - * to send focus to the appender. - */ - await expect( navBlock ).toBeFocused(); + await expect( navigation.getNavBlockInserter() ).toBeFocused(); } ); await test.step( 'should send focus to the newly created navigation link item when creating a link', async () => { - await pageUtils.pressKeys( 'ArrowDown' ); - await navigation.useBlockInserter(); await navigation.addPage( 'Cat' ); } ); @@ -429,20 +419,10 @@ test.describe( 'Navigation block', () => { await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); await navigation.useBlockInserter(); await navigation.addLinkClose(); - /** - * TODO: This is not desired behavior. Ideally the - * Appender should be focused again since it opened - * the link control. - * IMPORTANT: This check is not to enforce this behavior, - * but to make sure focus is kept nearby until we are able - * to send focus to the appender. It is falling back to the previous sibling. - */ - await navigation.checkLabelFocus( 'Dog' ); + await expect( navigation.getNavBlockInserter() ).toBeFocused(); } ); await test.step( 'should be able to add a custom link to a submenu nav item using the nav item appender', async () => { - await page.keyboard.press( 'End' ); - await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); await navigation.useBlockInserter(); await navigation.addCustomURL( 'https://wordpress.org' ); await navigation.expectToHaveTextSelected( 'wordpress.org' ); From 01d0e68899b2aa4dfcfee6b15304704cdf92fbc8 Mon Sep 17 00:00:00 2001 From: Jeremy Jones Date: Fri, 20 Dec 2024 09:06:39 -0600 Subject: [PATCH 10/14] Close link ui after insertion --- packages/block-library/src/navigation-link/appender.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/block-library/src/navigation-link/appender.js b/packages/block-library/src/navigation-link/appender.js index db03eaa1f51f61..634d3602b4208c 100644 --- a/packages/block-library/src/navigation-link/appender.js +++ b/packages/block-library/src/navigation-link/appender.js @@ -56,6 +56,7 @@ export default function NavigationLinkAppender( { rootClientId } ) { setIsLinkOpen( false ); } } onChange={ ( updatedValue ) => { + setIsLinkOpen( false ); updateAttributes( updatedValue, createNewLink ); } } anchor={ popoverAnchor } From 7ad1633fe596fb1bd744ba39f41fbe994ce02b7b Mon Sep 17 00:00:00 2001 From: Jeremy Jones Date: Fri, 20 Dec 2024 09:34:37 -0600 Subject: [PATCH 11/14] Open link ui when added --- packages/block-library/src/navigation-link/edit.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index 4e19ebfb8ecb69..a78df33c1a75b2 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -248,8 +248,8 @@ export default function NavigationLinkEdit( { selectBlock, selectPreviousBlock, } = useDispatch( blockEditorStore ); - // Have the link editing ui open on mount when lacking a url and selected. - const [ isLinkOpen, setIsLinkOpen ] = useState( isSelected && ! url ); + // Have the link editing ui open on mount if the block was just added. + const [ isLinkOpen, setIsLinkOpen ] = useState( isSelected ); // Store what element opened the popover, so we know where to return focus to (toolbar button vs navigation link text) // Use internal state instead of a ref to make sure that the component // re-renders when the popover's anchor updates. @@ -298,6 +298,7 @@ export default function NavigationLinkEdit( { }, [ clientId, maxNestingLevel ] ); + const { getBlocks } = useSelect( blockEditorStore ); /** From 9bd8fd3b07ec8e3b5ff1509213ea3383730826e5 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Fri, 20 Dec 2024 12:05:19 -0600 Subject: [PATCH 12/14] Fix duplicate locator issue in test --- test/e2e/specs/editor/blocks/search.spec.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/e2e/specs/editor/blocks/search.spec.js b/test/e2e/specs/editor/blocks/search.spec.js index 3a72f99b7cf0b3..e14b07dae3cad5 100644 --- a/test/e2e/specs/editor/blocks/search.spec.js +++ b/test/e2e/specs/editor/blocks/search.spec.js @@ -44,7 +44,10 @@ test.describe( 'Search', () => { } ); await navBlockInserter.click(); - await page.getByRole( 'button', { name: 'Add block' } ).click(); + // Move to and activate the "Add Block" button. + await page.keyboard.press( 'Tab' ); + await page.keyboard.press( 'Tab' ); + await page.keyboard.press( 'Enter' ); // Click on the Search block option. await page.getByRole( 'option', { name: 'Search' } ).click(); From 9e0ec8385ce9b9b2b8724a9ee3e6ebccd2e08b9c Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Fri, 20 Dec 2024 12:33:51 -0600 Subject: [PATCH 13/14] Focus link on popover close when newly added --- .../block-library/src/navigation-link/edit.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index a78df33c1a75b2..e858d5bd952481 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -250,6 +250,9 @@ export default function NavigationLinkEdit( { } = useDispatch( blockEditorStore ); // Have the link editing ui open on mount if the block was just added. const [ isLinkOpen, setIsLinkOpen ] = useState( isSelected ); + // If the link ui was opened because it was just added, we need to return focus to the block instead of + // the appender when it is closed. + const focusBlockOnLinkCloseRef = useRef( isLinkOpen ); // Store what element opened the popover, so we know where to return focus to (toolbar button vs navigation link text) // Use internal state instead of a ref to make sure that the component // re-renders when the popover's anchor updates. @@ -589,7 +592,19 @@ export default function NavigationLinkEdit( { return; } + // If the link ui was opened due to the block being added, + // we need to return focus to the block instead of the appender. + if ( + focusBlockOnLinkCloseRef.current && + linkUIref.current.contains( + window.document.activeElement + ) + ) { + ref.current.focus(); + } + setIsLinkOpen( false ); + focusBlockOnLinkCloseRef.current = false; } } anchor={ popoverAnchor } onRemove={ removeLink } From e47dec26df8dbcde737b547f4821fec06d051890 Mon Sep 17 00:00:00 2001 From: Jerry Jones Date: Fri, 20 Dec 2024 13:14:48 -0600 Subject: [PATCH 14/14] Do not open popover if just added and url-like label --- .../block-library/src/navigation-link/edit.js | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index e858d5bd952481..ba5a731228bf58 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -33,7 +33,7 @@ import { __unstableStripHTML as stripHTML } from '@wordpress/dom'; import { decodeEntities } from '@wordpress/html-entities'; import { link as linkIcon, addSubmenu } from '@wordpress/icons'; import { store as coreStore } from '@wordpress/core-data'; -import { useMergeRefs, usePrevious } from '@wordpress/compose'; +import { useMergeRefs } from '@wordpress/compose'; /** * Internal dependencies @@ -248,8 +248,16 @@ export default function NavigationLinkEdit( { selectBlock, selectPreviousBlock, } = useDispatch( blockEditorStore ); - // Have the link editing ui open on mount if the block was just added. - const [ isLinkOpen, setIsLinkOpen ] = useState( isSelected ); + + // If the link isSelected when mounted, it was just added via an appender + const isNewLink = isSelected; + const isLabelURLLike = + url && isURL( prependHTTP( label ) ) && /^.+\.[a-z]+/.test( label ); + + const [ isLinkOpen, setIsLinkOpen ] = useState( + isNewLink && ! isLabelURLLike + ); + // If the link ui was opened because it was just added, we need to return focus to the block instead of // the appender when it is closed. const focusBlockOnLinkCloseRef = useRef( isLinkOpen ); @@ -262,7 +270,14 @@ export default function NavigationLinkEdit( { const itemLabelPlaceholder = __( 'Add label…' ); const ref = useRef(); const linkUIref = useRef(); - const prevUrl = usePrevious( url ); + + useEffect( () => { + // If the link was just added and label is url-like, focus and select the label text. + if ( isNewLink && isLabelURLLike ) { + // Focus and select the label text. + selectLabelText(); + } + }, [] ); // Change the label using inspector causes rich text to change focus on firefox. // This is a workaround to keep the focus on the label field when label filed is focused we don't render the rich text. @@ -331,21 +346,6 @@ export default function NavigationLinkEdit( { } }, [ hasChildren ] ); - // If the LinkControl popover is open and the URL has changed, close the LinkControl and focus the label text. - useEffect( () => { - // We only want to do this when the URL has gone from nothing to a new URL AND the label looks like a URL - if ( - ! prevUrl && - url && - isLinkOpen && - isURL( prependHTTP( label ) ) && - /^.+\.[a-z]+/.test( label ) - ) { - // Focus and select the label text. - selectLabelText(); - } - }, [ prevUrl, url, isLinkOpen, label ] ); - /** * Focus the Link label text and select it. */ @@ -600,7 +600,7 @@ export default function NavigationLinkEdit( { window.document.activeElement ) ) { - ref.current.focus(); + ref.current?.focus(); } setIsLinkOpen( false );