Last Comment Bug 757340 - Preferences window uses an animation on OS X which causes a early return in the paneId setter
: Preferences window uses an animation on OS X which causes a early return in t...
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: ---
Assigned To: Remus Pop (:RemusPop)
Depends on: 770116
Blocks: 671382 736764
  Show dependency treegraph
Reported: 2012-05-22 00:16 PDT by Remus Pop (:RemusPop)
Modified: 2012-08-14 15:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

minimized testcase (1.53 KB, patch)
2012-05-22 06:06 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Splinter Review
WIP on pref lib (762 bytes, text/plain)
2012-06-07 00:37 PDT, Remus Pop (:RemusPop)
no flags Details
skip test v1 (default) (1.37 KB, patch)
2012-06-12 05:18 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
patch v1 (1.01 KB, patch)
2012-06-19 07:24 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Splinter Review
patch v1 (default) (1.00 KB, patch)
2012-06-19 07:29 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
Patch v2 (default) (1.10 KB, patch)
2012-06-21 01:05 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
paneContent example (1.25 KB, patch)
2012-06-21 01:38 PDT, Henrik Skupin (:whimboo)
no flags Details | Diff | Splinter Review
testcase + trial fix (3.19 KB, patch)
2012-06-21 07:21 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Splinter Review
patch v1 (default) (1.83 KB, patch)
2012-06-22 02:45 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
Patch v2 (default) (2.15 KB, patch)
2012-06-27 07:26 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Splinter Review
patch v3 (all branches) [checked-in] (2.18 KB, patch)
2012-06-28 02:05 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Splinter Review

Description Remus Pop (:RemusPop) 2012-05-22 00:16:05 PDT
When using the refactored pref tests in bug 736764, testPrefferedLanguage fails in finding the modal-dialog. That happens because the "Choose" button is not clicked.
Now comes the interesting part: this fails only if testPasswordSavedAndDeleted is in the folder and has been ran before.
This only happens on Mac.
Comment 1 Henrik Skupin (:whimboo) 2012-05-22 01:10:46 PDT
We usually should fix it in the same bug where the refactoring happens. If that's lot more work lets do it in-front and block bug 736764 from landing - as what you have already set. Remus, would you mind working on it?
Comment 2 Remus Pop (:RemusPop) 2012-05-22 01:25:19 PDT
Sorry, forgot to assign myself. I am already working on it.
This works if we call sleep before this line:
Until now it works with a value of 250ms but not with 100ms.
Comment 3 Henrik Skupin (:whimboo) 2012-05-22 01:29:34 PDT
Would you mind to put together a minimized testcase?
Comment 4 Remus Pop (:RemusPop) 2012-05-22 05:38:13 PDT
Working towards the minimized testcase it seems that setting the pane in the preferences causes the other test to fail even if the second test sets the desired pane.
Comment 5 Remus Pop (:RemusPop) 2012-05-22 06:06:57 PDT
Created attachment 625983 [details] [diff] [review]
minimized testcase

Copy this to a repo in any folder of functional tests (e.g. testPreferences).
Run it like you would normally run a test with mozmill, on any version.
The testcase opens the pref dialog and sets the pane to Security, then closes it, then opens it and sets the pane to Content and tries to click the "Choose" button. If it succeeds in clicking the "Choose button" it will pass. Otherwise it will fail with "Modal-dialog has been found and processed" because the button wasn't clicked.
Line #31 can be commented out for the test to pass.
Remember, this fails on Mac only.
Feel free to add feedback to the testcase if it's needed.
Comment 6 Henrik Skupin (:whimboo) 2012-05-23 01:50:02 PDT
So this is how we wait for the correct pane to be loaded. Not sure if this is a regression from Vlad's patch back on bug 671382. Remus, would you mind testing if backing out this patch solves the problem for you?
Comment 7 Henrik Skupin (:whimboo) 2012-05-23 01:51:39 PDT
Oh and what I missed, a sleep call of about 50ms before clicking the language button fixes it for me.
Comment 8 Remus Pop (:RemusPop) 2012-05-23 02:46:21 PDT
I tried with different values. I got to 120 ms working one time, but after that it wasn't sufficient. So not sure if this will work.
Comment 9 Henrik Skupin (:whimboo) 2012-05-23 03:11:08 PDT
As mentioned please backout vlad's patch locally and check if that works for you.
Comment 10 Remus Pop (:RemusPop) 2012-05-29 01:53:42 PDT
I backed out the patch and the failure stays the same. Though a sleep call also fixed this for me. What could we possibly wait for there?
Comment 11 Henrik Skupin (:whimboo) 2012-05-29 02:08:58 PDT
Please check for the states we are waiting for when setting the target pane. Dump all the values inside the waitFor call to the console. Probably we have to wait for some other event.
Comment 12 Remus Pop (:RemusPop) 2012-05-29 04:58:05 PDT
'Selected' and 'Focused' are present for a pane.
I used 'Selected' but I had no luck with it and 'Focused' seems to be always false when use in a test.
Comment 13 Henrik Skupin (:whimboo) 2012-05-29 07:23:56 PDT
I wonder if there is an event which gets raised when the pane has been switched. Would you mind checking that? It would be safer that way as polling the selected status.
Comment 14 Remus Pop (:RemusPop) 2012-05-30 00:32:25 PDT
Maybe we can find something useful here:
Comment 15 Henrik Skupin (:whimboo) 2012-06-05 04:38:37 PDT
Remus, we really have to get in the preferences patch, so would you need any help on this bug?
Comment 16 Remus Pop (:RemusPop) 2012-06-05 04:44:13 PDT
'paneload' event fires when a pane has been loaded. However, it is fired only once per pane. Visiting already visited panes does not longer trigger this event.
I need to check if closing the pref pane unloads all content and reopening prefs fires again the event for each pane.
Comment 17 Remus Pop (:RemusPop) 2012-06-06 02:57:16 PDT
It seems that we're safe on this side. Closing the pref dialog, reopening an re-attaching the event listener fires 'paneload' even if the pane was loaded previously.

I managed to attach the event listener in the test, but the testcase still fails. I'm not sure if this is the event that we want.
Comment 18 Henrik Skupin (:whimboo) 2012-06-06 14:09:36 PDT
Do you have a code snippet you can share with us? I would kinda like to check it myself.
Comment 19 Remus Pop (:RemusPop) 2012-06-07 00:37:11 PDT
Created attachment 630881 [details]
WIP on pref lib

Added my work in progress for prefs.js on the setter of paneId.
Comment 20 Remus Pop (:RemusPop) 2012-06-12 05:18:50 PDT
Created attachment 632214 [details] [diff] [review]
skip test v1 (default)

We decided to skip the test and go forward with the refactoring of the prefs.
Comment 21 Henrik Skupin (:whimboo) 2012-06-14 02:12:07 PDT
Comment on attachment 632214 [details] [diff] [review]
skip test v1 (default)

Talked with Remus on IRC yesterday and the probably best solution here is to really add a temporary sleep call in the preferences dialog class in prefs.js. If we would only disable this test others could be affected too and I really don't want to skip more tests.
Comment 23 Remus Pop (:RemusPop) 2012-06-19 07:29:59 PDT
Created attachment 634413 [details] [diff] [review]
patch v1 (default)

Sorry, used other machine to build the patch. Here is mine.
Comment 24 Henrik Skupin (:whimboo) 2012-06-20 04:40:08 PDT
Comment on attachment 634413 [details] [diff] [review]
patch v1 (default)

>+    // The pane is loaded lazy so we need to wait
>+    this._controller.sleep(500);

The comment has to point out the bug number and a short description why we have to do that, which means that the events aren't triggered correctly.
Comment 25 Remus Pop (:RemusPop) 2012-06-21 01:05:09 PDT
Created attachment 635219 [details] [diff] [review]
Patch v2 (default)

Updated the comment before the sleep call.
Comment 26 Henrik Skupin (:whimboo) 2012-06-21 01:36:50 PDT
Comment on attachment 635219 [details] [diff] [review]
Patch v2 (default)

Ok, so I have had time to check that quickly. As the testcase above shows me you are waiting for the paneload event on the wrong element. You really have to add the event listener to the target pane we want to switch to. I will upload an example patch which will only work for this case and needs to be generalized.
Comment 27 Henrik Skupin (:whimboo) 2012-06-21 01:38:02 PDT
Created attachment 635224 [details] [diff] [review]
paneContent example
Comment 28 Remus Pop (:RemusPop) 2012-06-21 07:21:37 PDT
Created attachment 635299 [details] [diff] [review]
testcase + trial fix

As talked on IRC, I'm attaching the testcase along with Henrik's try to fix this issue. The fix contains an events listener that is attached to the panel that we will switch to.
The testcase opens the pref pane once, closes it, opens it again, changes the pane and tries to click a button that should open a modal dialog. If the modal dialog will not be opened, the testcase will fail.
The testcase is in /test/functional/testPreferences folder.
Comment 29 Henrik Skupin (:whimboo) 2012-06-21 23:56:04 PDT
So I checked the code of prefpanes and the prefwindow and found the reason why we are failing on OS X. Why you can see here:

