diff --git a/packages/block-editor/src/components/link-control/index.js b/packages/block-editor/src/components/link-control/index.js index 980447166cc3e2..6442d2569384dd 100644 --- a/packages/block-editor/src/components/link-control/index.js +++ b/packages/block-editor/src/components/link-control/index.js @@ -15,7 +15,7 @@ import { __experimentalInputControlSuffixWrapper as InputControlSuffixWrapper, } from '@wordpress/components'; import { __, sprintf } from '@wordpress/i18n'; -import { useRef, useState, useEffect } from '@wordpress/element'; +import { useRef, useState, useEffect, useMemo } from '@wordpress/element'; import { useInstanceId } from '@wordpress/compose'; import { focus } from '@wordpress/dom'; import { ENTER } from '@wordpress/keycodes'; @@ -187,7 +187,15 @@ function LinkControl( { const wrapperNode = useRef(); const textInputRef = useRef(); const searchInputRef = useRef(); - const isEndingEditWithFocusRef = useRef( false ); + // TODO: Remove entityUrlFallbackRef and previewValue in favor of value prop after taxonomy entity binding + // is stable and returns the correct URL instead of null while resolving when creating the entity. + // + // Preserve the URL from entity suggestions before binding overrides it + // This is due to entity binding not being available immediately after the suggestion is selected. + // The URL can return null, especially for taxonomy entities, while entity binding is being resolved. + // To avoid unnecessary rerenders and focus loss, we preserve the URL from the suggestion and use it + // as a fallback until the entity binding is available. + const entityUrlFallbackRef = useRef(); const settingsKeys = settings.map( ( { id } ) => id ); @@ -244,8 +252,6 @@ function LinkControl( { wrapperNode.current; nextFocusTarget.focus(); - - isEndingEditWithFocusRef.current = false; }, [ isEditingLink, isCreatingPage ] ); // The component mounting reference is maintained separately @@ -261,18 +267,18 @@ function LinkControl( { const hasLinkValue = value?.url?.trim()?.length > 0; /** - * Cancels editing state and marks that focus may need to be restored after - * the next render, if focus was within the wrapper when editing finished. + * Cancels editing state. */ const stopEditing = () => { - isEndingEditWithFocusRef.current = !! wrapperNode.current?.contains( - wrapperNode.current.ownerDocument.activeElement - ); - setIsEditingLink( false ); }; const handleSelectSuggestion = ( updatedValue ) => { + // Preserve the URL for taxonomy entities before binding overrides it + if ( updatedValue?.kind === 'taxonomy' && updatedValue?.url ) { + entityUrlFallbackRef.current = updatedValue.url; + } + // Suggestions may contains "settings" values (e.g. `opensInNewTab`) // which should not override any existing settings values set by the // user. This filters out any settings values from the suggestion. @@ -396,6 +402,24 @@ function LinkControl( { const isDisabled = ! valueHasChanges || currentInputIsEmpty; const showSettings = !! settings?.length && isEditingLink && hasLinkValue; + const previewValue = useMemo( () => { + // There is a chance that the value is not yet set from the entity binding, so we use the preserved URL. + if ( + value?.kind === 'taxonomy' && + ! value?.url && + entityUrlFallbackRef.current + ) { + // combine the value prop with the preserved URL from the suggestion + return { + ...value, + url: entityUrlFallbackRef.current, + }; + } + + // If we don't have a fallback URL, use the value prop. + return value; + }, [ value ] ); + return (
setIsEditingLink( true ) } hasRichPreviews={ hasRichPreviews } hasUnlinkControl={ shownUnlinkControl } diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index 6efd869daa0cf7..f2f2985b35a171 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -30,7 +30,7 @@ import { useState, useEffect, useRef, useCallback } from '@wordpress/element'; 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, useInstanceId } from '@wordpress/compose'; +import { useMergeRefs, useInstanceId } from '@wordpress/compose'; /** * Internal dependencies @@ -213,8 +213,11 @@ export default function NavigationLinkEdit( { const itemLabelPlaceholder = __( 'Add label…' ); const ref = useRef(); const linkUIref = useRef(); - const prevUrl = usePrevious( url ); - const isNewLink = useRef( ! url && ! metadata?.bindings?.url ); + // A link is "new" only if it has an undefined label + // After the link is created, even if no label is provided, it's set to an empty string. + const isNewLink = useRef( label === undefined ); + // Track whether we should focus the submenu appender when closing the link UI + const shouldSelectSubmenuAppenderOnClose = useRef( false ); const { isAtMaxNesting, @@ -223,6 +226,7 @@ export default function NavigationLinkEdit( { hasChildren, validateLinkStatus, parentBlockClientId, + isSubmenu, } = useSelect( ( select ) => { const { @@ -267,6 +271,7 @@ export default function NavigationLinkEdit( { hasChildren: !! getBlockCount( clientId ), validateLinkStatus: enableLinkStatusValidation, parentBlockClientId: parentBlockId, + isSubmenu: parentBlockName === 'core/navigation-submenu', }; }, [ clientId, maxNestingLevel ] @@ -314,7 +319,7 @@ export default function NavigationLinkEdit( { // If we leave focus on this block, then when we close the link without creating a link, focus will // be lost during the new block selection process. useEffect( () => { - if ( isNewLink.current && isSelected && ! url ) { + if ( isNewLink.current && isSelected ) { selectBlock( parentBlockClientId ); } }, [] ); // eslint-disable-line react-hooks/exhaustive-deps @@ -333,20 +338,46 @@ export default function NavigationLinkEdit( { transformToSubmenu, ] ); - // If the LinkControl popover is open and the URL has changed, close the LinkControl and focus the label text. + // Handle link UI when a new link is created 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 ) - ) { + // We know if a link was just created from our link UI if + // 1. isNewLink.current is true + // 2. url has a value + // 3. isLinkOpen is true + if ( ! isNewLink.current || ! url || ! isLinkOpen ) { + return; + } + + // Ensure this only runs once + isNewLink.current = false; + + // We just created a link and the block is now selected. + // If the label looks like a URL, focus and select the label text. + if ( isURL( prependHTTP( label ) ) && /^.+\.[a-z]+/.test( label ) ) { // Focus and select the label text. selectLabelText(); + } else { + // If the link was just created, we want to select the block so the inspector controls + // are accurate. + selectBlock( clientId, null ); + + // Edge case: When the created link is the first child of a submenu, the focus will have + // originated from the add submenu toolbar button. In this case, we need to return focus + // to the submenu appender if the user closes the link ui using the keyboard. + // Check if this is the first and only child of a newly created submenu. + if ( isSubmenu ) { + const parentBlocks = getBlocks( parentBlockClientId ); + // If this is the only child, then this is a new submenu. + // Set the flag to select the submenu appender when the link ui is closed. + if ( + parentBlocks.length === 1 && + parentBlocks[ 0 ].clientId === clientId + ) { + shouldSelectSubmenuAppenderOnClose.current = true; + } + } } - }, [ prevUrl, url, isLinkOpen, label ] ); + }, [ url, isLinkOpen, isNewLink, label ] ); /** * Focus the Link label text and select it. @@ -444,23 +475,20 @@ export default function NavigationLinkEdit( { } ); - if ( - ! url || + const needsValidLink = + ( ! url && ! ( hasUrlBinding && isBoundEntityAvailable ) ) || isInvalid || isDraft || - ( hasUrlBinding && ! isBoundEntityAvailable ) - ) { + ( hasUrlBinding && ! isBoundEntityAvailable ); + + if ( needsValidLink ) { blockProps.onClick = () => { setIsLinkOpen( true ); }; } const classes = clsx( 'wp-block-navigation-item__content', { - 'wp-block-navigation-link__placeholder': - ! url || - isInvalid || - isDraft || - ( hasUrlBinding && ! isBoundEntityAvailable ), + 'wp-block-navigation-link__placeholder': needsValidLink, } ); const missingText = getMissingText( type ); @@ -589,9 +617,27 @@ export default function NavigationLinkEdit( { // Don't remove if binding exists (even if entity is unavailable) so user can fix it. if ( ! url && ! hasUrlBinding ) { onReplace( [] ); - } else if ( isNewLink.current ) { - // If we just created a new link, select it - selectBlock( clientId ); + return; + } + + // Edge case: If this is the first child of a new submenu, focus the submenu's appender + if ( + shouldSelectSubmenuAppenderOnClose.current + ) { + shouldSelectSubmenuAppenderOnClose.current = false; + + // The appender is the next sibling in the DOM after the current block + if ( + listItemRef.current?.nextElementSibling + ) { + const appenderButton = + listItemRef.current.nextElementSibling.querySelector( + '.block-editor-button-block-appender' + ); + if ( appenderButton ) { + appenderButton.focus(); + } + } } } } anchor={ popoverAnchor } diff --git a/test/e2e/specs/editor/blocks/navigation.spec.js b/test/e2e/specs/editor/blocks/navigation.spec.js index 6489f5b37658ce..5b9b301cf0c79d 100644 --- a/test/e2e/specs/editor/blocks/navigation.spec.js +++ b/test/e2e/specs/editor/blocks/navigation.spec.js @@ -296,13 +296,15 @@ test.describe( 'Navigation block', () => { } ); test.describe( 'Focus management', () => { + let catPage, dogPage; + test.beforeAll( async ( { requestUtils } ) => { // We need pages to be published so the Link Control can return pages - await requestUtils.createPage( { + catPage = await requestUtils.createPage( { title: 'Cat', status: 'publish', } ); - await requestUtils.createPage( { + dogPage = await requestUtils.createPage( { title: 'Dog', status: 'publish', } ); @@ -353,6 +355,7 @@ test.describe( 'Navigation block', () => { await pageUtils.pressKeys( 'ArrowDown' ); await navigation.useBlockInserter(); await navigation.addPage( 'Cat' ); + await pageUtils.pressKeys( 'ArrowLeft', { times: 2 } ); } ); await test.step( 'can use the shortcut to open the preview with the keyboard and escape keypress sends focus back to the navigation link block', async () => { @@ -381,6 +384,8 @@ test.describe( 'Navigation block', () => { await navigation.useBlockInserter(); await navigation.addCustomURL( 'https://example.com' ); await navigation.expectToHaveTextSelected( 'example.com' ); + // The link UI should be closed when creating a custom link + await expect( navigation.getLinkPopover() ).toBeHidden(); } ); await test.step( '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', async () => { @@ -420,7 +425,9 @@ test.describe( 'Navigation block', () => { // TODO: Use Enter after that bug is resolved await navigation.useLinkShortcut(); - await navigation.addPage( 'Dog' ); + await navigation.addSubmenuPage( 'Dog' ); + await pageUtils.pressKeys( 'ArrowUp' ); + await pageUtils.pressKeys( 'ArrowRight' ); } ); await test.step( 'can use the shortcut to open the preview with the keyboard and escape keypress sends focus back to the navigation link block in the submenu', async () => { @@ -533,8 +540,6 @@ test.describe( 'Navigation block', () => { await pageUtils.pressKeys( 'ArrowDown' ); await navigation.useBlockInserter(); await navigation.addPage( 'Cat' ); - await pageUtils.pressKeys( 'ArrowDown' ); - await pageUtils.pressKeys( 'ArrowRight', { times: 2 } ); await navigation.useBlockInserter(); await navigation.addCustomURL( 'https://example.com' ); await navigation.expectToHaveTextSelected( 'example.com' ); @@ -550,8 +555,6 @@ test.describe( 'Navigation block', () => { // TODO: Use Enter after that bug is resolved await navigation.useLinkShortcut(); await navigation.addPage( 'Dog' ); - 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' ); @@ -683,6 +686,193 @@ test.describe( 'Navigation block', () => { expect( focusIsInSidebar ).toBe( true ); } ); + + test( 'Selecting a new block from another link with a popover open should respect the new block selection', async ( { + editor, + page, + pageUtils, + navigation, + requestUtils, + } ) => { + let inspectorNavigationLabel, + catLinkText, + dogLinkText, + linkPopover, + unavailableLinkText; + + // Test setup step + await test.step( 'Test setup', async () => { + const nonExistentPageId = 99999; + // Create a menu with three links: + // 1. Invalid synced link (deleted page) + // 2. Valid synced link (Cat page) + // 3. Valid synced link (Dog page) + // 4. Custom URL link (example.com) + const menu = await requestUtils.createNavigationMenu( { + title: 'Test Menu with Unavailable Entity, Synced Cat Page, and Custom URL', + content: ` + + +`, + } ); + + await editor.insertBlock( { + name: 'core/navigation', + attributes: { + ref: menu.id, + }, + } ); + + // Open the insepctor sidebar, as this is the easiest way to visually see block selection + await editor.openDocumentSettingsSidebar(); + + // CRITICAL: Wait for synced link entities to load BEFORE interacting with them + // Synced links load their URLs asynchronously. If we click them before the URLs + // are loaded, the popover opens in edit mode (url: null) instead of preview mode. + // Select the Cat link temporarily to check if its URL is loaded in the sidebar. + catLinkText = editor.canvas + .getByRole( 'textbox', { + name: 'Navigation link text', + } ) + .filter( { hasText: /^Cat$/ } ); + await catLinkText.click(); + + const linkInput = page + .getByRole( 'tabpanel', { name: 'Settings' } ) + .getByRole( 'textbox', { + name: 'Link', + } ); + // Wait for the Cat link's URL to load + await expect( linkInput ).not.toHaveValue( '' ); + await expect( linkInput ).toBeDisabled(); // Synced links have disabled Link field + } ); + + await test.step( 'Popover closing from unsynced link to a synced link should not steal focus back to the previously selected (Cat) link', async () => { + // Cat link is already selected from setup step, with entity loaded + // Verify sidebar shows Cat + inspectorNavigationLabel = page + .getByRole( 'tabpanel', { name: 'Settings' } ) + .getByRole( 'textbox', { + name: 'Text', + } ); + await expect( inspectorNavigationLabel ).toHaveValue( 'Cat' ); + + await pageUtils.pressKeys( 'primary+k' ); + linkPopover = navigation.getLinkPopover(); + await expect( linkPopover ).toBeVisible(); + const catPopoverLink = navigation.getLinkControlLink( 'Cat' ); + await expect( catPopoverLink ).toBeVisible(); + + // Check that the popover has focus on the Cat link + await expect( catPopoverLink ).toBeFocused(); + + dogLinkText = editor.canvas + .getByRole( 'textbox', { + name: 'Navigation link text', + } ) + .filter( { hasText: 'Dog' } ); + await dogLinkText.click(); + + // Verify the popover is closed + await expect( linkPopover ).toBeHidden(); + // Check that the Label in the inspector sidebar is Dog + await expect( inspectorNavigationLabel ).toHaveValue( 'Dog' ); + } ); + + await test.step( 'Popover closing from synced (Dog) link to an unsynced link should not steal focus back to the previously selected (Dog) link', async () => { + await pageUtils.pressKeys( 'primary+k' ); + await expect( linkPopover ).toBeVisible(); + + const dogPopoverLink = navigation.getLinkControlLink( 'Dog' ); + await expect( dogPopoverLink ).toBeVisible(); + + // Check that the popover has focus on the Cat link + await expect( dogPopoverLink ).toBeFocused(); + + unavailableLinkText = editor.canvas + .locator( 'a' ) + .filter( { hasText: 'Unavailable Page (Invalid)' } ); + await unavailableLinkText.click(); + + // Check that the Label in the inspector sidebar is Unavailable Page + await expect( inspectorNavigationLabel ).toHaveValue( + 'Unavailable Page' + ); + } ); + + await test.step( 'Selecting a new block from a invalid synced link with a popover open should respect the new block selection', async () => { + // Verify the popover is visible (we want the invalid link click to have opened the popover) + await expect( linkPopover ).toBeVisible(); + await expect( + linkPopover.getByRole( 'combobox', { + name: 'Search or type URL', + } ) + ).toBeFocused(); + // Check that the popover has focus in the editable link state + + await catLinkText.click(); + + // Verify the popover is closed + await expect( linkPopover ).toBeHidden(); + // Check that the Label in the inspector sidebar is Cat + await expect( inspectorNavigationLabel ).toHaveValue( 'Cat' ); + } ); + + await test.step( 'Creating a new category link should respect new block selection', async () => { + // Use the block inserter to add a new category link + await editor.canvas + .getByRole( 'button', { name: 'Add block' } ) + .click(); + + // Verify the popover is visible (we want the invalid link click to have opened the popover) + await expect( linkPopover ).toBeVisible(); + await expect( + linkPopover.getByRole( 'combobox', { + name: 'Search or type URL', + } ) + ).toBeFocused(); + + await linkPopover + .getByRole( 'button', { name: 'Add block' } ) + .click(); + + const addBlockDialog = page.getByRole( 'dialog', { + name: 'Add block', + } ); + + await expect( addBlockDialog ).toBeVisible(); + + await expect( + addBlockDialog.getByRole( 'button', { name: 'Back' } ) + ).toBeFocused(); + + await addBlockDialog + .getByRole( 'option', { name: 'Custom Link' } ) + .click(); + + await navigation.useLinkControlSearch( 'Uncategorized' ); + + // expect the sidebar to show 'Uncategorized' as the label + await expect( inspectorNavigationLabel ).toHaveValue( + 'Uncategorized' + ); + + await expect( + navigation.getLinkControlLink( 'Uncategorized' ) + ).toBeVisible(); + + await expect( + navigation.getLinkControlLink( 'Uncategorized' ) + ).toBeFocused(); + + await catLinkText.click(); + + // Verify the popover is closed + await expect( linkPopover ).toBeHidden(); + // Check that the Label in the inspector sidebar is Cat + await expect( inspectorNavigationLabel ).toHaveValue( 'Cat' ); + } ); + } ); } ); test( 'Adding new links to a navigation block with existing inner blocks triggers creation of a single Navigation Menu', async ( { @@ -848,6 +1038,7 @@ test.describe( 'Navigation block', () => { await pageUtils.pressKeys( 'ArrowDown' ); await navigation.useBlockInserter(); await navigation.addPage( 'Test Page 1' ); + await pageUtils.pressKeys( 'ArrowLeft', { times: 2 } ); } ); await test.step( 'Verify bound link displays correctly in Link UI popover', async () => { @@ -1452,6 +1643,12 @@ class Navigation { return this.getNavBlock().getByLabel( 'Add block' ).first(); } + getSubmenuBlockInserter() { + return this.editor.canvas + .getByRole( 'document', { name: 'Block: Submenu' } ) + .getByLabel( 'Add block' ); + } + getLinkControlSearch() { return this.page.getByRole( 'combobox', { name: 'Search or type URL', @@ -1503,12 +1700,35 @@ class Navigation { * Usage: * - Open the new link control however you'd like (block appender, command+k on Add link label...) * - * @param {string} label Text of page you want added. Must be a part of the pages added in the beforeAll in this test suite. + * @param {string} label Text of page you want added. Must be a part of the pages added in the beforeAll in this test suite. + * @param {boolean} submenu Whether the page is being added to a submenu. */ - async addPage( label ) { - const linkControlSearch = this.page.getByRole( 'combobox', { - name: 'Search or type URL', - } ); + async addPage( label, submenu = false ) { + await this.useLinkControlSearch( label ); + + const linkControlLink = await this.getLinkControlLink( label ); + + await expect( linkControlLink ).toBeVisible(); + await expect( linkControlLink ).toBeFocused(); + + await this.page.keyboard.press( 'Escape' ); + await expect( this.getLinkControlSearch() ).toBeHidden(); + + // Check appender has focus + if ( submenu ) { + // chec for the submenu appender + await expect( this.getSubmenuBlockInserter() ).toBeFocused(); + } else { + await expect( this.getNavBlockInserter() ).toBeFocused(); + } + } + + async addSubmenuPage( label ) { + await this.addPage( label, true ); + } + + async useLinkControlSearch( label ) { + const linkControlSearch = this.getLinkControlSearch(); await expect( linkControlSearch ).toBeFocused(); @@ -1524,15 +1744,6 @@ class Navigation { await this.pageUtils.pressKeys( 'ArrowDown' ); await this.page.keyboard.press( 'Enter' ); - - const linkControlLink = await this.getLinkControlLink( label ); - await expect( linkControlLink ).toBeFocused(); - - await this.page.keyboard.press( 'Escape' ); - - await expect( linkControlSearch ).toBeHidden(); - - await this.checkLabelFocus( label ); } /**