Dismiss Modal with Escape Keypress

[nonissue]

Feature request 🚀

• I will create Pull Request
• It's just a suggestion

Expected

• "Escape" key should dismiss modal as per https://www.w3.org/TR/wai-aria-practices-1.1/examples/dialog-modal/dialog.html

Examples

• See https://vercel.com/design/modal for how I think this should function.

Misc

• It may be worth implementing a useKeyPress hook so the functionality can be used elsewhere. Should I open a second issue for this suggestion?
• On that note, this may be a consistent behaviour to add to many components, like the Auto-Complete component (Vercel's implementation also allows you to dismiss the results list by pressing escape).

I may be able to tackle this, but just started playing with the project today, and so I'll look things over and read the contributing guide, and update this issue if I feel capable.

[unix]

I quite agree with this feature, this is a very constructive suggestion. 👍

I have the following views on this function:

• As you said, there are now multiple components that can add this function.
• Each component should have a prop(keyboard={false}) to turn off this function.
• Add a hook (useKeypress) is a good idea! We can even open the hooks to users. (as a separate function is also good)
• I can do it, I will start the work as soon as I finish feat(modal): lock tab action inside modal #354 . But you can take them if you want any of them.

Feel free to leave me any thoughts or updates.

[nonissue]

Thanks for being so receptive.

I'll review the contributing guidelines and see if I can implement it tomorrow. I actually have written the use-key-press hook for another project, so I'll modify my implementation to match the guidelines, write the tests, and make sure I fulfill all the other requirements. I should be able to submit the PR tomorrow evening if all goes well.

If the code for the hook looks good and I have the time this week, I can take a stab at modifying components like modal to add the hook and the keyboard prop (Very good idea).

Should we start a list somewhere for the components that should implement the hook?

Sorry if I'm a bit scattered, I haven't contributed to any projects before!

Thanks again.

[nonissue]

@unix So I've implemented use-key-press, written some tests (please review and provide feedback if you have any, I don't have much experience with testing react), verified tests are passing. I haven't submitted PR yet because the docs are not complete, but if you could give it a look over and let me know if it's an acceptable start I'll continue working on it!

https://github.com/nonissue/react/tree/feat/use-key-press

PR on my repo so you can easily view the changes:

https://github.com/nonissue/react/pull/2

[unix]

@nonissue Thank you for your amazing work!
In the design of API, I have some simple suggestions for the implementation of useKeypress:

• Reduce user code.

      1. I know I can get the trigger of handler through useEffect, but users still need a lot of extra code to do it, maybe we can provide a more convenient API:

// Sometimes users need to use Event, we should not discard this information.
const handler = event => alert('down')
useKeyPress('Escape', handler)

      2. Use type to distinguish action: (most of the time, users only use keydown)

useKeyPress('Escape', handler, {
  type: 'keyup',  // 'keydown' 'keyup'
})

• Support the Array to help users automatically listen to multiple keys:

// listen to two actions at the same time
useKeyPress(['Enter', 'Escape'], handler)

// Listen combo action
// All keys are pressed at the same time to trigger
useKeyPress(['Shift', 'Escape'], handler, { combo: true })

• Easy to expand. We support adding custom params, which may help in the future. For example, when we need to deal with the holding time of the keyboard, we can easily expand the params. ({ time: 3000 }) I just give an example to show that parameters are good design:

type Options = {
  ...
}
UseKeyPress = (key: string, handler: Function, options: Options) => void

• If possible, I'm not sure, it may be helpful to specify the elements that listen for events:

useKeyPress('Enter', handler, {
  element: SomeRef.current,
})

Besides, about the style of the code, �I suggest that some comments can be reduced. Add comments only for special scenarios or dirty logic, good naming and logic itself are very readable.
And, you can add a file (use-key-press.mdx) in folder /pages/en-us/components, then run command yarn dev, so we can add a document for hooks.

These are my personal suggestions, I didn't write the code, so the ideas may not be accurate, I'd like to hear from you.

[nonissue]

Thanks for the feedback! The comments were only meant to be temporary, I didn't intend to merge this as it is, I just created the PR so you could more easily see the changes.

I agree with:

• Modifying the hook to accept a handler, this makes a lot of sense
• Accept both a string or an array of strings for the keys param (in fact, I've already implemented this locally 👍)

I'm not so sure about:

• Having a type param. I'm not 100% against it, but it might be premature optimization? Are there any existing comments where you could see this being useful? If we do handle event types, this could possibly be something passed in the options object?
• Handling combo key presses (eg. all keys have to be pressed at once). I did look into this quickly, and I think it might be quite complex to implement, but again, it could be worthwhile if there's a need for it, or we could maybe consider it as a separate hook?
• "Specifying the elements that listen for hooks" -> Ah this is interesting. I think it would be doable, and this could maybe be used for situations like trapping focus in the modal (like when tab is pressed). Does that make sense?

I'll push what I've done tonight to the branch on my repo (definitely not finished) so you can have a look. I may not have time to work on things tomorrow, but should be able to spend some time on this later in the week.

[unix]

@nonissue I'm glad to see you're following up on this, I agree with you, the last three features you mentioned actually don't use scenarios, and we really don't have to think about them too early.

It's a good choice for us to implement hook first in a simpler way, if there is a similar demand in the future, we can follow up and solve it. (we can discuss these complex features in a later release, keep it easy, please don't have too much burden.)

[unix]

@nonissue Are there any updates here?

[gepd]

Would be good to have this hook available, currently there are some accessibility problems with some components (select, toggle, slider), that only can be solve with a hook like this @nonissue do you think you will have time to finish this? I can help you if you want

[unix]

Now we have done this by adding useKeyboard. See the canary documentation for more information.

[unix]

Response to keyboard events has now been added to the modal internals as well. (added keyboard props) #574