Fix ComboboxControl reset button when using the keyboard. #63410

afercia · 2024-07-11T08:49:16Z

Fixes #63408

What?

The combobox Reset button doesn't work with the Enter key.

Why?

All UI controls should work with the keyboard.

How?

Prevents propagation of the keydown event.

Testing Instructions

Repeat the test instructions from the issue ComboboxControl: the Reset button doesn't work when using the keyboard #63408
Observe the pressing the Enter key on the Reset button does reset the comobox.

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2024-07-11T08:53:32Z

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: afercia <afercia@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

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

ciampo · 2024-07-11T12:26:45Z

packages/components/CHANGELOG.md

@@ -19,6 +19,7 @@

 ### Bug Fixes

+-   `ComboboxControl`: Fix ComboboxControl reset button when using the keyboard. ([#63410](https://github.com/WordPress/gutenberg/pull/63410))


We will need to move this CHANGELOG entry to the Unreleased section above, in a new Bug fixes sub section

ciampo

Thank you for working on this, @afercia!

Code changes LGTM. Would you be able to also add a unit test for the reset behavior, so that we don't incur in future regressions?

afercia · 2024-07-11T14:40:06Z

Tanks @ciampo. Sure will work on an ~~e2e~~ unit test tomorrow and adjust the changelog.

ciampo · 2024-07-11T14:55:18Z

Sure will work on an e2e test tomorrow and adjust the changelog.

A unit test is better suited — we already have unit tests for ComboboxControl in packages/components/src/combobox-control/test/index.tsx, so you could just add a new one.

You would then be able to run the test suite with the command npm run test:unit packages/components/src/combobox-control. Happy to help if you need!

afercia · 2024-07-12T07:36:45Z

@ciampo I added a few unit tests. Confirmed the specific test for the Enter key fails without the fix in this PR.
Thanks for your feedback and review.

afercia · 2024-07-12T07:47:46Z

Note: I was surprised the Testing Library syntax to press the Spacebar is await user.keyboard( ' ' );.
I would've expected something like await user.keyboard( '{Spacebar} );
Is that correct? Any other more elegant syntax to use?

github-actions · 2024-07-12T07:56:43Z

Flaky tests detected in a978978.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9904222095
📝 Reported issues:

[Flaky Test] Create a pattern with overrides #58009 in /test/e2e/specs/editor/various/pattern-overrides.spec.js

ciampo · 2024-07-12T14:03:32Z

Is that correct? Any other more elegant syntax to use?

await user.keyboard( '[Space]' ) should work

ciampo · 2024-07-12T14:05:54Z

packages/components/src/combobox-control/test/index.tsx

+		expect( resetButton ).toBeInTheDocument();
+		expect( resetButton ).toBeVisible();


Suggested change

expect( resetButton ).toBeInTheDocument();

expect( resetButton ).toBeVisible();

expect( resetButton ).toBeVisible();

toBeVisible() should already imply that the element is in the document

Yup, I just copied and pasted from the existing test should render with visible label, will fix it there as well

ciampo · 2024-07-12T14:08:12Z

packages/components/src/combobox-control/test/index.tsx

+	it( 'should render with Reset button enabled after option selection', async () => {
+		const user = await userEvent.setup();
+		const targetOption = timezones[ 13 ];
+
+		render(
+			<Component
+				options={ timezones }
+				label={ defaultLabelText }
+				allowReset
+			/>
+		);
+
+		// Pressing tab selects the input and shows the options.
+		await user.tab();
+		// Type enough characters to ensure a predictable search result.
+		await user.keyboard( getOptionSearchString( targetOption ) );
+		// Pressing Enter/Return selects the currently focused option.
+		await user.keyboard( '{Enter}' );
+
+		const resetButton = screen.getByRole( 'button', { name: 'Reset' } );
+
+		expect( resetButton ).toBeInTheDocument();
+		expect( resetButton ).toBeVisible();
+		expect( resetButton ).toBeEnabled();
+	} );


This test feels superflous, since the next three tests (click/enter/spacebar on the button) would also fail if the button was disabled. At most we can add the expect( resetButton ).toBeEnabled() check in each test?

afercia · 2024-07-12T14:36:30Z

Thanks, I think I addressed all the points.

…63410) * Fix combobox reset button when using the keyboard. * Add changelog entry. * Add unit tests. * Adjust changelog entry. * Improve unit tests. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>

afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Jul 11, 2024

afercia requested a review from ajitbohra as a code owner July 11, 2024 08:49

afercia changed the title ~~Fix combobox reset button when using the keyboard.~~ Fix ComboboxControl reset button when using the keyboard. Jul 11, 2024

afercia requested review from mirka and ciampo July 11, 2024 08:53

ciampo reviewed Jul 11, 2024

View reviewed changes

ciampo approved these changes Jul 11, 2024

View reviewed changes

ciampo self-requested a review July 11, 2024 12:26

afercia added 4 commits July 12, 2024 09:30

Fix combobox reset button when using the keyboard.

0b9b6c3

Add changelog entry.

73b7d85

Add unit tests.

227e3c7

Adjust changelog entry.

a978978

afercia force-pushed the fix/combobox-reset-button-keyboard branch from 36a8345 to a978978 Compare July 12, 2024 07:35

ciampo reviewed Jul 12, 2024

View reviewed changes

Improve unit tests.

0b32859

afercia merged commit c54c002 into trunk Jul 15, 2024
61 checks passed

afercia deleted the fix/combobox-reset-button-keyboard branch July 15, 2024 12:49

github-actions bot added this to the Gutenberg 18.9 milestone Jul 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ComboboxControl reset button when using the keyboard. #63410

Fix ComboboxControl reset button when using the keyboard. #63410

afercia commented Jul 11, 2024

github-actions bot commented Jul 11, 2024 •

edited

Loading

ciampo Jul 11, 2024

ciampo left a comment

afercia commented Jul 11, 2024 •

edited

Loading

ciampo commented Jul 11, 2024

afercia commented Jul 12, 2024

afercia commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

ciampo commented Jul 12, 2024

ciampo Jul 12, 2024

afercia Jul 12, 2024

ciampo Jul 12, 2024

afercia Jul 12, 2024

afercia commented Jul 12, 2024

		@@ -19,6 +19,7 @@

		### Bug Fixes

		- `ComboboxControl`: Fix ComboboxControl reset button when using the keyboard. ([#63410](https://github.com/WordPress/gutenberg/pull/63410))

		expect( resetButton ).toBeInTheDocument();
		expect( resetButton ).toBeVisible();

Fix ComboboxControl reset button when using the keyboard. #63410

Fix ComboboxControl reset button when using the keyboard. #63410

Conversation

afercia commented Jul 11, 2024

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Jul 11, 2024 • edited Loading

ciampo Jul 11, 2024

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

afercia commented Jul 11, 2024 • edited Loading

ciampo commented Jul 11, 2024

afercia commented Jul 12, 2024

afercia commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

ciampo commented Jul 12, 2024

ciampo Jul 12, 2024

Choose a reason for hiding this comment

afercia Jul 12, 2024

Choose a reason for hiding this comment

ciampo Jul 12, 2024

Choose a reason for hiding this comment

afercia Jul 12, 2024

Choose a reason for hiding this comment

afercia commented Jul 12, 2024

github-actions bot commented Jul 11, 2024 •

edited

Loading

afercia commented Jul 11, 2024 •

edited

Loading