Skip to content

Commit

Permalink
Fix navigation menu focus loss when switching menus
Browse files Browse the repository at this point in the history 
Remove navigation selector ref

Update e2e test

Switch back to useCallback

Fix test
  • Loading branch information
@talldan
talldan committed Aug 23, 2022
1 parent 5e2876c commit 56462ff
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 116 deletions.
107 changes: 28 additions & 79 deletions packages/block-library/src/navigation/edit/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ function Navigation( {
isNavigationMenuResolved,
isNavigationMenuMissing,
navigationMenus,
navigationMenu,
canUserUpdateNavigationMenu,
hasResolvedCanUserUpdateNavigationMenu,
canUserDeleteNavigationMenu,
Expand Down Expand Up @@ -240,8 +239,6 @@ function Navigation( {

const navRef = useRef();

const isDraftNavigationMenu = navigationMenu?.status === 'draft';

const {
convert: convertClassicMenu,
status: classicMenuConversionStatus,
Expand Down Expand Up @@ -331,11 +328,6 @@ function Navigation( {
] = useState();
const [ detectedOverlayColor, setDetectedOverlayColor ] = useState();

const handleUpdateMenu = ( menuId ) => {
setRef( menuId );
selectBlock( clientId );
};

useEffect( () => {
hideClassicMenuConversionNotice();
if ( classicMenuConversionStatus === CLASSIC_MENU_CONVERSION_PENDING ) {
Expand Down Expand Up @@ -426,27 +418,6 @@ function Navigation( {
ref,
] );

const navigationSelectorRef = useRef();
const [ shouldFocusNavigationSelector, setShouldFocusNavigationSelector ] =
useState( false );

// Focus support after menu selection.
useEffect( () => {
if (
isDraftNavigationMenu ||
! isEntityAvailable ||
! shouldFocusNavigationSelector
) {
return;
}
navigationSelectorRef?.current?.focus();
setShouldFocusNavigationSelector( false );
}, [
isDraftNavigationMenu,
isEntityAvailable,
shouldFocusNavigationSelector,
] );

const isResponsive = 'never' !== overlayMenu;

const overlayMenuPreviewClasses = classnames(
Expand Down Expand Up @@ -598,21 +569,16 @@ function Navigation( {
<BlockControls>
<ToolbarGroup className="wp-block-navigation__toolbar-menu-selector">
<NavigationMenuSelector
ref={ null }
currentMenuId={ null }
currentMenuId={ ref }
clientId={ clientId }
onSelectNavigationMenu={ ( menuId ) => {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectNavigationMenu={ setRef }
onSelectClassicMenu={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
setShouldFocusNavigationSelector( true );
setRef( navMenu.id );
}
} }
onCreateNew={ () => createNavigationMenu( '', [] ) }
Expand Down Expand Up @@ -658,28 +624,22 @@ function Navigation( {
}

// Show a warning if the selected menu is no longer available.
// TODO - the user should be able to select a new one?
if ( ref && isNavigationMenuMissing ) {
return (
<TagName { ...blockProps }>
<BlockControls>
<ToolbarGroup className="wp-block-navigation__toolbar-menu-selector">
<NavigationMenuSelector
ref={ navigationSelectorRef }
currentMenuId={ ref }
clientId={ clientId }
onSelectNavigationMenu={ ( menuId ) => {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectNavigationMenu={ setRef }
onSelectClassicMenu={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
setShouldFocusNavigationSelector( true );
setRef( navMenu.id );
}
} }
onCreateNew={ () => createNavigationMenu( '', [] ) }
Expand Down Expand Up @@ -740,17 +700,17 @@ function Navigation( {
isResolvingCanUserCreateNavigationMenu
}
onSelectNavigationMenu={ ( menuId ) => {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
setRef( menuId );
selectBlock( clientId );
} }
onSelectClassicMenu={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
setShouldFocusNavigationSelector( true );
setRef( navMenu.id );
selectBlock( clientId );
}
} }
onCreateEmpty={ () => createNavigationMenu( '', [] ) }
Expand All @@ -763,37 +723,26 @@ function Navigation( {
<EntityProvider kind="postType" type="wp_navigation" id={ ref }>
<RecursionProvider uniqueId={ recursionId }>
<BlockControls>
{ ! isDraftNavigationMenu && isEntityAvailable && (
<ToolbarGroup className="wp-block-navigation__toolbar-menu-selector">
<NavigationMenuSelector
ref={ navigationSelectorRef }
currentMenuId={ ref }
clientId={ clientId }
onSelectNavigationMenu={ ( menuId ) => {
handleUpdateMenu( menuId );
setShouldFocusNavigationSelector( true );
} }
onSelectClassicMenu={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
handleUpdateMenu( navMenu.id );
setShouldFocusNavigationSelector(
true
);
}
} }
onCreateNew={ () =>
createNavigationMenu( '', [] )
<ToolbarGroup className="wp-block-navigation__toolbar-menu-selector">
<NavigationMenuSelector
currentMenuId={ ref }
clientId={ clientId }
onSelectNavigationMenu={ setRef }
onSelectClassicMenu={ async ( classicMenu ) => {
const navMenu = await convertClassicMenu(
classicMenu.id,
classicMenu.name
);
if ( navMenu ) {
setRef( navMenu.id );
}
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
showManageActions
/>
</ToolbarGroup>
) }
} }
onCreateNew={ () => createNavigationMenu( '', [] ) }
/* translators: %s: The name of a menu. */
actionLabel={ __( "Switch to '%s'" ) }
showManageActions
/>
</ToolbarGroup>
</BlockControls>
{ stylingInspectorControls }
{ isEntityAvailable && (
Expand Down
31 changes: 11 additions & 20 deletions packages/block-library/src/navigation/edit/navigation-menu-selector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,23 @@ import {
import { __, sprintf } from '@wordpress/i18n';
import { decodeEntities } from '@wordpress/html-entities';
import { addQueryArgs } from '@wordpress/url';
import { forwardRef, useMemo } from '@wordpress/element';
import { useMemo } from '@wordpress/element';

/**
* Internal dependencies
*/
import useNavigationMenu from '../use-navigation-menu';
import useNavigationEntities from '../use-navigation-entities';

function NavigationMenuSelector(
{
currentMenuId,
onSelectNavigationMenu,
onSelectClassicMenu,
onCreateNew,
showManageActions = false,
actionLabel,
toggleProps = {},
},
forwardedRef
) {
export default function NavigationMenuSelector( {
currentMenuId,
onSelectNavigationMenu,
onSelectClassicMenu,
onCreateNew,
showManageActions = false,
actionLabel,
toggleProps = {},
} ) {
/* translators: %s: The name of a menu. */
const createActionLabel = __( "Create from '%s'" );

Expand Down Expand Up @@ -78,7 +75,6 @@ function NavigationMenuSelector(

return (
<ToolbarDropdownMenu
ref={ forwardedRef }
label={ __( 'Select Menu' ) }
text={ __( 'Select Menu' ) }
icon={ null }
Expand All @@ -90,10 +86,7 @@ function NavigationMenuSelector(
<MenuGroup label={ __( 'Menus' ) }>
<MenuItemsChoice
value={ currentMenuId }
onSelect={ ( menuId ) => {
onClose();
onSelectNavigationMenu( menuId );
} }
onSelect={ onSelectNavigationMenu }
choices={ menuChoices }
/>
</MenuGroup>
Expand Down Expand Up @@ -142,5 +135,3 @@ function NavigationMenuSelector(
</ToolbarDropdownMenu>
);
}

export default forwardRef( NavigationMenuSelector );
34 changes: 17 additions & 17 deletions packages/e2e-tests/specs/editor/blocks/navigation.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1681,13 +1681,10 @@ Expected mock function not to be called but it was called with: ["POST", "http:/
);
} );

// Skip reason: running it in interactive works but selecting and
// checking for focus consistently fails in the test.
// eslint-disable-next-line jest/no-disabled-tests
it.skip( 'should always focus select menu button after item selection', async () => {
it( 'keeps focus the menu item after navigation menu selection', async () => {
// Create some navigation menus to work with.
await createNavigationMenu( {
title: 'First navigation',
title: 'First Example Navigation',
content:
'<!-- wp:navigation-link {"label":"WordPress Example Navigation","type":"custom","url":"http://www.wordpress.org/","kind":"custom","isTopLevelLink":true} /-->',
} );
Expand All @@ -1703,23 +1700,26 @@ Expected mock function not to be called but it was called with: ["POST", "http:/
// Insert new block and wait for the insert to complete.
await insertBlock( 'Navigation' );
await waitForBlock( 'Navigation' );

const navigationSelector = await page.waitForXPath(
"//button[text()='Select Menu']"
// First select a menu from the block placeholder
const selectMenuDropdown = await page.waitForSelector(
'[aria-label="Select Menu"]'
);
await navigationSelector.click();

const theOption = await page.waitForXPath(
"//button[@aria-checked='false'][contains(., 'First navigation')]"
await selectMenuDropdown.click();
const firstNavigationOption = await page.waitForXPath(
'//button[.="First Example Navigation"]'
);
theOption.click();
await firstNavigationOption.click();

// Once the options are closed, does select menu button receive focus?
const selectMenuDropdown2 = await page.$(
'[aria-label="Select Menu"]'
// Next switch menu using the toolbar.
await clickBlockToolbarButton( 'Select Menu' );

const secondNavigationOption = await page.waitForXPath(
'//button[.="Second Example Navigation"]'
);
await secondNavigationOption.click();

await expect( selectMenuDropdown2 ).toHaveFocus();
// The menu item should still have focus.
await expect( secondNavigationOption ).toHaveFocus();
} );
} );
} );

0 comments on commit 56462ff

Please sign in to comment.