E2E Tests: Split large Navigation tests into smaller tests

[im3dabasia]

What?

Closes #64305

Why?

The existing E2E test for the Navigation block was too large, making it harder to maintain and debug. Splitting it into smaller tests improves reliability and clarity.

How?

Refactored the test by breaking it into multiple smaller tests, each focusing on a specific aspect of managing focus while creating, editing, and deleting items.

Testing Instructions

1. Run the E2E tests.
2. Ensure all tests pass without failures.
3. Verify that focus management works correctly when creating, editing, and deleting navigation items.

To run the single test file :

npm run test:e2e test/e2e/specs/editor/blocks/navigation.spec.js

Screenshots or screencast

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <im3dabasia1@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[t-hamano]

Sorry for the late reply.

I'd like to review this PR, but can you fix the conflict first?

Thank you!

[im3dabasia]

Hi @t-hamano ,

Thank you for the review.

The conflicts were related to this particular code the trunk now uses primaryShift+Backspace. instead of Backspace which was used previously in this PR (trunk when this branch was created). I resolved the conflicts and kept the new implementation.

await pageUtils.pressKeys( 'primaryShift+Backspace' );

Since we have split the tests, a lot of the code has been refactored. I have tried to minimize repetition as much as possible. Previously, we had one large test where we tested many things iteratively and built each subsequent test upon the previous setup. This approach is no longer ideal here, as it would make the tests unnecessarily large.

I have focused on creating individual tests with titles that clearly indicate what's being tested. This approach has allowed us to reduce redundant code and focus on testing only what's necessary, rather than having previous implementations that aren't needed in the split tests.

After this PR:

This are the tests after split:

The "Navigation block focus management" tests the following:

1. Focus management when using the navigation link appender
2. Focus management when creating navigation links
3. Can add submenu item using the keyboard
4. Can add submenu item(custom-link) using the keyboard
5. Deleting returns items focus to its sibling
6. Adding new links to a navigation block with existing inner blocks triggers creation of a single Navigation Menu

Please LMK your feedback. Looking forward

[im3dabasia]

Hi @t-hamano , I'd appreciate your thoughts/input on this PR. When you have a moment please check this PR.

[t-hamano]

Thanks for the update! I've left some feedback.

Additionally, the following test should not be part of focus management tests:

Adding new links to a navigation block with existing inner blocks triggers creation of a single Navigation Menu

That is, they should be grouped as follows:

Before:

Navigation block › Navigation block focus management › Focus management when using the navigation link appender
Navigation block › Navigation block focus management › Focus management when creating navigation links
Navigation block › Navigation block focus management › Can add submenu item using the keyboard
Navigation block › Navigation block focus management › Can add submenu item(custom-link) using the keyboard
Navigation block › Navigation block focus management › Deleting returns items focus to its sibling
Navigation block › Navigation block focus management › Adding new links to a navigation block with existing inner blocks triggers creation of a single Navigation Menu

After:

Navigation block › Navigation block focus management › Focus management when using the navigation link appender
Navigation block › Navigation block focus management › Focus management when creating navigation links
Navigation block › Navigation block focus management › Can add submenu item using the keyboard
Navigation block › Navigation block focus management › Can add submenu item(custom-link) using the keyboard
Navigation block › Navigation block focus management › Deleting returns items focus to its sibling
Navigation block › Adding new links to a navigation block with existing inner blocks triggers creation of a single Navigation Menu

[t-hamano]

Another thing that bothers me is that there is some code duplication.

For example, here's the code, which should only need to be tested once:

			/**
			 * Test: Can add submenu item using the keyboard
			 */
			navigation.useToolbarButton( 'Add submenu' );

			// Expect the submenu Add link to be present
			await expect(
				editor.canvas.locator( 'a' ).filter( { hasText: 'Add link' } )
			).toBeVisible();

			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();

			await navigation.addPage( 'Dog' );

			/**
			 * 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' );

[im3dabasia]

@t-hamano ,

Sorry for the delay, I have implemented the changes you had suggested earlier.

Request you to look at the current implementation, and provide me additional feedbacks

[t-hamano]

Sorry for the late reply!.

Almost looks good, but finally, it seems like the test for deleting items contains some redundant code.

With that in mind, we can remove the unnecessary assertions from this code and compress it to the following:

Details

test( 'Deleting items', async ( {
	page,
	pageUtils,
	navigation,
	editor,
} ) => {
	// Add top-level nav items.
	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' );

	// Add submenu items.
	navigation.useToolbarButton( 'Add submenu' );
	// Expect the submenu Add link to be present
	await expect(
		editor.canvas.locator( 'a' ).filter( { hasText: 'Add link' } )
	).toBeVisible();
	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();
	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' );
	await pageUtils.pressKeys( 'ArrowUp' );

	/**
	 * Test: Deleting second item returns focus to its sibling
	 */
	await pageUtils.pressKeys( 'access+z' );
	await navigation.checkLabelFocus( 'Dog' );

	/**
	 * Test: Deleting first item returns focus to the parent submenu item
	 */
	// Add a link back so we can delete the first submenu 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 pageUtils.pressKeys( 'ArrowUp', { times: 2 } );
	await navigation.checkLabelFocus( 'Dog' );
	await pageUtils.pressKeys( 'ArrowUp', { times: 1 } );
	await pageUtils.pressKeys( 'access+z' );
	await pageUtils.pressKeys( 'ArrowDown' );
	await navigation.checkLabelFocus( 'example.com' );

	/**
	 * Test: Deleting top-level second item returns focus to its sibling
	 */
	await pageUtils.pressKeys( 'access+z' );
	await navigation.checkLabelFocus( 'Cat' );

	/**
	 * Test: Deleting with no more siblings should focus the navigation block again
	 */
	await pageUtils.pressKeys( 'access+z' );
	await expect( navigation.getNavBlock() ).toBeFocused();
	// Wait until the nav block inserter is visible before we continue.
	await expect( navigation.getNavBlockInserter() ).toBeVisible();
	// Now the appender should be visible and reachable with an arrow down
	await pageUtils.pressKeys( 'ArrowDown' );
	await expect( navigation.getNavBlockInserter() ).toBeFocused();
} );

The features of this code are as follows:

• The title was changed from Deleting returns items focus to its sibling to Deleting items because focus can return to more than just siblings.
• The code has been organized into the following four sections:
  • Add top-level nav items.
  • Add submenu items.
  • Test: Deleting second item returns focus to its sibling
  • Test: Deleting first item returns focus to the parent submenu item
  • Test: Deleting top-level second item returns focus to its sibling
  • Test: Deleting with no more siblings should focus the navigation block again
• Unnecessary assertions have been removed.
• Use the access+z key instead of the backspace key: Is there a reason you changed to using the backspace key?

Let me know your thoughts!

[t-hamano]

@im3dabasia I'm sorry for the late reply. I've missed confirming your changes.

The changes all look good. Just to be sure, can you rebase on top of the trunk branch before merging?