906712 - implement tab modal dialog handling for desktop with Marionette (alert, confirm, prompt)

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P1)

Description

[David Burns :automatedtester]

we need to support handling of desktop modals like prompt/alert and confirmation via Marionette

Comment 1

[Al Tsai [:atsai]]

I plan to take it. Here's how I am going to implement.

Because there's no easy way to spoof alert or prompt function. I will rewrite some impelmentation of prompt service.

First, I'll add a promptService.js under <gecko>/testing/marionette/components which implements nsIprompt and nsIpromptService.
Second, there will be a marionette-alert.js under <gecko>/testing/marionette which implements the control of prompt/alert. For the automation, users only need to interact with the marionette-alert.js for prompt controlling.
Also, there will be unit tests for prompts as well.

Comment 2

[Jonathan Griffin (:jgriffin)]

Andreas is going to take this, and start by implementing the alert API's.  Al, this bug is for desktop Firefox only; the B2G modal dialog handling is handled in bug 779284; let us know if that isn't sufficient for your needs.

Comment 3

[Hallvord R. M. Steen [:hallvors]]

Enhancing summary a bit to make this bug easier to find, hope you don't mind Andreas! ;-)

Comment 4

[Hallvord R. M. Steen [:hallvors]]

Will this work cover showModalDialog() as well? Hopefully on the way out though..

Comment 5

[Andreas Tolfsen ❲:ato❳]

Not working on this anymore because of refactoring work.

Comment 6

[Hallvord R. M. Steen [:hallvors]]

I suppose this bug doesn't cover HTTP auth dialogs - is there a bug for handling those?

Comment 7

[David Burns :automatedtester]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #6)
> I suppose this bug doesn't cover HTTP auth dialogs - is there a bug for
> handling those?

that is covered under bug 887274 since they have slightly different mechanisms

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As we talked during the work week this bug is specifically for in-tab modal dialogs, like those which are raised with window.alert().

David, we might want to have a look at bug 887274 first, and maybe set the preference "prompts.tab_modal.enabled" to false for the time being until this bug has been fixed? With that we always get global modal dialogs.

Comment 9

[David Burns :automatedtester]

I am happy with the change

Comment 10

[Chris Manchester (limited bugmail, email directly)]

:ato, if you're not working on this do you see any reason I couldn't get it started (understanding that things might need significant updates once bug 1107706 lands)?

Comment 11

[Andreas Tolfsen ❲:ato❳]

