-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Issue #12859 Keyboard modifiers support for simulateKeyEvent #12863
Conversation
Sorry about being so slow with this, been swamped with work as of late. I'll review this on Sunday / Monday. Again, apologies for taking so long! |
No worries! Thanks for putting aside time to review this.
… On 25 Nov 2016, at 08:47, Pete Nykänen ***@***.***> wrote:
Sorry about being so slow with this, been swamped with work as of late. I'll review this on Sunday / Monday.
Again, apologies for taking so long!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
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.
Some review comments, mostly looks good to me! Needs an unit test though 👍
function simulateKeyEvent(key, event, element, options) { | ||
var doc = element.ownerDocument; | ||
|
||
if(typeof options === 'undefined') { |
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.
We can use Object.assign
to make this a bit easier:
var defaultOptions = {
options.view = doc.defaultView;
options.bubbles = true;
options.cancelable = true;
options.keyIdentifier = key;
};
var newOptions = Object.assign({}, options, defaultOptions});
var oEvent = new KeyboardEvent(event, newOptions);
|
||
if (event !== "keydown" && event !== "keyup" && event !== "keypress") { | ||
console.log("SpecRunnerUtils.simulateKeyEvent() - unsupported keyevent: " + event); | ||
return; | ||
} | ||
} |
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.
Nit: trailing whitespace
Made the requested changes, and provided a unit test. Had to pop it in a new file, |
cancelable: true, | ||
keyIdentifier: key | ||
}; | ||
var newOptions = Object.assign({}, options, defaultOptions); |
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.
It was brought up in another PR that using Object.assign
actually brokes the Linux version due to using an age old CEF which doesn't support it 😢
If you could change this to the previous format then this one is ready to be merged
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.
No problem at all, reverted to the old version. 👍
I'll try and test this out for the final time today and then merge it in, LGTM so far 🥇 |
Great, thanks!
… On 22 Dec 2016, at 08:40, Pete Nykänen ***@***.***> wrote:
I'll try and test this out for the final time today and then merge it in, LGTM so far 🥇
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@@ -995,15 +995,28 @@ define(function (require, exports, module) { | |||
* Simulate key event. Found this code here: |
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.
We can remove these comments now as the implementation is different
Okay, one more nitpick and then it's ready to go 👍 Great work @haslam22! |
Woops, forgot to update that part. Should be all good now. |
Thanks for this great contribution @haslam22! |
Added support for sending additional arguments to simulateKeyEvent, e.g. modifiers such as
ctrlKey: true
or any other value supported byKeyboardEventInit
.