486407 - Preferences dialog is modal on Windows which stops execution of Mozmill tests

Status: VERIFIED FIXED

(Toolkit :: Preferences, defect, P1)

Description

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

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090331 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090331044608 and MozMill 1.1

With MozMill on Windows it is not possible to run any test which switches the panes inside the preferences dialog. You always run into the following error:

Error: Expression "id("BrowserPreferences")" returned null. Anonymous == false
Source File: file:///C:/Dokumente%20und%20Einstellungen/henrik/Anwendungsdaten/Mozilla/Firefox/Profiles/l0ucug4s.default/extensions/[e-mail]/resource/modules/frame.js
Line: 408

Comment 1

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

Attachment: MozMill test

Comment 2

[Mikeal Rogers [:mikeal] (mrogers@)]

I don't have a windows environment in my hotel room right now so I can't look at this right away but I do have one question.

If you inspect the element you're trying to click on in Windows does it generate a different expression?

It's possible that Windows is creating different elements or something, we may need a platform specific if statement there.

Comment 3

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

Mikeal, I know you will not believe what I say in the next sentence but: "This test was created with the recorder under Windows". So no inconsistency should exists at all. We are just failing in running this code.

Comment 4

[Mikeal Rogers [:mikeal] (mrogers@)]

oh wow, ok, this is bad.

If you inspect the element, does the validation show true?

-Mikeal

Comment 5

[Mikeal Rogers [:mikeal] (mrogers@)]

also, try sticking a static sleep before the call that fails, I know there are render speed issues with the preferences window in general.

Comment 6

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

It doesn't make a difference when having one more sleep call here. It fails in the first click method. And yes, validation is true.

Comment 7

[Adam Christian]

I'll look into this tonight.

Comment 8

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

No way to create tests for the preferences controller on Windows. Marking as blocker.

Comment 9

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

Ok, the main problem with this bug is that the preferences dialog on Windows is modal in comparance to OS X and Linux. I tried to debug a Mozmill test and noticed that Venkman doesn't stop the execution of the test. It waits in background and the breakpoint is activated when I manually closed the preferences dialog.

So it looks like that we have to implement a similar way like we are handling modal dialogs. Clint, how smart is your code that we can also detect modal windows and not only modal dialogs? We could also use a callback function with the controller as parameter. IMO that would be a nice API enhancement for the prefsAPI?

Would be great if TestDev could come up with a solution. Until this time all tests which make use of the preferences dialog have to wait. :/

One strange thing I want to throw in is when the preferences window is already open before you start the test everything works. But starting with the browser window only and using getPreferencesController() will make the test fail.

Comment 10

[Adam Christian]

It looks like when the modal dialog is initialized it actually stops all javascript threads that were running before the initialization, but if you run the test after the dialog has been opened by the user it executes correctly as it's in a JS thread post initialization. This looks like it's going to require some Clint modal dialog APIage.

Comment 11

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

Updating bug references and summary to reflect the current investigations.

Comment 12

[cmtalbert]

Attachment: The method for using the modal dialog API that will work for the prefernece dialog

You can still use the standard modal dialog API without any changes to get the preference dialog to work on windows.  I've updated your test so that it will work properly.

The big difference is that you need to stall in your handler function because the preference dialog takes so long to load.  You might be able to use waitFor* functions, but I remember Mikeal had quite a few problems doing that when creating getPreferencesController so the best way to get around this might be a straight sleep like I did in the example.  I'm going to mark this WFM.  Let me know if it works for you.

Comment 13

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

Sorry, but I cannot get this to work. It always fails to me. I'll attach a patch which could be applied to the mozmill-test repo. It would be great if you can modify it so it will work for you.

Further the handling will be completely different between Windows and Linux/OS X. We should not re-invent the wheel for each test. The decision which way has to be used should be punted into the prefsAPI so we only have to call the following:

PrefsAPI.handlePreferences(handleDialog);

which will call "handleDialog = function(controller)" transparently.

Clint, will testdev has time to enhance the prefsAPI so it will also handle this dialog?

Comment 14

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

Attachment: Example test with modal dialog API

So what is wrong that it doesn't work with latest trunk?

Comment 15

[cmtalbert]

(In reply to comment #14)
> Created an attachment (id=382119) [details]
> Example test with modal dialog API
> 
> So what is wrong that it doesn't work with latest trunk?

The problem with this is that you forgot the line:
module.modalDialogAPI = collector.getModule('ModalDialogAPI');

in your setupModule function.  Without that, the subsystem won't understand what modalDialogAPI is.

I don't see that the handling will have to be that different between OS's.  I think test writers have two options:
1. Split windows out and use the modalDialog Callbacks for windows only while using the mozmill.getPreferencesController for linux and os x.
2. Just use the modalDialog API for the preferences dialog across the board.  It will still work even if the preferences dialog is not modal on that OS.

I kind of like option 2 myself.

I explored this a little bit, and our module system isn't configured to allow for modules to be dependent on other modules.  So you can't make the preferences module dependent on the modalDiaogAPI, for example.  And even if I were to find a way around it, it's not clear to me that the flow of control would seamlessly fall through back to the testing function after the modal dialog was handled.

I'm going to move this back as WFM, because I don't think that this is a good generalization to make.  I'd say let's just use the modalDialog work around for all OS's and simplify the code that way.

Comment 16

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

(In reply to comment #15)
> The problem with this is that you forgot the line:
> module.modalDialogAPI = collector.getModule('ModalDialogAPI');
> 
> in your setupModule function.  Without that, the subsystem won't understand
> what modalDialogAPI is.

No, this is intended and not needed anymore. The collector is doing this automatically depending on the specified modules in the MODULE_REQUIRES array. Grab the latest trunk to check this. Otherwise I would get errors in the Error Console which I don't see.

> 1. Split windows out and use the modalDialog Callbacks for windows only while
> using the mozmill.getPreferencesController for linux and os x.

That would require test authors to add the identical code in nearly all the tests which work on the preferences dialog. I don't think that this is the way we should go. It doesn't make it easier.

> 2. Just use the modalDialog API for the preferences dialog across the board. 
> It will still work even if the preferences dialog is not modal on that OS.

No, that doesn't work because the test function is not blocked on other OSes as Windows. That means tests which should happen after the preference dialog tests will be run immediately and will fail.

> I explored this a little bit, and our module system isn't configured to allow
> for modules to be dependent on other modules.  So you can't make the
> preferences module dependent on the modalDiaogAPI, for example.  And even if I

That works and I already do it that way in my private browsing test. No idea how it works now after the changes to the collector. I'll have to test it.

> I'm going to move this back as WFM, because I don't think that this is a good
> generalization to make.  I'd say let's just use the modalDialog work around for
> all OS's and simplify the code that way.

It feels like ping-pong. :) Please let us discuss and make it work for everyone before closing this bug again. It will add too much noise to depending bugs.

Comment 17

[cmtalbert]

Attachment: Patch

Ok, here are the changes and a test.

This patch provides an API on the preferences helper class called getPreferencesDialog(handler).  The API takes a callback handler function that will then exercise the preferences dialog once it is opened. On Mac and Linux, the Preferences dialog is not modal, and therefore, this function merely uses a pass-through mechanism rather than incurring the overhead for the modal dialog handling.  On windows, it uses modal dialog handling.

= Assumptions =
The code makes the following assumptions
* The user will close the preferences dialog when they are finished with it.  If this does not happen the test will hang on windows.  The only way around this would be to have the getPreferencesDialog function close the dialog for the test writer, but then you get into the area where that might conflict with the test writer's intentions.  If the test writer wants fine grained control over the preference dialog's state, they should still use the modalDialogAPI and getPreferencesController directly.

* That the modalDialogAPI and the PreferencesAPI live in the same directory (see RELATIVE_ROOT declaration in PreferencesAPI)

= Testing =
The modal dialog API was also updated to be able to look for modal windows in the docshell list it queries rather than simple commondialog.xul elements.  I tested this on:
Mac - using the private browsing tests (modal dialog on mac to test modal dialog changes)
Mac - using the attached preference dialog to test the non-modal pref dialog path
Windows - using the attached preference dialog test to test the modal pref dialog functionality.

Comment 18

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

Comment on attachment 382403 [details] [diff] [review]
Patch

Clint, due to all the code is located in our Mozmill test repository I feel free take the review.

Comment 19

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

Comment on attachment 382403 [details] [diff] [review]
Patch

That looks great Clint. I did a test-run with my private browsing tests and they work too. Looks like we can get it in. But I have some small nits for the current version. See below...

>diff --git a/firefox/testTechnicalTools/testPreferencesDialog.js b/firefox/testTechnicalTools/testPreferencesDialog.js

Can you remove this test? We do not need it. We have already a couple of those in the queue. It would also be the wrong folder.

>diff --git a/shared-modules/testPrefsAPI.js b/shared-modules/testPrefsAPI.js

> /**
>  * Preferences helper object for accessing nsIPrefBranch.
>  *
>  * @class Preferences
>  */
> var preferences = {
>+  getPreferencesDialog: function gpd(handler) {
>+    if (mozmill.isWindows) {
>+      // Pref dialog is modal on windows, set up our handler
>+      var md = collector.getModule('ModalDialogAPI');
>+      var prefModal = new md.modalDialog(handler);
>+      prefModal.start();
>+    }
>+    // Launch the dialog
>+    var prefctrl = new mozmill.getPreferencesController();
>+
>+    // If the dialog is not modal, run the callback directly
>+    if (!mozmill.isWindows) {
>+      handler(prefctrl);
>+    }
>+  },

Can you please add it as a separate object? preferences is used for direct access to prefs while the preferences dialog doesn't fit in here. I would propose "preferencesDialog" with a get function inside.

With the above comments fixed, it will get my r+.

Comment 20

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

(In reply to comment #19)
> Can you please add it as a separate object? preferences is used for direct
> access to prefs while the preferences dialog doesn't fit in here. I would
> propose "preferencesDialog" with a get function inside.

I just wanna confirm that multi-level modal dialogs are fine too, e.g. see the master password dialog inside the preferences. Looks so good!

Comment 21

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

Oh, and we should let the user decide how the dialog should be opened (shortcut, menu). We should not use getPreferencesController() here.

Comment 22

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

Comment on attachment 382403 [details] [diff] [review]
Patch

r- until those issues are fixed.

Comment 23

[cmtalbert]

Attachment: Patch v2

Modularizes it into its own function
Adds a test and a directory for tests for the HelperClasses
Cleans up the test code to remove scope conflicts
Fixes modal dialog API to ensure it works against modal dialogs that aren't commondialogs.xul based.

Comment 24

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

Comment on attachment 382838 [details] [diff] [review]
Patch v2

That's a sweet work, Clint! As talked on IRC we still had some problems with keypress handling. I fixed that and other parts... more with the upcoming patch

Comment 25

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

Attachment: Patch v3

Updated patch with following changes:
* It's not possible to open the preference dialog via shortcuts
* Updated usage of Cc, and Ci
* Removed not necessary code from getDialogDoc due to we don't need a childDoc
* Handling of Pref dialog needs small sleep calls before and after to work
* Removed second copy of example test under technical tools

Comment 26

[cmtalbert]

Comment on attachment 382895 [details] [diff] [review]
Patch v3

>+  // Attempt default behavior
>+  PrefsAPI.doPreferencesDialog(doPref);
>+
>+  // Launch with menu clicks
>+  PrefsAPI.doPreferencesDialog(doPref, launchPrefsWithMenu);
>+
>+  // Launch with keystroke
>+  PrefsAPI.doPreferencesDialog(doPref, launchPrefsWithKeys);
CHange these to handlePreferenceDialog...and it works great :)

>diff --git a/shared-modules/testModalDialogAPI.js b/shared-modules/testModalDialogAPI.js
>--- a/shared-modules/testModalDialogAPI.js
>+++ b/shared-modules/testModalDialogAPI.js

> var mdObserver = {
>   QueryInterface : function (iid) {
>-    const interfaces = [Components.interfaces.nsIObserver,
>-                        Components.interfaces.nsISupports,
>-                        Components.interfaces.nsISupportsWeakReference];
>+    const interfaces = [Ci.nsIObserver,
>+                        Ci.nsISupports,
>+                        Ci.nsISupportsWeakReference];
I had to change these to the long form when I first wrote this code because the short form was giving me errors due to duplicate definitions of Ci etc.  It seems we must have solved that now because this works well.

>diff --git a/shared-modules/testPrefsAPI.js b/shared-modules/testPrefsAPI.js
>--- a/shared-modules/testPrefsAPI.js
>+++ b/shared-modules/testPrefsAPI.js
>@@ -14,16 +14,17 @@
>  * The Original Code is MozMill Test code.
>  *
>  * The Initial Developer of the Original Code is Mozilla Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 2009
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Henrik Skupin <[e-mail]>
>+ *   Clint Talbert <[e-mail]>
THanks!

Looks good.  Thanks Whimboo.  Ship it. r=ctalbert with those changes

Comment 27

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

(In reply to comment #26)
> >+  // Launch with keystroke
> >+  PrefsAPI.doPreferencesDialog(doPref, launchPrefsWithKeys);
> CHange these to handlePreferenceDialog...and it works great :)

Ouch. Looks like I missed the late update of the API here.
> > var mdObserver = {
> >   QueryInterface : function (iid) {
> >-    const interfaces = [Components.interfaces.nsIObserver,
> >-                        Components.interfaces.nsISupports,
> >-                        Components.interfaces.nsISupportsWeakReference];
> >+    const interfaces = [Ci.nsIObserver,
> >+                        Ci.nsISupports,
> >+                        Ci.nsISupportsWeakReference];
> I had to change these to the long form when I first wrote this code because the
> short form was giving me errors due to duplicate definitions of Ci etc.  It
> seems we must have solved that now because this works well.

Yes, it was added recently by http://code.google.com/p/mozmill/source/detail?r=480 

> Looks good.  Thanks Whimboo.  Ship it. r=ctalbert with those changes

Thanks.

Comment 28

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

Attachment: Patch v3.1

Slightly updated patch by applying previous comments.

Comment 29

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

Checked-in as http://hg.mozilla.org/qa/mozmill-tests/rev/3a4ba5140dc3

Marking as fixed.

Comment 30

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

Marking verified. Now for final! :)

Comment 31

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

Attachment: Follow-up patch to wait until loading has been finished

Clint, we were a bit too hasty in removing this code from the observer. We should really wait until the modal dialog/window has been finished loading before calling the handler.

Comment 32

[cmtalbert]

Comment on attachment 384601 [details] [diff] [review]
Follow-up patch to wait until loading has been finished

good idea, let's take it.  I don't like that we don't have anything we can point at and say that this will fix though.

Comment 33

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

(In reply to comment #32)
> (From update of attachment 384601 [details] [diff] [review])
> good idea, let's take it.  I don't like that we don't have anything we can
> point at and say that this will fix though.

We have. I had problems with the subscribe to a feed modal dialog when I clicked an element which didn't exist. So we were a bit too fast in returning back to the handler.

Checked in as http://hg.mozilla.org/qa/mozmill-tests/rev/027773843eee