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

RESOLVED FIXED in Firefox 38

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: automatedtester, Assigned: chmanchester)

Tracking

(Blocks 1 bug, {pi-marionette-client, pi-marionette-server, pi-marionette-spec})

Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [marionette=1.0])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

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

Comment 1

6 years ago
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.
Assignee: nobody → ato
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.
Enhancing summary a bit to make this bug easier to find, hope you don't mind Andreas! ;-)
Summary: implement modal dialog handling for desktop with Marionette → implement modal dialog handling for desktop with Marionette (alert, confirm, prompt)
Will this work cover showModalDialog() as well? Hopefully on the way out though..
Reporter

Updated

5 years ago
Whiteboard: [spec]
Reporter

Updated

5 years ago
Priority: -- → P1
Reporter

Updated

5 years ago
Depends on: 1073539
Not working on this anymore because of refactoring work.
Assignee: ato → administration
I suppose this bug doesn't cover HTTP auth dialogs - is there a bug for handling those?
Reporter

Comment 7

5 years ago
(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
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.
Flags: needinfo?(dburns)
Summary: implement modal dialog handling for desktop with Marionette (alert, confirm, prompt) → implement tab modal dialog handling for desktop with Marionette (alert, confirm, prompt)
No longer blocks: 887274
Depends on: 887274
Reporter

Comment 9

5 years ago
I am happy with the change
Flags: needinfo?(dburns)
Reporter

Updated

5 years ago
Whiteboard: [spec] → [spec][marionette=1.0]
Reporter

Updated

5 years ago
Whiteboard: [spec][marionette=1.0] → [marionette=1.0]
: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)?
Flags: needinfo?(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. (-:
Flags: needinfo?(ato)
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?
Assignee: administration → cmanchester
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(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.
(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.
(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.
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.
(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
/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r fe2ebaecea93115a93029dba67e4e3087a49ae05
Assignee

Updated

4 years ago
Attachment #8544119 - Flags: feedback?(ato)
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.
Reporter

Comment 22

4 years ago
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.
(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?
Flags: needinfo?(dburns)
Assignee

Updated

4 years ago
Attachment #8544119 - Flags: feedback?(ato)
/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r ab9066fa12e99cee6adec63ee389d04960216943
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!
Attachment #8544119 - Flags: feedback?(ato)
Reporter

Comment 26

4 years ago
(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
Flags: needinfo?(dburns)
Assignee

Updated

4 years ago
Attachment #8544119 - Flags: feedback?(ato)
/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r 4bf6b087ff8c64210646ed07ae3c04fd4a402c45
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.
Assignee

Updated

4 years ago
Attachment #8544119 - Flags: review?(ato)
/r/1995 - Bug 906712 - Tab modal dialog support for marionette.

Pull down this commit:

hg pull review -r 4bf6b087ff8c64210646ed07ae3c04fd4a402c45
Review ping
(In reply to Chris Manchester [:chmanchester] from comment #30)
> Review ping

People might miss who is meant here. So lets n-i Andreas.
Flags: needinfo?(ato)
Reporter

Comment 32

4 years ago
I will review
Flags: needinfo?(ato)
Reporter

Updated

4 years ago
Attachment #8544119 - Flags: review+
Reporter

Comment 33

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

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

Ship It!
Reporter

Updated

4 years ago
Attachment #8544119 - Flags: review?(ato)
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.
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 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
Attachment #8544119 - Flags: review+
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.
Attachment #8544119 - Flags: review?(dburns)
Reporter

Updated

4 years ago
Attachment #8544119 - Flags: review?(dburns)
Reporter

Comment 40

4 years ago
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)
(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 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
Attachment #8544119 - Flags: review?(dburns)
Reporter

Updated

4 years ago
Attachment #8544119 - Flags: review?(dburns) → review+
Reporter

Comment 44

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

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

Ship It!
Reporter

Updated

4 years ago
Blocks: webdriver
https://hg.mozilla.org/mozilla-central/rev/f81edee561a2
https://hg.mozilla.org/mozilla-central/rev/5a3852f540bb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee

Updated

4 years ago
No longer depends on: 1107706
Attachment #8544119 - Attachment is obsolete: true
Attachment #8618017 - Flags: review+
Attachment #8618018 - Flags: review+
Assignee

Updated

3 years ago
Duplicate of this bug: 887274
You need to log in before you can comment on or make changes to this bug.