From a3b65ad48157e5f50f056de1fc05e9c1a507e3c3 Mon Sep 17 00:00:00 2001 From: Birunthan Mohanathas Date: Tue, 6 Dec 2016 14:41:18 -0800 Subject: [PATCH 1/8] Add before-input-event event for webContents (fixes #7586) Embedding arbitrary web content is problematic when it comes to keyboard shortcuts because: * Web content can steal app shortcuts (see e.g. brave/browser-laptop#4408) * Blocked web content (e.g. a focused performing expensive computation) will also prevent app shortcuts from firing immediately The new before-input-event event can be used to overcome these issues by always handle certain keyboard events in the main process. Note that this requires electron/brightray#261 to compile. --- atom/browser/api/atom_api_web_contents.cc | 29 ++++++ atom/browser/api/atom_api_web_contents.h | 3 + docs/api/web-contents.md | 20 ++++ spec/api-web-contents-spec.js | 118 ++++++++++++++++++++++ 4 files changed, 170 insertions(+) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index f495ed80fa..a84cca7a00 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -70,6 +70,7 @@ #include "third_party/WebKit/public/web/WebFindOptions.h" #include "third_party/WebKit/public/web/WebInputEvent.h" #include "ui/display/screen.h" +#include "ui/events/keycodes/dom/keycode_converter.h" #if !defined(OS_MACOSX) #include "ui/aura/window.h" @@ -488,6 +489,34 @@ void WebContents::HandleKeyboardEvent( } } +bool WebContents::PreHandleKeyboardEvent( + content::WebContents* source, + const content::NativeWebKeyboardEvent& event, + bool* is_keyboard_shortcut) { + const char* type = + event.type == blink::WebInputEvent::Type::RawKeyDown ? "keyDown" : + event.type == blink::WebInputEvent::Type::KeyUp ? "keyUp" : + nullptr; + if (!type) { + // This should never happen. + assert(false); + return false; + } + + mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate()); + dict.Set("type", type); + dict.Set("key", ui::KeycodeConverter::DomKeyToKeyString(event.domKey)); + + using Modifiers = blink::WebInputEvent::Modifiers; + dict.Set("isAutoRepeat", (event.modifiers & Modifiers::IsAutoRepeat) != 0); + dict.Set("shift", (event.modifiers & Modifiers::ShiftKey) != 0); + dict.Set("control", (event.modifiers & Modifiers::ControlKey) != 0); + dict.Set("alt", (event.modifiers & Modifiers::AltKey) != 0); + dict.Set("meta", (event.modifiers & Modifiers::MetaKey) != 0); + + return Emit("before-input-event", dict); +} + void WebContents::EnterFullscreenModeForTab(content::WebContents* source, const GURL& origin) { auto permission_helper = diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index ff2823a282..68669e7f4b 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -244,6 +244,9 @@ class WebContents : public mate::TrackableObject, void HandleKeyboardEvent( content::WebContents* source, const content::NativeWebKeyboardEvent& event) override; + bool PreHandleKeyboardEvent(content::WebContents* source, + const content::NativeWebKeyboardEvent& event, + bool* is_keyboard_shortcut) override; void EnterFullscreenModeForTab(content::WebContents* source, const GURL& origin) override; void ExitFullscreenModeForTab(content::WebContents* source) override; diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 30fcca58d9..8f4ca02ea1 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -232,6 +232,26 @@ Emitted when a plugin process has crashed. Emitted when `webContents` is destroyed. +#### Event: 'before-input-event' + +Returns: + +* `event` Event +* `input` Object - Input properties + * `type` String - Either `keyUp` or `keyDown` + * `key` String - Equivalent to [KeyboardEvent.key](keyboardevent) + * `isAutoRepeat` Boolean - Equivalent to [KeyboardEvent.repeat](keyboardevent) + * `shift` Boolean - Equivalent to [KeyboardEvent.shiftKey](keyboardevent) + * `control` Boolean - Equivalent to [KeyboardEvent.controlKey](keyboardevent) + * `alt` Boolean - Equivalent to [KeyboardEvent.altKey](keyboardevent) + * `meta` Boolean - Equivalent to [KeyboardEvent.metaKey](keyboardevent) + +Emitted before dispatching the `keydown` and `keyup` events in the page. +Calling `event.preventDefault` will prevent the page `keydown`/`keyup` events +from being dispatched. + +[keyboardevent]: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent + #### Event: 'devtools-opened' Emitted when DevTools is opened. diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index d7a2ae50f3..b387536720 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -98,6 +98,124 @@ describe('webContents module', function () { }) }) + describe('will-navigate event', function () { + it('can be prevented', (done) => { + const targetURL = 'file://' + path.join(__dirname, 'fixtures', 'pages', 'location-change.html') + w.loadURL(targetURL) + w.webContents.once('did-finish-load', () => { + + w.webContents.on('will-navigate', (event, url) => { + assert.ok(url, targetURL) + }) + + setTimeout(done, 5000) + + w.webContents.on('did-navigate', (event, url) => { + assert.ok(url, targetURL) + }) + }) + }) + }) + + describe('before-input-event event', () => { + it('can prevent document keyboard events', (done) => { + w.loadURL('file://' + path.join(__dirname, 'fixtures', 'pages', 'key-events.html')) + w.webContents.once('did-finish-load', () => { + w.webContents.once('before-input-event', (event, input) => { + assert.equal(input.key, 'a') + event.preventDefault() + }) + + ipcMain.once('keydown', (event, key) => { + assert.equal(key, 'b') + done() + }) + + w.webContents.sendInputEvent({type: 'keyDown', keyCode: 'a'}) + w.webContents.sendInputEvent({type: 'keyDown', keyCode: 'b'}) + }) + }) + + it('has the correct properties', (done) => { + w.loadURL('file://' + path.join(__dirname, 'fixtures', 'pages', 'base-page.html')) + w.webContents.once('did-finish-load', () => { + const testBeforeInput = (opts) => { + return new Promise((resolve, reject) => { + w.webContents.once('before-input-event', (event, input) => { + assert.equal(input.type, opts.type) + assert.equal(input.key, opts.key) + assert.equal(input.isAutoRepeat, opts.isAutoRepeat) + assert.equal(input.shift, opts.shift) + assert.equal(input.control, opts.control) + assert.equal(input.alt, opts.alt) + assert.equal(input.meta, opts.meta) + resolve() + }) + + const modifiers = [] + if (opts.shift) modifiers.push('shift') + if (opts.control) modifiers.push('control') + if (opts.alt) modifiers.push('alt') + if (opts.meta) modifiers.push('meta') + if (opts.isAutoRepeat) modifiers.push('isAutoRepeat') + + w.webContents.sendInputEvent({ + type: opts.type, + keyCode: opts.keyCode, + modifiers: modifiers + }) + }) + } + + Promise.resolve().then(() => { + return testBeforeInput({ + type: 'keyDown', + key: 'A', + keyCode: 'a', + shift: true, + control: true, + alt: true, + meta: true, + isAutoRepeat: true + }) + }).then(() => { + return testBeforeInput({ + type: 'keyUp', + key: '.', + keyCode: '.', + shift: false, + control: true, + alt: true, + meta: false, + isAutoRepeat: false + }) + }).then(() => { + return testBeforeInput({ + type: 'keyUp', + key: '!', + keyCode: '1', + shift: true, + control: false, + alt: false, + meta: true, + isAutoRepeat: false + }) + }).then(() => { + return testBeforeInput({ + type: 'keyUp', + key: 'Tab', + keyCode: 'Tab', + shift: false, + control: true, + alt: false, + meta: false, + isAutoRepeat: true + }) + }).then(done).catch(done) + }) + }) + }) + describe('sendInputEvent(event)', function () { beforeEach(function (done) { w.loadURL('file://' + path.join(__dirname, 'fixtures', 'pages', 'key-events.html')) From 25ac23ab17cec4ad600a5ee18010185ce239d872 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 14:51:32 -0800 Subject: [PATCH 2/8] Upgrade brightray --- vendor/brightray | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/brightray b/vendor/brightray index b8c8a31e92..365eacb67d 160000 --- a/vendor/brightray +++ b/vendor/brightray @@ -1 +1 @@ -Subproject commit b8c8a31e9253406ef410f0d253bf1507448c222d +Subproject commit 365eacb67d86581670519d09c3ce266f5c47313e From 5ae80d541b8ece2a5610d881c008b10b5c506f3f Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 15:00:36 -0800 Subject: [PATCH 3/8] Remove will-navigate spec --- spec/api-web-contents-spec.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index b387536720..ab202c615e 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -98,25 +98,6 @@ describe('webContents module', function () { }) }) - describe('will-navigate event', function () { - it('can be prevented', (done) => { - const targetURL = 'file://' + path.join(__dirname, 'fixtures', 'pages', 'location-change.html') - w.loadURL(targetURL) - w.webContents.once('did-finish-load', () => { - - w.webContents.on('will-navigate', (event, url) => { - assert.ok(url, targetURL) - }) - - setTimeout(done, 5000) - - w.webContents.on('did-navigate', (event, url) => { - assert.ok(url, targetURL) - }) - }) - }) - }) - describe('before-input-event event', () => { it('can prevent document keyboard events', (done) => { w.loadURL('file://' + path.join(__dirname, 'fixtures', 'pages', 'key-events.html')) From 7a5ec96d53d7b4b82677d622f1160850017fa18b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 15:19:15 -0800 Subject: [PATCH 4/8] Prevent input event from main process --- spec/api-web-contents-spec.js | 8 ++------ spec/static/main.js | 7 +++++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index ab202c615e..fc486be46d 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -4,7 +4,7 @@ const assert = require('assert') const path = require('path') const {closeWindow} = require('./window-helpers') -const {remote} = require('electron') +const {ipcRenderer, remote} = require('electron') const {BrowserWindow, webContents, ipcMain} = remote const isCi = remote.getGlobal('isCi') @@ -102,16 +102,12 @@ describe('webContents module', function () { it('can prevent document keyboard events', (done) => { w.loadURL('file://' + path.join(__dirname, 'fixtures', 'pages', 'key-events.html')) w.webContents.once('did-finish-load', () => { - w.webContents.once('before-input-event', (event, input) => { - assert.equal(input.key, 'a') - event.preventDefault() - }) - ipcMain.once('keydown', (event, key) => { assert.equal(key, 'b') done() }) + ipcRenderer.send('prevent-next-input-event', 'a', w.webContents.id) w.webContents.sendInputEvent({type: 'keyDown', keyCode: 'a'}) w.webContents.sendInputEvent({type: 'keyDown', keyCode: 'b'}) }) diff --git a/spec/static/main.js b/spec/static/main.js index 1b512e1b0d..de599988df 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -8,6 +8,7 @@ const ipcMain = electron.ipcMain const dialog = electron.dialog const BrowserWindow = electron.BrowserWindow const protocol = electron.protocol +const webContents = electron.webContents const v8 = require('v8') const Coverage = require('electabul').Coverage @@ -184,6 +185,12 @@ app.on('ready', function () { event.returnValue = 'done' }) + ipcMain.on('prevent-next-input-event', (event, key, id) => { + webContents.fromId(id).once('before-input-event', (event, input) => { + if (key === input.key) event.preventDefault() + }) + }) + ipcMain.on('executeJavaScript', function (event, code, hasCallback) { if (hasCallback) { window.webContents.executeJavaScript(code, (result) => { From 3237c6751a47c7e96229d13f30b9c05ce42551ad Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 15:31:02 -0800 Subject: [PATCH 5/8] Use DCHECK instead of assert --- atom/browser/api/atom_api_web_contents.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index a84cca7a00..c306ba695f 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -497,9 +497,8 @@ bool WebContents::PreHandleKeyboardEvent( event.type == blink::WebInputEvent::Type::RawKeyDown ? "keyDown" : event.type == blink::WebInputEvent::Type::KeyUp ? "keyUp" : nullptr; + DCHECK(type); if (!type) { - // This should never happen. - assert(false); return false; } From 7842040d9d4ef7488518f1b51fc767836c4b9e70 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 15:52:37 -0800 Subject: [PATCH 6/8] Add ToV8 converter for NativeWebKeyboardEvent --- atom/browser/api/atom_api_web_contents.cc | 24 ++++--------------- .../native_mate_converters/blink_converter.cc | 21 ++++++++++++++++ .../native_mate_converters/blink_converter.h | 2 ++ 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index c306ba695f..2ee2879124 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -493,27 +493,11 @@ bool WebContents::PreHandleKeyboardEvent( content::WebContents* source, const content::NativeWebKeyboardEvent& event, bool* is_keyboard_shortcut) { - const char* type = - event.type == blink::WebInputEvent::Type::RawKeyDown ? "keyDown" : - event.type == blink::WebInputEvent::Type::KeyUp ? "keyUp" : - nullptr; - DCHECK(type); - if (!type) { + if (event.type == blink::WebInputEvent::Type::RawKeyDown + || event.type == blink::WebInputEvent::Type::KeyUp) + return Emit("before-input-event", event); + else return false; - } - - mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate()); - dict.Set("type", type); - dict.Set("key", ui::KeycodeConverter::DomKeyToKeyString(event.domKey)); - - using Modifiers = blink::WebInputEvent::Modifiers; - dict.Set("isAutoRepeat", (event.modifiers & Modifiers::IsAutoRepeat) != 0); - dict.Set("shift", (event.modifiers & Modifiers::ShiftKey) != 0); - dict.Set("control", (event.modifiers & Modifiers::ControlKey) != 0); - dict.Set("alt", (event.modifiers & Modifiers::AltKey) != 0); - dict.Set("meta", (event.modifiers & Modifiers::MetaKey) != 0); - - return Emit("before-input-event", dict); } void WebContents::EnterFullscreenModeForTab(content::WebContents* source, diff --git a/atom/common/native_mate_converters/blink_converter.cc b/atom/common/native_mate_converters/blink_converter.cc index 07971e78dd..950b4f5f0a 100644 --- a/atom/common/native_mate_converters/blink_converter.cc +++ b/atom/common/native_mate_converters/blink_converter.cc @@ -18,6 +18,7 @@ #include "third_party/WebKit/public/web/WebFindOptions.h" #include "third_party/WebKit/public/web/WebInputEvent.h" #include "ui/base/clipboard/clipboard.h" +#include "ui/events/keycodes/dom/keycode_converter.h" #include "ui/events/keycodes/keyboard_code_conversion.h" namespace { @@ -215,6 +216,26 @@ bool Converter::FromV8( return true; } +v8::Local Converter::ToV8( + v8::Isolate* isolate, const content::NativeWebKeyboardEvent& in) { + mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate); + + if (in.type == blink::WebInputEvent::Type::RawKeyDown) + dict.Set("type", "keyDown"); + else if (in.type == blink::WebInputEvent::Type::KeyUp) + dict.Set("type", "keyUp"); + dict.Set("key", ui::KeycodeConverter::DomKeyToKeyString(in.domKey)); + + using Modifiers = blink::WebInputEvent::Modifiers; + dict.Set("isAutoRepeat", (in.modifiers & Modifiers::IsAutoRepeat) != 0); + dict.Set("shift", (in.modifiers & Modifiers::ShiftKey) != 0); + dict.Set("control", (in.modifiers & Modifiers::ControlKey) != 0); + dict.Set("alt", (in.modifiers & Modifiers::AltKey) != 0); + dict.Set("meta", (in.modifiers & Modifiers::MetaKey) != 0); + + return dict.GetHandle(); +} + bool Converter::FromV8( v8::Isolate* isolate, v8::Local val, blink::WebMouseEvent* out) { mate::Dictionary dict; diff --git a/atom/common/native_mate_converters/blink_converter.h b/atom/common/native_mate_converters/blink_converter.h index 78275ab62e..34156f313e 100644 --- a/atom/common/native_mate_converters/blink_converter.h +++ b/atom/common/native_mate_converters/blink_converter.h @@ -45,6 +45,8 @@ template<> struct Converter { static bool FromV8(v8::Isolate* isolate, v8::Local val, content::NativeWebKeyboardEvent* out); + static v8::Local ToV8(v8::Isolate* isolate, + const content::NativeWebKeyboardEvent& in); }; template<> From 88dac36c914cd3a99a970e51fe963ed9c5a14caa Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 15:58:03 -0800 Subject: [PATCH 7/8] Move footer link to bottom --- docs/api/web-contents.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 8f4ca02ea1..36d7f20cc4 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -250,8 +250,6 @@ Emitted before dispatching the `keydown` and `keyup` events in the page. Calling `event.preventDefault` will prevent the page `keydown`/`keyup` events from being dispatched. -[keyboardevent]: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent - #### Event: 'devtools-opened' Emitted when DevTools is opened. @@ -1237,3 +1235,5 @@ when the DevTools has been closed. #### `contents.debugger` A [Debugger](debugger.md) instance for this webContents. + +[keyboardevent]: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent From 590bde5e1474d6679dbf2e163d1644af1a530cfe Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Tue, 13 Dec 2016 16:00:37 -0800 Subject: [PATCH 8/8] Remove unused include --- atom/browser/api/atom_api_web_contents.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 2ee2879124..09c220c6e1 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -70,7 +70,6 @@ #include "third_party/WebKit/public/web/WebFindOptions.h" #include "third_party/WebKit/public/web/WebInputEvent.h" #include "ui/display/screen.h" -#include "ui/events/keycodes/dom/keycode_converter.h" #if !defined(OS_MACOSX) #include "ui/aura/window.h"