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

Feature: Close popup on "escape" keydown #113

Closed
wants to merge 4 commits into from
Closed

Feature: Close popup on "escape" keydown #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc95213
Add ability to press Escape to close
ranbena Feb 2, 2019
a14974b
Updated simple example
ranbena Feb 2, 2019
bc47e15
Updated readme
ranbena Feb 2, 2019
b408cba
Added tests
ranbena Feb 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,12 @@ ReactDOM.render((
<td>true</td>
<td>whether to support click mask to hide</td>
</tr>
<tr>
<td>keyboard</td>
<td>Boolean</td>
<td>false</td>
<td>whether support press esc to close</td>
</tr>
<tr>
<td>popupVisible</td>
<td>boolean</td>
Expand Down
17 changes: 17 additions & 0 deletions examples/simple.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ class Test extends React.Component {
});
}

onKeyboard = (e) => {
this.setState({
keyboard: e.target.checked,
});
}

destroy = () => {
this.setState({
destroyed: true,
Expand Down Expand Up @@ -248,6 +254,16 @@ class Test extends React.Component {
maskClosable
</label>

&nbsp;&nbsp;&nbsp;&nbsp;
<label>
<input
checked={!!this.state.keyboard}
type="checkbox"
onChange={this.onKeyboard}
/>
keyboard
</label>

<br />
<label>
offsetX:
Expand Down Expand Up @@ -296,6 +312,7 @@ class Test extends React.Component {
</div>
}
popupTransitionName={state.transitionName}
keyboard={state.keyboard}
>
<a
style={{ margin: 20, display: 'inline-block', background: `rgba(255, 0, 0, 0.05)` }}
Expand Down
23 changes: 23 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import contains from 'rc-util/lib/Dom/contains';
import addEventListener from 'rc-util/lib/Dom/addEventListener';
import ContainerRender from 'rc-util/lib/ContainerRender';
import Portal from 'rc-util/lib/Portal';
import KeyCode from 'rc-util/lib/KeyCode';
import classNames from 'classnames';

import { getAlignFromPlacement, getAlignPopupClassName } from './utils';
Expand Down Expand Up @@ -78,6 +79,7 @@ export default class Trigger extends React.Component {
maskAnimation: PropTypes.string,
stretch: PropTypes.string,
alignPoint: PropTypes.bool, // Maybe we can support user pass position in the future
keyboard: PropTypes.bool,
};

static contextTypes = contextTypes;
Expand Down Expand Up @@ -105,6 +107,7 @@ export default class Trigger extends React.Component {
action: [],
showAction: [],
hideAction: [],
keyboard: false,
};

constructor(props) {
Expand Down Expand Up @@ -196,6 +199,13 @@ export default class Trigger extends React.Component {
this.contextMenuOutsideHandler2 = addEventListener(window,
'blur', this.onContextMenuClose);
}
// close popup on escape keyboard event
if (!this.keyDownHandler && props.keyboard) {
currentDocument = currentDocument || props.getDocument();
this.keyDownHandler = addEventListener(currentDocument,
Copy link
Member

Choose a reason for hiding this comment

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

In nest Trigger popup, this makes all Trigger close when press ESC.

Copy link
Author

@ranbena ranbena Feb 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, and I wish it wasn't so.

But this has no simple implementation:

  1. 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.
  2. Because of #​1, the order of event emission is LIFO and for this to work, we need FIFO. This is unchangeable.
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ranbena ranbena Feb 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

'keydown', this.onKeyDown);
}

return;
}

Expand Down Expand Up @@ -334,6 +344,14 @@ export default class Trigger extends React.Component {
}
}

onKeyDown = (event) => {
// close popup if escape key pressed
if (event.keyCode === KeyCode.ESC || event.key === 'Escape') {
event.stopPropagation();
this.close();
}
}

getPopupDomNode() {
// for test
if (this._component && this._component.getPopupDomNode) {
Expand Down Expand Up @@ -516,6 +534,11 @@ export default class Trigger extends React.Component {
this.touchOutsideHandler.remove();
this.touchOutsideHandler = null;
}

if (this.keyDownHandler) {
this.keyDownHandler.remove();
this.keyDownHandler = null;
}
}

createTwoChains(event) {
Expand Down
30 changes: 30 additions & 0 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,33 @@ export function getAlignPopupClassName(builtinPlacements, prefixCls, align, isAl
export function saveRef(name, component) {
this[name] = component;
}

// TestUtils.Simulate.keyDown doesn't work on PhantomJS
// https://github.com/ariya/phantomjs/issues/11289#issuecomment-278147426
export function keyboardEvent(eventType, init) {
try {
return new KeyboardEvent(eventType, init);
} catch (error) {
const modKeys = [
init.ctrlKey ? 'Control' : '',
init.shiftKey ? 'Shift' : '',
init.altKey ? 'Alt' : '',
init.altGrKey ? 'AltGr' : '',
init.metaKey ? 'Meta' : '',
].join(' ');
const keyEvent = document.createEvent('KeyboardEvent');
keyEvent.initKeyboardEvent(
eventType, // type
false, // canBubble
false, // cancelable
window, // view
init.char || '', // char
init.keyCode || 0, // key
0, // location
modKeys,
init.repeat || false,
);
keyEvent.key = init.key;
return keyEvent;
}
}
62 changes: 61 additions & 1 deletion tests/basic.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import '../assets/index.less';
import Trigger from '../index';
import './basic.less';
import async from 'async';
import { saveRef } from '../src/utils';
import { saveRef, keyboardEvent } from '../src/utils';

const Simulate = TestUtils.Simulate;
const scryRenderedDOMComponentsWithClass = TestUtils.scryRenderedDOMComponentsWithClass;
Expand Down Expand Up @@ -895,4 +895,64 @@ describe('rc-trigger', function main() {
expect(domNode.className).to.be('target className-in-trigger-1 className-in-trigger-2');
});
});

describe('keyboard', () => {
let visible;
const onChange = (value) => {
visible = value;
};

it('esc key works if keyboard=true ', () => {
const trigger = ReactDOM.render(
<Trigger
keyboard
popup={<div />}
action={['click']}
popupAlign={placementAlignMap.right}
onPopupVisibleChange={onChange}
>
<div>click</div>
</Trigger>,
div
);
const domNode = ReactDOM.findDOMNode(trigger);

// click trigger and show popup
Simulate.click(domNode);
expect(visible).to.be(true);

// click escape
const event = keyboardEvent('keydown', { key: 'Escape', keyCode: 27 });
document.dispatchEvent(event);

// assert that popup hidden
expect(visible).to.be(false);
});

it('esc key doesn\'t work if keyboard=false ', () => {
const trigger = ReactDOM.render(
<Trigger
popup={<div />}
action={['click']}
popupAlign={placementAlignMap.right}
onPopupVisibleChange={onChange}
>
<div>click</div>
</Trigger>,
div
);
const domNode = ReactDOM.findDOMNode(trigger);

// click trigger and show popup
Simulate.click(domNode);
expect(visible).to.be(true);

// click escape
const event = keyboardEvent('keydown', { key: 'Escape', keyCode: 27 });
document.dispatchEvent(event);

// assert that popup still visible
expect(visible).to.be(true);
});
});
});