Feature: Close popup on "escape" keydown #113
Conversation
| // close popup on escape keyboard event | ||
| if (!this.keyDownHandler && props.keyboard) { | ||
| currentDocument = currentDocument || props.getDocument(); | ||
| this.keyDownHandler = addEventListener(currentDocument, |
There was a problem hiding this comment.
In nest Trigger popup, this makes all Trigger close when press ESC.
There was a problem hiding this comment.
You're right, and I wish it wasn't so.
But this has no simple implementation:
- Must put the event listener on a single object (e.g.
document) since it won't fire on a non-focusable element (e.g.<div>), thus event bubble/capture is irrelevant. - Because of #1, the order of event emission is LIFO and for this to work, we need FIFO. This is unchangeable.
- Even if we do solve this for nested Triggers, because of #2, a wrapping
<Modal>would still react to the keydown event and there's nothing we can do about that (unless we use some custom event manager for all Ant components...).
The reason it works with the<Select>is that it uses an<input>which doesn't have the restriction noted in #1.
The main purpose of this feature is to fix ant-design/ant-design#14699 - the Triggers shouldn't stay open when Modal closes.
Let me know what you think.
There was a problem hiding this comment.
Add global event for key board is not a good way cause ESC can be triggered on any component. We can't tell which ESC should trigger the hide function.
My suggestion is that add key event on child node. If child node can be focusable, it can close by ESC. Else do nothing about it.
There was a problem hiding this comment.
We can't tell which ESC should trigger the hide function
I'm not sure I understand the risk - the event listener is only active if the Trigger is open. If any other component in the page is interacted with (mouse out / click outside / blur), the Trigger would close and the listener would be removed.
If child node can be focusable, it can close by ESC
Yeah that could work.
Else do nothing about it.
Not worried that it won't be consistent?
There was a problem hiding this comment.
Else do nothing about it.
Not worried that it won't be consistent?
If we go this route, I think we should make this feature enabled by default and non-configurable. That way there's no expectation, no communication and no risk involved. Just a bug fix.
There was a problem hiding this comment.
Here is the sample: https://codesandbox.io/s/l5p88216l
ESC to close is good help function, but we need to consider some user may not want close by press ESC everywhere. So I hope just handle can be focused one. (Like button or something else)
|
Thank you for the cooperation @zombieJ. Putting this aside as it is not crucial. |
Due to issue ant-design/ant-design#14699 in which popover stays open when modal is escaped, I'd like to allow first closing the popover, then the modal (similar to select in modal behavior).
Commit #1: Added feature
Added a prop
keyboard(not the best keyword but following the similar feature in<Dialog>. Let me know if you'd prefer a different name), false by default.Adds keydown listener when popup visible, self removes when closed.
Commit # 2: Updated simple example
Added checkbox "keyboard" to
simple.htmland when checked, no matter what action trigger opens the popup, press "esc" and it closes.Commit #3: Updated Readme.md
Updated the table of props.
Commit #4: Added tests
Test #;1 -
keyboard=true, open popup, click esc - assert that popup closed.Test #2 -
keyboard=false, open popup, click esc - assert that popup is still open.Notice that simulating an "esc" event can't be done with
TestUtils.Simulate.keyDownprobably cause PhantomJS doesn't support the current browser API. So I made a simple polyfill util. Works well.