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.

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

Behavior of keyboard navigation focus causing test to fail for quote block #46519

Closed
artemiomorales opened this issue Dec 14, 2022 · 6 comments
Closed
Assignees
Labels
[Block] Quote Affects the Quote Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] E2E Tests /packages/e2e-tests [Type] Bug An existing feature does not function as intended

Comments

@artemiomorales
Copy link
Contributor

artemiomorales commented Dec 14, 2022

Description

Users should be able to unwrap the contents of a Quote Block so it becomes a Paragraph Block(s) using backspace.

While this generally works, the current focus behavior is also causing a test to fail in quote.test.js when running end-to-end tests, erroneously preventing pull requests from passing checks, though it may also pass after one or more failed attempts.

When testing manually, I'm able to reproduce this bug in theme Twenty Twenty-Three, but not Twenty Twenty-Two.

This appears related to #4466

Step-by-step reproduction instructions

  1. Create or edit a post
  2. Insert a quote block
  3. Add some content and press backspace as shown in the video below

Expected: The quote block gets converted to a paragraph block.
Actual: The quote block is deleted.

Screenshots, screen recording, code snippet

This follows the steps taken in packages/e2e-tests/specs/editor/blocks/quote.test.js under the test can be unwrapped with content on Backspace.

quote-block-bug.mov
	it( 'can be unwrapped with content on Backspace', async () => {
		await insertBlock( 'Quote' );
		await page.keyboard.type( '1' );
		await page.keyboard.press( 'ArrowRight' );
		await page.keyboard.type( '2' );

		expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
		"<!-- wp:quote -->
		<blockquote class=\\"wp-block-quote\\"><!-- wp:paragraph -->
		<p>1</p>
		<!-- /wp:paragraph --><cite>2</cite></blockquote>
		<!-- /wp:quote -->"
	` );

		await page.keyboard.press( 'ArrowLeft' );
		await page.keyboard.press( 'ArrowUp' );
		await page.keyboard.press( 'ArrowUp' );
		await page.keyboard.press( 'Backspace' );

		expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
		"<!-- wp:paragraph -->
		<p>1</p>
		<!-- /wp:paragraph -->

		<!-- wp:quote -->
		<blockquote class=\\"wp-block-quote\\"><cite>2</cite></blockquote>
		<!-- /wp:quote -->"
	` );
	} );

Environment info

  • Reproduced using them Twenty Twenty-Three on Gutenberg trunk, 14.7.1, 14.6, 14.5, 14.4, 14.3 (not sure how far back this goes).

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@artemiomorales artemiomorales changed the title Behavior for quote unwrapping with content on backspace breaks test Behavior keyboard navigation focus causing test to fail for quote block Dec 14, 2022
@artemiomorales artemiomorales changed the title Behavior keyboard navigation focus causing test to fail for quote block Behavior of keyboard navigation focus causing test to fail for quote block Dec 14, 2022
@artemiomorales artemiomorales added [Type] Bug An existing feature does not function as intended [a11y] Keyboard & Focus [Package] E2E Tests /packages/e2e-tests labels Dec 14, 2022
@t-hamano t-hamano added the [Block] Quote Affects the Quote Block label Dec 14, 2022
@t-hamano
Copy link
Contributor

I have experienced this test failure in the past.
Even recent GitHub actions show that this test is unstable.

My suggestion would be to first rewrite tests by Playwright as tracked in #38851. I hope that this instability will be resolved and I would like to work on the migration.

@t-hamano t-hamano self-assigned this Dec 14, 2022
@Mamaduka
Copy link
Member

Thank you, @t-hamano. Feel free to ping me for a review when PR is ready.

@artemiomorales
Copy link
Contributor Author

@t-hamano I just realized that the snapshot shown in the test (below) doesn't match the outcome of the manual testing in the video (blank), so I'm not sure how much the original description will help you in tracking this down. Anyway, I thought I'd mention this in case it's helpful.

Screen Shot 2022-12-14 at 8 36 26 AM

@t-hamano
Copy link
Contributor

@artemiomorales

Thank you for your help.

I looked into this issue and found that if the cite element doesn't overlap with the block inserter, it will not behave as intended when the arrow-up key is pressed.

Details are in #46546.

In the following test, we expect the first arrow-up gets the focus on the block inserter and the second arrow-up gets the focus on the paragraph block:

await page.keyboard.press( 'ArrowLeft' );
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'ArrowUp' );
await page.keyboard.press( 'Backspace' );

expect( await getEditedPostContent() ).toMatchInlineSnapshot( `
"<!-- wp:paragraph -->
<p>1</p>
<!-- /wp:paragraph -->

However, in Twenty Twenty Three, the first arrow-up doesn't focus on the block inserter, so the test gets an unexpected result.

Twenty Twenty Two is used by CI, but this might be the reason for the instability. Nevertheless, changing the arrow-up to the arrow-left should produce the expected result for any theme:

tt3_left.mp4

@t-hamano
Copy link
Contributor

I have submitted a PR #46549 to rewrite the E2E test to Playwright. Once this PR is merged into trunk and we can confirm that the test always passes consistently, I would like to close this issue.

@t-hamano
Copy link
Contributor

Since #46549 was merged into trunk, the Playwright test has been run 8 times and this test has never failed. I consider the instability of the test resolved and I would like to close this issue.

@artemiomorales
Thank you for your cooperation!

@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Keyboard & Focus labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Quote Affects the Quote Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] E2E Tests /packages/e2e-tests [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants