handleJavascriptDialogs (#1939) #2106
Conversation
There was a problem hiding this comment.
Woot!
Mind also adding a test to https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/gather/driver-test.js? There are some examples in there that stub events.
lighthouse-core/gather/driver.js
Outdated
| @@ -833,6 +833,16 @@ class Driver { | |||
| const promiseArr = urlPatterns.map(url => this.sendCommand('Network.addBlockedURL', {url})); | |||
| return Promise.all(promiseArr); | |||
| } | |||
|
|
|||
| handleJavaScriptDialogs() { | |||
There was a problem hiding this comment.
You'll need to put the enable in the promise chain:
handleJavaScriptDialogs() {
this.sendCommand('Page.enable').then(_ => {
this.on('Page.javascriptDialogOpening', _ => {
this.sendCommand('Page.handleJavaScriptDialog', {
accept: true,
promptText: 'Lighthouse prompt response',
});
});
});
}Can you also add a doc string to this method?
There was a problem hiding this comment.
My first reaction was that maybe we should use this.once instead of this.on. IOW, dismiss the first prompt we see and ignore the rest. Thinking towards the future when we have auditing after the page load (SPAs), this will dismiss all prompts the page throws up. That might not be something we want to do.
That said, it's a ways out and this on is probably safer for now.
There was a problem hiding this comment.
sounds good, will make the changes.
lighthouse-core/gather/driver.js
Outdated
| @@ -833,6 +833,16 @@ class Driver { | |||
| const promiseArr = urlPatterns.map(url => this.sendCommand('Network.addBlockedURL', {url})); | |||
| return Promise.all(promiseArr); | |||
| } | |||
|
|
|||
| handleJavaScriptDialogs() { | |||
There was a problem hiding this comment.
maybedismissJavaScriptDialogs
|
I still need to add content to driver-test.js and am not sure why Travis is unhappy about the method. Am I missing something? |
yeah, that's a little hard to figure out because the stack trace is so truncated. You'll need to add a mocked version of |
|
@googlebot shh bby. I added the method to the mock driver just to see if that resolves tests completely. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
lighthouse-core/gather/driver.js
Outdated
| dismissJavaScriptDialogs() { | ||
| return this.sendCommand('Page.enable').then(_ => { | ||
| this.once('Page.javascriptDialogOpening', _ => { | ||
| this.sendCommand('Page.handleJavaScriptDialog', { |
There was a problem hiding this comment.
if this rejects, this is intentionally unhandled (since we need to resolve as soon as 'Page.enable' is sent). Should we add a note that that's it's intended for future code readers?
|
@cedricbellet thanks! |
fixes #1939