-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Stop keypresses being caught by other elements when they happen in a CustomSelectControl #30557
Conversation
👋 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. |
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 If the user tries to type into these boxes, for example 'United Kingdom', then the here's a video to show it happening (note the inputs given while the select box is focused are |
There was a problem hiding this 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:
- insert a
Template Part
and clickNew template part
- open the select control
- press
Escape
I would expect the Modal to stay open and only the select to close, but this is not happening..
459df8b
to
e7ba1b9
Compare
Thanks @ntsekouras I've made a change to stop the synthetic event propagating and it has solved the issue you mentioned in your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e7ba1b9
to
ce80b19
Compare
Hey @ciampo thanks for the review and sorry for the delay! All done and ready for a re-review :) |
There was a problem hiding this 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.
There was a problem hiding this 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?
This stops other listeners receiving this event, whereas stopping the native event still somehow allowed propagation.
6014960
to
58a2dca
Compare
…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
Description
In the
<ul>
output byCustomSelectControl
I have added a function to handleonKeyDown
- this stops the event from propagating, but still calls theonKeyDown
handler (which is derived ultimately fromdownshift
).This PR fixes #30204.
How has this been tested?
In the linked issue (#30204) the reproducible steps can also be used to test:
npm run storybook:dev
CustomSelectControl > Default
component, click it and start typings
to get to the entrySmall
s
keypress being cancelled.Types of changes
Bug fix
Checklist: