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.

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

Add before-input-event event for webContents (fixes #7586) #8143

Merged
merged 8 commits into from Dec 14, 2016

Conversation

Projects
None yet
2 participants
@poiru
Member

poiru commented Dec 5, 2016

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-archive/brightray#261 to compile.

Fixes #7586

assert.ok(beforeInputEventReceived)
ipcMain.removeListener('keydown', onEvent)
done(error)
}, 500)

This comment has been minimized.

@kevinsawicki

kevinsawicki Dec 6, 2016

Contributor

Timeouts can be pretty flaky on CI, maybe it could just send two input events instead, prevent the first and then call done() when the second is delivered in the page?

This comment has been minimized.

@poiru

poiru Dec 6, 2016

Member

@kevinsawicki Done, but I just realized that event.preventDefault does not work in the render process. For example

app.on('web-contents-created', (event, wc) => {
  wc.on('before-input-event', (event) => { // or will-navigate etc.
    event.preventDefault()
  })
})

works fine in the main process, but

remote.app.on('web-contents-created', (event, wc) => {
  wc.on('before-input-event', (event) => { // or will-navigate etc.
    event.preventDefault()
  })
})

in the render process will not work.

The spec tests use remote heavily so the test in this PR will always fail. Is this a known issue? Do you have any suggestions about how to work around this?

This comment has been minimized.

@kevinsawicki

kevinsawicki Dec 6, 2016

Contributor

Yeah, preventDefault does not work for events originating from the main process since the main process dispatches them to the listening render processes asynchronously via the remote module and at that point it is too late to prevent them.

You could work around them using a technique like

ipcMain.on('set-download-option', function (event, needCancel, preventDefault) {
window.webContents.session.once('will-download', function (e, item) {
if (preventDefault) {
e.preventDefault()

Add a specific ipcMain channel that sets things up in spec/main.js and then you tell it to listen to an event, optionally prevent default, and callback afterwards and verify that from the render process.

@kevinsawicki kevinsawicki self-assigned this Dec 13, 2016

poiru and others added some commits Dec 6, 2016

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 <webview> 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-archive/brightray#261 to compile.
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Dec 13, 2016

@poiru I update the spec, event.preventDefault() needed to be called from the main process instead of the render process.

Also added a ToV8 converter for content::NativeWebKeyboardEvent& event to so that webContents can just emit it directly in C++.

Also bumped brightray and resolved the conflict via rebasing.

kevinsawicki added some commits Dec 13, 2016

@kevinsawicki kevinsawicki merged commit 25feb92 into electron:master Dec 14, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Dec 14, 2016

Thanks for this @poiru 👍 🚢

@poiru

This comment has been minimized.

Member

poiru commented Dec 14, 2016

@kevinsawicki Thanks for taking the time to fix and land this! 🙌

@poiru poiru deleted the poiru:webcontents-before-input-event branch Dec 14, 2016

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