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

Stop keypresses being caught by other elements when they happen in a CustomSelectControl #30557

Merged
merged 10 commits into from
Jan 11, 2022

Conversation

opr
Copy link
Contributor

@opr opr commented Apr 7, 2021

Description

In the <ul> output by CustomSelectControl I have added a function to handle onKeyDown - this stops the event from propagating, but still calls the onKeyDown handler (which is derived ultimately from downshift).

This PR fixes #30204.

How has this been tested?

In the linked issue (#30204) the reproducible steps can also be used to test:

  1. Open the storybook for this project: npm run storybook:dev
  2. Go to the CustomSelectControl > Default component, click it and start typing s to get to the entry Small
  3. Notice the storybook sidebar no longer closes due to the s keypress being cancelled.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards. Unsure, I had to add eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions but I don't see any other way of capturing this event
  • I've tested my changes with keyboard and screen readers.

@github-actions
Copy link

github-actions bot commented Apr 7, 2021

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @opr! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 7, 2021
@nerrad nerrad added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Apr 8, 2021
@opr
Copy link
Contributor Author

opr commented Jun 21, 2021

Hi folks - coming in with an update to provide context. Here's a real-world example of what this PR aims to solve, when a user has a WordPress site, connected via Jetpack, there is a sidebar that can be opened by pressing N.

When a customer is using WooCommerce Blocks, there are input fields for Country/State that use CustomSelectControl.

If the user tries to type into these boxes, for example 'United Kingdom', then the N key is hit, causing the sidebar to open.

here's a video to show it happening (note the inputs given while the select box is focused are u -> n and the sidebar appears on the right-hand side immediately after n.)

https://www.loom.com/share/2dbe6a589c4541599438782d066151a7

@gziolo gziolo requested a review from ciampo October 19, 2021 15:32
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @opr !

After reading a bit more the issue in downshift and related discussions there, this approach seems to be the suggested workaround. I'm wondering though why even with this code, the Escape still propagated.

I inserted a CustomSelectControl in a Modal here, after the TextControl
-- demo snippet:

<CustomSelectControl
	label="Font Size"
	options={ [
		{
			key: 'small',
			name: 'Small',
			style: { fontSize: '50%' },
		},
		{
			key: 'normal',
			name: 'Normal',
			style: { fontSize: '100%' },
		},
	] }
/>

Then:

  1. insert a Template Part and click New template part
  2. open the select control
  3. press Escape

I would expect the Modal to stay open and only the select to close, but this is not happening..

packages/components/src/custom-select-control/index.js Outdated Show resolved Hide resolved
@opr opr force-pushed the fix/keydown-listener-customselect branch from 459df8b to e7ba1b9 Compare November 26, 2021 14:11
@opr
Copy link
Contributor Author

opr commented Nov 26, 2021

Thanks @ntsekouras I've made a change to stop the synthetic event propagating and it has solved the issue you mentioned in your review.

@opr opr requested a review from ntsekouras November 26, 2021 14:14
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.

Hey @opr , thank you for working on this fix!

I've added a couple of inline comments. On top of that, I've got two additional suggestions:

  • Could you add a new entry to the CHANGELOG ?
  • Could you add a unit test for this fix?

Thank you!

packages/components/src/custom-select-control/index.js Outdated Show resolved Hide resolved
packages/components/src/custom-select-control/index.js Outdated Show resolved Hide resolved
@ciampo ciampo added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 30, 2021
@ciampo ciampo added the [Feature] Component System WordPress component system label Nov 30, 2021
@opr opr force-pushed the fix/keydown-listener-customselect branch from e7ba1b9 to ce80b19 Compare January 10, 2022 17:02
@opr
Copy link
Contributor Author

opr commented Jan 10, 2022

Hey @ciampo thanks for the review and sorry for the delay! All done and ready for a re-review :)

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.

Hey @opr , thank you for addressing the initial round of feedback!

I've left some additional feedback to align the unit tests with the most common practices used in the components package.

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 @opr for swiftly addressing the feedback! Code changes LGTM 🚀

The e2e test failures don't seem related, maybe rebasing would solve them?

@opr opr force-pushed the fix/keydown-listener-customselect branch from 6014960 to 58a2dca Compare January 11, 2022 09:17
@opr
Copy link
Contributor Author

opr commented Jan 11, 2022

Thank you @opr for swiftly addressing the feedback! Code changes LGTM 🚀

The e2e test failures don't seem related, maybe rebasing would solve them?

Rebasing worked, thanks @ciampo

@ciampo ciampo merged commit d1da591 into WordPress:trunk Jan 11, 2022
@github-actions github-actions bot added this to the Gutenberg 12.4 milestone Jan 11, 2022
@opr opr deleted the fix/keydown-listener-customselect branch January 11, 2022 10:59
@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jan 17, 2022
noisysocks pushed a commit that referenced this pull request Jan 17, 2022
…CustomSelectControl (#30557)

* Stop the keydown event from propagating in CustomSelect component

* Stop the synthetic event propagating instead of native

This stops other listeners receiving this event, whereas stopping the native event still somehow allowed propagation.

* Move keyDown handler into useCallback and use optional chaining

* Add unit tests for CustomSelectControl

* Update changelog

* Add inline comment about role and remove unneeded class name

* Move mocked function into test

* Add correct spacing to changelog

* Add options as an array of key value pairs

* Use accessible roles instead of looking for elements by className
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CustomSelectControl does not prevent keypress events from bubbling up
5 participants