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 ComboboxControl reset button when using the keyboard. #63410

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Fix ComboboxControl reset button when using the keyboard. #63410

merged 5 commits into from
Jul 15, 2024

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Jul 11, 2024

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

Testing Instructions for Keyboard

Screenshots or screencast

@afercia 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 afercia requested a review from ajitbohra as a code owner July 11, 2024 08:49
@afercia afercia changed the title Fix combobox reset button when using the keyboard. Jul 11, 2024
@github-actions GitHub Actions
Copy link

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

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.
@afercia afercia requested review from mirka and ciampo July 11, 2024 08:53
ciampo
ciampo reviewed Jul 11, 2024
View reviewed changes
packages/components/CHANGELOG.md Outdated
@@ -19,6 +19,7 @@

### Bug Fixes

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

Choose a reason for hiding this comment

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

We will need to move this CHANGELOG entry to the Unreleased section above, in a new Bug fixes sub section
ciampo
ciampo approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

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?
@ciampo ciampo self-requested a review July 11, 2024 12:26
@afercia
Copy link
Contributor Author

afercia commented Jul 11, 2024 •
edited
Loading

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

ciampo commented Jul 11, 2024

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 afercia force-pushed the fix/combobox-reset-button-keyboard branch from 36a8345 to a978978 Compare July 12, 2024 07:35
@afercia
Copy link
Contributor Author

afercia commented Jul 12, 2024

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

afercia commented Jul 12, 2024

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 GitHub Actions
Copy link

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:
@ciampo
Copy link
Contributor

ciampo commented Jul 12, 2024

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

await user.keyboard( '[Space]' ) should work
ciampo
ciampo reviewed Jul 12, 2024
View reviewed changes
packages/components/src/combobox-control/test/index.tsx Outdated
Comment on lines 321 to 322
expect( resetButton ).toBeInTheDocument();
expect( resetButton ).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

toBeVisible() should already imply that the element is in the document
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I just copied and pasted from the existing test should render with visible label, will fix it there as well
packages/components/src/combobox-control/test/index.tsx Outdated
Comment on lines 326 to 350
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();
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

OK
@afercia
Copy link
Contributor Author

afercia commented Jul 12, 2024

Thanks, I think I addressed all the points.
@afercia afercia merged commit c54c002 into trunk Jul 15, 2024
61 checks passed
@afercia afercia deleted the fix/combobox-reset-button-keyboard branch July 15, 2024 12:49
@github-actions github-actions bot added this to the Gutenberg 18.9 milestone Jul 15, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jul 18, 2024
@afercia @ciampo @carstingaxion
Fix ComboboxControl reset button when using the keyboard. (WordPress#…
cc53a6a 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
2 participants
@afercia @ciampo