The WebDriver specification has a concept of a user prompt handler (http://w3c.github.io/webdriver/webdriver-spec.html#dfn-handle-any-user-prompts) that Marionette currently does not implement. We must implement it to be WebDriver compatible. The idea is that you can set a handler state that will automatically run an action tied to said state when a user prompt is present.

David, why do you think it is dependent on bug 1311041?

because its going to be simpler to implement when 1311041 is complete.

Tab modal prompts are not tight to window ids but to the window it lives in. And those we track perfectly because the listener registers itself and as such we register for the modal dialog observer notification. So bug 1311041 is not a hard blocker, and currently I don't see what could make it simpler.

(In reply to Henrik Skupin (:whimboo) from comment #4) > Tab modal prompts are not tight to window ids but to the window > it lives in. And those we track perfectly because the listener > registers itself and as such we register for the modal dialog > observer notification. So bug 1311041 is not a hard blocker, and > currently I don't see what could make it simpler. I guess I don’t really understand what you’re saying here, but Marionette currently keeps exactly one reference to the last modal dialogue to appear. This means we don’t track modals on a per-tab level, which we need to do because it is possible to switch to and interact with other tabs. In my opinion the current code we have for tracking user prompts is far too complicated. We could do away with keeping a reference to the modal, and also stop tracking global modal dialogues because they are not used when interacting with web content. The implementation of a specification conforming user prompt handler is not impossible without bug 1311041, but easier with it because each browsing context would have a unique abstraction, as opposed to the way curBrowser works right now.

Part of the conformance work we are doing this quarter, but not actively worked on.

As mentioned in the last meetings to get the various prompt behaviors implemented as the WebDriver specification refers to, we do not have to wait for the window tracking patch to be done. I already have a local patch which is mostly done for Marionette, but needs a bit of work for wdspec tests, so that we can run the user prompt tests for all commands.

While I wait on the dependency to be fixed I continue to work on this patch but won't bring it up for review. By doing so I noticed the following... Overall we have 44 different commands we have to check for user prompt handling. This is a huge amount given that for each of them we have 6 tests (default, accept, accept and notify, dismiss, dismiss and notify, ignore). Between each test we restart Firefox which adds a certain amount of time to each of the 244 tests. When I only run the user prompt tests for `execute_script` it takes about 1 minute with a debug build (opt builds will be faster). Extrapolating this to all the commands would mean we have a duration of 44 minutes only for those user prompt tests! I'm not satisfied with this at the moment given that we know there are some shutdown and startup hangs in Firefox once in a while. But also I'm worried of the overall duration of the Wd job. So if we want to see all the tests being executed we would have to start chunking the Wd spec job. I don't know what are usually the limits, and how long each chunk usually should be. Joel, do you have any recommendations? Andreas, and David, please also clarify on the above. I assume chunking comes for free from the wpt-runner, and we only have to specify the configuration in taskcluster.

I have to correct... Not all 44 commands have that check. We have to clearly exclude all the alert commands and a couple others.

(In reply to Henrik Skupin (:whimboo) from comment #10) > Overall we have 44 different commands we have to check for user > prompt handling. This is a huge amount given that for each of them > we have 6 tests (default, accept, accept and notify, dismiss, > dismiss and notify, ignore). Between each test we restart > Firefox which adds a certain amount of time to each of the 244 > tests. When I only run the user prompt tests for `execute_script` > it takes about 1 minute with a debug build (opt builds will be > faster). Extrapolating this to all the commands would mean we have > a duration of 44 minutes only for those user prompt tests! We need to run the user prompt tests for each command that invokes the user prompt handler. There is no other way of testing that each implementation is conforming. It’s unfortunate that WebDriver needs a new session each time we have to change the capabilities, but I feel this especially impacts Firefox negatively because the startup is so incredibly slow. We might be able to employ “a cheat” to not restart Firefox when deleting and creating the session like we do for the Mn tests. I haven’t thought carefully about this but it might be possible to instruct geckodriver not to shut down the Firefox process through a new extension capability. As I’ve mentioned in the past there is limited value in running wdspec tests against debug builds, but chunking seems to be the way forward to ensure the tests complete in a reasonable timeframe. > I assume chunking comes for free from the wpt-runner, and we only > have to specify the configuration in taskcluster. Chunking seems to be the way forward here to ensure the tests complete in a timely manner. I don’t know the details of the TC chunking setup, but in theory there is nothing in WPT or in wdspec that prevents chunking.

(In reply to Andreas Tolfsen 「:ato」 from comment #12) > We might be able to employ “a cheat” to not restart Firefox when > deleting and creating the session like we do for the Mn tests. I > haven’t thought carefully about this but it might be possible to > instruct geckodriver not to shut down the Firefox process through > a new extension capability. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1470120 to run an experiment on this. Who knows, we might make the tests a lot faster, if only for development purposes.

I am happy to wait for the bug 147010 results but for the most part I think its fine to see about chunking these as wptrunner should handle it

I say opt chunks should be 15-20 minutes, debug twice as long- more chunks!

Alright. So current Wd jobs take ~15min for opt, and ~30min for debug. It means that depending on the amount of extra time this patch adds, I will create chunks based on the total execution time. Thanks.

(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from comment #16) > Alright. So current Wd jobs take ~15min for opt, and ~30min for debug. It > means that depending on the amount of extra time this patch adds, I will > create chunks based on the total execution time. Thanks. I thought about all this and I think it will be better when I just push this work out to a follow-up bug. This bug is about to add the unhandledPromptBehavior capability, which I got working. So adding all the remaining user prompt tests goes really out of scope for this bug. Given that this is my last day before PTO I would like to maybe even see this landed today.

Comment on attachment 8990218 [details] Bug 1264259 - [marionette] Add support for unhandledPromptBehavior capability. https://reviewboard.mozilla.org/r/255244/#review262148 Code analysis found 8 defects in this patch: - 8 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: testing/marionette/driver.js:3240 (Diff revision 1) > + break; > + > + case UnhandledPromptBehavior.AcceptAndNotify: > + this.acceptDialog(); > + throw new UnexpectedAlertOpenError(undefined, alertText); > + break; Error: Unreachable code. [eslint: no-unreachable] ::: testing/marionette/driver.js:3249 (Diff revision 1) > + break; > + > + case UnhandledPromptBehavior.DismissAndNotify: > - this.dismissDialog(); > + this.dismissDialog(); > - throw e; > + throw new UnexpectedAlertOpenError(null, alertText); > + break; Error: Unreachable code. [eslint: no-unreachable] ::: testing/marionette/driver.js:3253 (Diff revision 1) > - throw e; > + throw new UnexpectedAlertOpenError(null, alertText); > + break; > + > + case UnhandledPromptBehavior.Ignore: > + throw new UnexpectedAlertOpenError(null, alertText); > + break; Error: Unreachable code. [eslint: no-unreachable] ::: testing/marionette/test/unit/test_capabilities.js:370 (Diff revision 1) > > run_next_test(); > }); > > +add_test(function test_UnhandledPromptBehavior() { > + equal(session.UnhandledPromptBehavior.Accept, "accept"); Error: 'session' is not defined. [eslint: no-undef] ::: testing/marionette/test/unit/test_capabilities.js:371 (Diff revision 1) > run_next_test(); > }); > > +add_test(function test_UnhandledPromptBehavior() { > + equal(session.UnhandledPromptBehavior.Accept, "accept"); > + equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify"); Error: 'session' is not defined. [eslint: no-undef] ::: testing/marionette/test/unit/test_capabilities.js:372 (Diff revision 1) > }); > > +add_test(function test_UnhandledPromptBehavior() { > + equal(session.UnhandledPromptBehavior.Accept, "accept"); > + equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify"); > + equal(session.UnhandledPromptBehavior.Dismiss, "dismiss"); Error: 'session' is not defined. [eslint: no-undef] ::: testing/marionette/test/unit/test_capabilities.js:373 (Diff revision 1) > > +add_test(function test_UnhandledPromptBehavior() { > + equal(session.UnhandledPromptBehavior.Accept, "accept"); > + equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify"); > + equal(session.UnhandledPromptBehavior.Dismiss, "dismiss"); > + equal(session.UnhandledPromptBehavior.DismissAndNotify, "dismiss and notify"); Error: 'session' is not defined. [eslint: no-undef] ::: testing/marionette/test/unit/test_capabilities.js:374 (Diff revision 1) > +add_test(function test_UnhandledPromptBehavior() { > + equal(session.UnhandledPromptBehavior.Accept, "accept"); > + equal(session.UnhandledPromptBehavior.AcceptAndNotify, "accept and notify"); > + equal(session.UnhandledPromptBehavior.Dismiss, "dismiss"); > + equal(session.UnhandledPromptBehavior.DismissAndNotify, "dismiss and notify"); > + equal(session.UnhandledPromptBehavior.Ignore, "ignore"); Error: 'session' is not defined. [eslint: no-undef]

Comment on attachment 8990218 [details] Bug 1264259 - [marionette] Add support for unhandledPromptBehavior capability. https://reviewboard.mozilla.org/r/255244/#review262190 This is pretty good, but I found a couple of omissions. I don’t think you will find any of my issues very surprising, so I would say you’re fine to fix those up and land this without any need for further review. ::: testing/marionette/capabilities.js:391 (Diff revision 2) > + ["acceptInsecureCerts", false], > ["browserName", appinfo.name], > ["browserVersion", appinfo.version], > ["platformName", getWebDriverPlatformName()], > ["platformVersion", Services.sysinfo.getProperty("version")], > ["pageLoadStrategy", PageLoadStrategy.Normal], > - ["acceptInsecureCerts", false], > - ["timeouts", new Timeouts()], > ["proxy", new Proxy()], > + ["timeouts", new Timeouts()], > + ["unhandledPromptBehavior", UnhandledPromptBehavior.DismissAndNotify], This probably does not matter functionally, but these capabilities are ordered as they are in the specification. This has the benefit that browserName, browserVersion, platformName, et al. will appear first in the matched capabilities list. I think this is worth preserving. ::: testing/marionette/capabilities.js:399 (Diff revision 2) > ["pageLoadStrategy", PageLoadStrategy.Normal], > - ["acceptInsecureCerts", false], > - ["timeouts", new Timeouts()], > ["proxy", new Proxy()], > + ["timeouts", new Timeouts()], > + ["unhandledPromptBehavior", UnhandledPromptBehavior.DismissAndNotify], This highlights a bug in the specification. It says that the default value should be null which it refers to as the default missing state, but it makes more sense for it to be set to the "dismiss and notify" state. Currently this line is in violation of the specification because it says the returned matched capabilities should have unhandledPromptBehavior set to null. I think the spec is wrong here as this behaviour deviates from how we treat all other capabilities, so it’s probably sufficient to file a spec bug on this. ::: testing/marionette/capabilities.js:500 (Diff revision 2) > + assert.string(v, > + pprint`Expected ${k} to be a string, got ${v}`); I note that pageLoadStrategy above also checks for null, which I think is probably wrong. I will follow up on that later. ::: testing/marionette/driver.js:3252 (Diff revision 2) > + > + case UnhandledPromptBehavior.Ignore: > + throw new UnexpectedAlertOpenError(); > + > + default: > + throw new UnknownError(`Unknown unhandledPromptBehavior "${behavior}"`); TypeError is probably more appropriate since you can’t find the desired variation on the enum. Also I’m not sure about the need for a string here since we validate the input. If there’s a programming error that leads us here it’s pretty clear what the problem is from the stacktrace. Messages are only really useful when we need to inform the user of something or provide additional debug information. ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:164 (Diff revision 2) > self.marionette.start_session(caps) > self.assertIn("timeouts", self.marionette.session_capabilities) > self.assertDictEqual(self.marionette.session_capabilities["timeouts"], timeouts) > self.assertDictEqual(self.marionette._send_message("getTimeouts"), timeouts) > + > + def test_unhandled_prompt_behavior(self): You will also want to test the default value. ::: testing/marionette/harness/marionette_harness/tests/unit/test_unhandled_prompt_behavior.py:23 (Diff revision 2) > + try: > + alert = self.marionette.switch_to_alert() > + alert.dismiss() > + > + Wait(self.marionette).until(lambda _: not self.alert_present) > + except: Be more specific about the exception so that we don’t ignore unwanted error scenarios. This should be a NoAlertPresentException IIRC. ::: testing/marionette/harness/marionette_harness/tests/unit/test_unhandled_prompt_behavior.py:98 (Diff revision 2) > + @parameterized("alert", "alert", None) > + @parameterized("confirm", "confirm", None) > + @parameterized("prompt", "prompt", None) > + def test_ignore(self, prompt_type, result): > + self.marionette.start_session({"unhandledPromptBehavior": "ignore"}) > + self.perform_user_prompt_check(prompt_type, "foo {}".format(prompt_type), result, > + expected_close=False) Also a test for the default behaviour.

Comment on attachment 8990218 [details] Bug 1264259 - [marionette] Add support for unhandledPromptBehavior capability. https://reviewboard.mozilla.org/r/255244/#review262190 > This probably does not matter functionally, but these capabilities > are ordered as they are in the specification. This has the benefit > that browserName, browserVersion, platformName, et al. will appear > first in the matched capabilities list. I think this is worth > preserving. I find it strange that an internal data structure has to care how toJSON() operates. But anyway I did not revert this change but actually sorted correctly as listed by the Spec. Even the former order was not correct. With the upcoming patch we are aligned again. > This highlights a bug in the specification. It says that the default > value should be null which it refers to as the default missing > state, but it makes more sense for it to be set to the "dismiss and > notify" state. > > Currently this line is in violation of the specification because > it says the returned matched capabilities should have > unhandledPromptBehavior set to null. > > I think the spec is wrong here as this behaviour deviates from how > we treat all other capabilities, so it’s probably sufficient to > file a spec bug on this. Good catch. I didn't read it that way, but what you say makes sense and that we should update the spec. I filed https://github.com/w3c/webdriver/issues/1276. > I note that pageLoadStrategy above also checks for null, which I > think is probably wrong. I will follow up on that later. Thank you. That is indeed a bug and should be fixed on our end. When you follow-up please add a wdspec test for it. > TypeError is probably more appropriate since you can’t find the > desired variation on the enum. > > Also I’m not sure about the need for a string here since we validate > the input. If there’s a programming error that leads us here it’s > pretty clear what the problem is from the stacktrace. Messages are > only really useful when we need to inform the user of something or > provide additional debug information. I will change it to `TypeError`. Regarding the second paragraph we add a message to all other TypeError instances too, so I will just follow that. Also the stacktrace will not necessary tell you which behavior is currently set. You could only assume it when going back in the log and check the response of the new session command. I will keep that to stay in sync with the rest of the file. > You will also want to test the default value. Ups. I forgot that! > Be more specific about the exception so that we don’t ignore unwanted > error scenarios. This should be a NoAlertPresentException IIRC. Yes, I will change. > Also a test for the default behaviour. The default is already covered with `test_default`. I will move the test to the bottom to make it more visible.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/44861ed17925 [marionette] Add support for unhandledPromptBehavior capability. r=ato