So we could disable the animation when switching panes. That's something I would prefer compared to the sleep approach. We can do it the same way as for opening/closing tabs.
Comment 30 Remus Pop (:RemusPop) 2012-06-22 02:45:20 PDT
Created attachment 635674 [details] [diff] [review]
patch v1 (default)

We now disable the animation pref before switching the pane so listening to the 'paneload' event is now safe and guarantees that the pane has been loaded.
Comment 31 Henrik Skupin (:whimboo) 2012-06-22 08:24:34 PDT
Comment on attachment 635674 [details] [diff] [review]
patch v1 (default)

>+    animationPref = "browser.preferences.animateFadeIn";

This is a const at the top level.

>+    var selected = false;
>+    var pane = new elementslib.ID(this._controller.window.document, id);
>+    function paneloaded() {
>+      pane.getNode().removeEventListener('paneload', paneloaded, false);
>+      selected = true;
>+    }

No, we don't want to do that anymore. Reason is that this even is really fired only once when selecting the pane the first time. We want to check that the right pane is 'selected'.

>+      return selected && selector.getNode().selectedItem.getAttribute('pane') === id;
>+    }, "Pane has been changed - expected '" + id + "'");

We even don't need the selector anymore. Please check the documentElement of the window which has a 'lastSelected' property.

>+    // Set the default behavior for the animation when switching tabs
>+    preferences.clearUserPref(animationPref);

Keep in mind that this will not reset the pref if the waitFor throws an exception!
Comment 32 Remus Pop (:RemusPop) 2012-06-27 01:44:14 PDT
I used lastSelected property but that gives huge transition times in the waitFor, ~3-4 seconds. Do we still want to use it?
Comment 33 Henrik Skupin (:whimboo) 2012-06-27 02:34:45 PDT
When and for which pane? This is expected if the specified pane hasn't been selected before and content has to be loaded first. It especially applies to the application tab.
Comment 34 Remus Pop (:RemusPop) 2012-06-27 02:37:05 PDT
I am waiting for the lastSelected to be the expected pane, but it changes very slow.
Comment 35 Henrik Skupin (:whimboo) 2012-06-27 02:42:28 PDT
Remus, that's not I was asking for. I know for what lastSelected is used for. But what I want to know is which pane are you waiting for.
Comment 36 Remus Pop (:RemusPop) 2012-06-27 02:51:26 PDT
This is how the code looks like;
Comment 37 Henrik Skupin (:whimboo) 2012-06-27 02:55:14 PDT
You can't do the following line outside of the waitFor callback:

var selected = this._controller.window.document.documentElement.lastSelected;

Reason is that lastSelected is a simple property and not copied as reference in this case.
Comment 38 Remus Pop (:RemusPop) 2012-06-27 07:26:14 PDT
Created attachment 637110 [details] [diff] [review]
Patch v2 (default)

Added a try-catch-finally block which will catch the exception from the waitFor function and the finally block will assure the clearing of the prefs. If any exception is thrown by the try block we'll catch it and throw it again.
Comment 39 Henrik Skupin (:whimboo) 2012-06-27 07:46:23 PDT
Comment on attachment 637110 [details] [diff] [review]
Patch v2 (default)

In general that looks good. Just one more thing I would like to see changed:

>+    try {
>+      this._controller.waitFor(function () {
>+        return this._controller.window.document.documentElement.lastSelected === id;
>+      }, "Pane has been changed - expected '" + id + "'", undefined, undefined, this);

Create a local variable of documentElement before the waitFor. So we will have a shorter line and also can get rid of the undefined and 'this' parameters. 

>+    finally {
>+      preferences.clearUserPref(PREF_PANE_ANIMATION);
>+    }
>     return this.paneId;

nit: missing empty line
Comment 40 Remus Pop (:RemusPop) 2012-06-28 02:05:44 PDT
Created attachment 637414 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Addressed all requests.
Comment 41 Henrik Skupin (:whimboo) 2012-06-28 23:45:36 PDT
Comment on attachment 637414 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Looks good now. Thanks Remus.
Comment 42 Henrik Skupin (:whimboo) 2012-06-28 23:46:47 PDT
Pushed to default:

Note You need to log in before you can comment on or make changes to this bug.