Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fix CTRL+Space handling while the CodeHintList is open, fixes #13481#13560

Merged
swmitra merged 2 commits intomasterfrom
petetnt/fix-ctrl-space
Jul 14, 2017
Merged

Fix CTRL+Space handling while the CodeHintList is open, fixes #13481#13560
swmitra merged 2 commits intomasterfrom
petetnt/fix-ctrl-space

Conversation

@petetnt
Copy link
Copy Markdown
Collaborator

@petetnt petetnt commented Jul 13, 2017

This PR fixes regression in filed as #13481 by instead of relying on
modifying original event we add a boolean flag telling _keydownHook the
event is actually faked and should be treated as such.

Signed-off-by: Pete Nykanen pete.a.nykanen@gmail.com

Signed-off-by: Pete Nykanen <pete.a.nykanen@gmail.com>
@petetnt petetnt requested a review from swmitra July 13, 2017 14:55
Copy link
Copy Markdown
Collaborator

@swmitra swmitra left a comment

Choose a reason for hiding this comment

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

Looks good to me barring the NITs and relevancy of the inner condition check 👍

Comment thread src/editor/CodeHintList.js Outdated
* @param {bool} isFakeCallUp - True if faked call up (for example calling CTRL+Space while hints are open)
*/
CodeHintList.prototype._keydownHook = function (event) {
CodeHintList.prototype._keydownHook = function (event, isFakeCallUp) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

NIT: isFakeKeyDown might me more suitable in this case?

Comment thread src/editor/CodeHintList.js Outdated

// (page) up, (page) down, enter and tab key are handled by the list
if (event.type === "keydown" && this.isHandlingKeyCode(event)) {
if (event.type === "keydown" && this.isHandlingKeyCode(event) || isFakeCallUp) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could be combined like this to make our intention more explicit ?

 if ((event.type === "keydown" || isFakeKeyDown) && this.isHandlingKeyCode(event))

Comment thread src/editor/CodeHintList.js Outdated
_rotateSelection.call(this, -1);
} else if (keyCode === KeyEvent.DOM_VK_DOWN ||
(event.ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE)) {
(event.ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE) || isFakeCallUp) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need to touch this condition? In case of a fake key down for Ctrl+Space (event.ctrlKey && keyCode === KeyEvent.DOM_VK_SPACE) will return true anyways.

Signed-off-by: Pete Nykanen <pete.a.nykanen@gmail.com>
@petetnt
Copy link
Copy Markdown
Collaborator Author

petetnt commented Jul 13, 2017

Addressed comments in 8bd7adb, thanks @swmitra

@swmitra swmitra merged commit 1842ac2 into master Jul 14, 2017
@swmitra swmitra deleted the petetnt/fix-ctrl-space branch July 14, 2017 07:32
@swmitra
Copy link
Copy Markdown
Collaborator

swmitra commented Jul 14, 2017

@petetnt Good work. This was a nagging issue and used to clutter the console completely with errors.
Thanks a lot for fixing this long pending problem 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants