Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix navigation select menu focus loss #42956

Closed
wants to merge 1 commit into from
Closed

Fix navigation select menu focus loss #42956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 28 additions & 79 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
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';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be unused. Previously, 'unsaved inner blocks' when saved were created as drafts, but that was accidentally removed months ago by some refactoring and no-one really complained, so I'll remove this as well. 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think @draganescu might need it back very shortly 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was exploring the idea of saving menus as drafts initially indifferent of how they're created so the user does the "final call" action when clicking save. The idea is to avoid fiddling with live site menus by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that there's still some code that changes the status to publish when saving wp_navigation entities in an editor:

gutenberg/packages/editor/src/components/entities-saved-states/index.js

Lines 32 to 37 in 54c9d81

const PUBLISH_ON_SAVE_ENTITIES = [
{
kind: 'postType',
name: 'wp_navigation',
},
];

gutenberg/packages/editor/src/components/entities-saved-states/index.js

Lines 149 to 157 in 54c9d81

if (
PUBLISH_ON_SAVE_ENTITIES.some(
( typeToPublish ) =>
typeToPublish.kind === kind &&
typeToPublish.name === name
)
) {
editEntityRecord( kind, name, key, { status: 'publish' } );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@draganescu Let me know what you'd like me to do. I can make a separate PR that removes all of the draft handling, which you can revert when you need it back. Or I can put isDraftNavigationMenu back in this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should proceed as things are no need for any separate PR. We'll see how trunk looks when we start changing the publish behavior 🙏🏻


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,
] );
getdave marked this conversation as resolved.
Show resolved Hide resolved

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 && (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these conditions do anything important, because there are already early returns in the component for missing entities or unselected menus.

<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
Open in desktop
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
Open in desktop
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();
} );
} );
} );