(In reply to Chris Manchester [:chmanchester] from comment #10)
> :ato, if you're not working on this do you see any reason I couldn't get it
> started (understanding that things might need significant updates once bug
> 1107706 lands)?

No, please go ahead. (-:

Comment 12

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d85e90c37be0

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Chris, one question which comes into my mind when checking the try patch. What kind of tab modal dialogs do we have at all? In your patch I see a lot of code handling the default elements. But would Marionette also be able to handle a new type of dialog?

Comment 14

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Chris, one question which comes into my mind when checking the try patch.
> What kind of tab modal dialogs do we have at all? In your patch I see a lot
> of code handling the default elements. But would Marionette also be able to
> handle a new type of dialog?

If your concern is that I'm interacting with front-end ui classes directly and making those a dependency, I share that concern. But what do you mean by a new type of dialog? These are the default elements, but until they're changed for some reason (and while that's unfortunate, these tests will fail and we'll know we need to update when that happens) it seems like they're the only ones that will be shown as a result of alert/confirm/prompt. 

The platform code shares some logic between global notifications and tab-modals, so I'm going to look at the http auth dialogs next to see if we can generalize a bit in marionette.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Chris Manchester [:chmanchester] from comment #14) 
> If your concern is that I'm interacting with front-end ui classes directly
> and making those a dependency, I share that concern. But what do you mean by
> a new type of dialog? These are the default elements, but until they're
> changed for some reason (and while that's unfortunate, these tests will fail
> and we'll know we need to update when that happens) it seems like they're
> the only ones that will be shown as a result of alert/confirm/prompt. 

Those seem to be not the only ones. Please have a look at about:preferences and click on "Use Bookmark" for the home page setting. The sub dialog which comes up seem to be similar.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> (In reply to Chris Manchester [:chmanchester] from comment #14) 
> > If your concern is that I'm interacting with front-end ui classes directly
> > and making those a dependency, I share that concern. But what do you mean by
> > a new type of dialog? These are the default elements, but until they're
> > changed for some reason (and while that's unfortunate, these tests will fail
> > and we'll know we need to update when that happens) it seems like they're
> > the only ones that will be shown as a result of alert/confirm/prompt. 
> 
> Those seem to be not the only ones. Please have a look at about:preferences
> and click on "Use Bookmark" for the home page setting. The sub dialog which
> comes up seem to be similar.

That does have a similar role, but I don't see how it would be triggered by alert/confirm/prompt in a content script, and would require a different mechanism to interact with (it's a "groupbox" element, not a "tabmodalprompt" element). This seems like the sort of thing we'll need to add to the greenlight ui libraries.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Not sure where you get this information from, but its not a groupbox! I checked it myself now, and it is indeed a tab modal dialog:

http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.js#328

See `modal=yes` in the call to open(). So it fits in here perfectly.

With that I still think the logic as getting implemented here, should be able to cope with any kind of tabmodal dialog. But as you say, the core Marionette code might only have to handle the default dialog buttons which are in use by websites. On the other side the code has to give us (in the greenlight repository) the chance to handle everything else in it.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> Not sure where you get this information from, but its not a groupbox! I
> checked it myself now, and it is indeed a tab modal dialog:
> 
> http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> in-content/main.js#328
> 
> See `modal=yes` in the call to open(). So it fits in here perfectly.
> 
> With that I still think the logic as getting implemented here, should be
> able to cope with any kind of tabmodal dialog. But as you say, the core
> Marionette code might only have to handle the default dialog buttons which
> are in use by websites. On the other side the code has to give us (in the
> greenlight repository) the chance to handle everything else in it.

I got that from the browser toolbox inspector -- it looks like preferences/in-content/main.js puts a "dialog" element inside the "groupbox" element. What I'm saying is it doesn't share the ui structure of the tab modals produced by alert/confirm/prompt, so there isn't a "button0" to click on, for instance. However, the version you get with prompts.tab_modal.enabled turned off does, so that can share some of this code:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae875e1763d

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://906712/chmanchester

Comment 20

[Chris Manchester (limited bugmail, email directly)]

/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r fe2ebaecea93115a93029dba67e4e3087a49ae05

Comment 21

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/1993/#review1299

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +    this._dialogWindowRef = Cu.getWeakReference(null);

I don't think this is the right thing to do here, I will update.

Comment 22

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/1993/#review1559

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +  "getDialogText": MarionetteServerConnection.prototype.getDialogText,

I wonder if getTextFromDialog would be better as that is more symmetrical with the sendKeysToDialog

::: testing/marionette/client/marionette/marionette.py
(Diff revision 1)
> +    def dismiss_alert(self):

I would prefer if we were to emulate the API in http://selenium.googlecode.com/git/docs/api/py/webdriver/selenium.webdriver.common.alert.html#module-selenium.webdriver.common.alert

This forces the test writer to know that they are in an alert rather than blindly using alert API

::: testing/marionette/client/marionette/tests/unit/test_modal_dialogs.py
(Diff revision 1)
> +

It would be useful to see how this works with onBeforeUnload alerts

::: testing/marionette/client/marionette/tests/unit/test_modal_dialogs.py
(Diff revision 1)
> +

It would be useful to have tests that try do something like getAttribute/click/<something that is not alert related> when there is an alert

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +  if (appName != "B2G") {

this should probably be appName === 'Firefox' since we aren't sure that it works on fennec if we were to switch it on.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

(In reply to David Burns :automatedtester from comment #22)
> It would be useful to see how this works with onBeforeUnload alerts

I've just added a test for this, and it handles these as it does alert/confirm/prompt, but I'm not sure that's right. Should we be automatically dismissing or accepting these as soon as they come up, or giving tests the opportunity to dismiss or accept themselves?

Comment 24

[Chris Manchester (limited bugmail, email directly)]

/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r ab9066fa12e99cee6adec63ee389d04960216943

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

I pushed up the tests for onbeforeunload, didn't mean to unset the feedback flag!

Comment 26

[David Burns :automatedtester]

(In reply to Chris Manchester [:chmanchester] from comment #23)
> (In reply to David Burns :automatedtester from comment #22)
> > It would be useful to see how this works with onBeforeUnload alerts
> 
> I've just added a test for this, and it handles these as it does
> alert/confirm/prompt, but I'm not sure that's right. Should we be
> automatically dismissing or accepting these as soon as they come up, or
> giving tests the opportunity to dismiss or accept themselves?

We shouldn't automatically handle them, we might add a capability in the future to do that but for now we would expect the developer to handle that situation

Comment 27

[Chris Manchester (limited bugmail, email directly)]

/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r 4bf6b087ff8c64210646ed07ae3c04fd4a402c45

Comment 28

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

I didn't mean to unset the feedback flag that time either. I'll just add to mozreview.

Comment 29

[Chris Manchester (limited bugmail, email directly)]

/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r 4bf6b087ff8c64210646ed07ae3c04fd4a402c45

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Review ping

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Chris Manchester [:chmanchester] from comment #30)
> Review ping

People might miss who is meant here. So lets n-i Andreas.

Comment 32

[David Burns :automatedtester]

I will review

Comment 33

[David Burns :automatedtester]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

https://reviewboard.mozilla.org/r/1993/#review2795

Ship It!

Comment 34

[Chris Manchester (limited bugmail, email directly)]

Thanks for the review. The tests I wrote for this seem racy with e10s enabled, I'm going to see if I can work that out before landing.

Comment 35

[Chris Manchester (limited bugmail, email directly)]

The issue that cropped up in comment 34 was a problem with timing, but also the tests weren't cleaning up after themselves (an onbeforeunload handler from a prior test was impeding progress). 

I noticed running on a different OS for the case of global modal windows and with e10s enabled, the last test about the behavior when trying to process other commands when a modal is present isn't valid: .click() is processed by the underlying page when issued by marionette (although not when I try clicking it myself). I'm guessing marionette shouldn't process this click command when it doesn't correspond to something a user can do, so maybe this is something that needs to be dealt with in a follow up. 

I'm planning to run this through try again Monday and land more or less in its current form if all goes well there.

Comment 36

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f6f2806ad5

Comment 37

[Chris Manchester (limited bugmail, email directly)]

I'm waiting on try results here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7fa6138bba&exclusion_profile=false&exclusion_state=all
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b579833f4ee8

I'll check back in the morning.

Comment 38

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

/r/1995 - Bug 906712 - Tab modal dialog support for marionette.;r=automatedtester

Pull down this commit:

hg pull review -r 5562da9f13b7b83f915fcf4760dee98e12a6dae7

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

Try looks good. This needed somewhat substantial changes, an additional sanity check seems in order.

Comment 40

[David Burns :automatedtester]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

https://reviewboard.mozilla.org/r/1993/#review2901

::: testing/marionette/client/marionette/marionette.py
(Diff revision 4)
> +class Alert(object):

In webdriver we have switch_to_alert() call that is essentially something like

```
def switch_to_alert(self):
    return Alert(self)
```

I noticed in tests we call this directly e.g.
```
  Alert(self).<method>
```

the `switch_to_alert()` would be a lot more elegant from an API point of view. I am happy for this to be a follow up bug (even a good first bug) (sorry for not noticing this before)

Comment 41

[Chris Manchester (limited bugmail, email directly)]

(In reply to David Burns :automatedtester from comment #40)
> Comment on attachment 8544119 [details]
> MozReview Request: bz://906712/chmanchester
> 
> https://reviewboard.mozilla.org/r/1993/#review2901
> 
> ::: testing/marionette/client/marionette/marionette.py
> (Diff revision 4)
> > +class Alert(object):
> 
> In webdriver we have switch_to_alert() call that is essentially something
> like
> 
> ```
> def switch_to_alert(self):
>     return Alert(self)
> ```
> 
> I noticed in tests we call this directly e.g.
> ```
>   Alert(self).<method>
> ```
> 
> the `switch_to_alert()` would be a lot more elegant from an API point of
> view. I am happy for this to be a follow up bug (even a good first bug)
> (sorry for not noticing this before)

Oh, that's much better. I guess it would be cleaner to do it here so we don't get users of the other API creeping in.

Comment 42

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

/r/1995 - Bug 906712 - Tab modal dialog support for marionette.;r=automatedtester
/r/3697 - Bug 906712 - Add a switch_to_alert API to marionette for handling alerts;r=automatedtester

Pull down these commits:

hg pull review -r c371d195af2d4e5ca154ee5e1a06d5d8528bf319

Comment 43

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/3697/#review2915

Ship It!

Comment 44

[David Burns :automatedtester]

Comment on attachment 8544119 [details]
MozReview Request: bz://906712/chmanchester

https://reviewboard.mozilla.org/r/1993/#review2921

Ship It!

Comment 45

[Chris Manchester (limited bugmail, email directly)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f81edee561a2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a3852f540bb

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f81edee561a2
https://hg.mozilla.org/mozilla-central/rev/5a3852f540bb

Comment 48

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 906712 - Add a switch_to_alert API to marionette for handling alerts;r=automatedtester

Comment 49

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 906712 - Tab modal dialog support for marionette.;r=automatedtester

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving bugs for Marionette client due to component changes.