757340 - Preferences window uses an animation on OS X which causes a early return in the paneId setter

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

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][⌚️UTC+1]]

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)]

Sorry, forgot to assign myself. I am already working on it.
This works if we call sleep before this line:
http://hg.mozilla.org/qa/mozmill-tests/file/01dbffb740b4/tests/functional/testPreferences/testPreferredLanguage.js#l65
Until now it works with a value of 250ms but not with 100ms.

Comment 3

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

Would you mind to put together a minimized testcase?

Comment 4

[Remus Pop (:RemusPop)]

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)]

Attachment: 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][⌚️UTC+1]]

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][⌚️UTC+1]]

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)]

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][⌚️UTC+1]]

As mentioned please backout vlad's patch locally and check if that works for you.

Comment 10

[Remus Pop (:RemusPop)]

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][⌚️UTC+1]]

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)]

'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][⌚️UTC+1]]

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)]

Maybe we can find something useful here: https://developer.mozilla.org/en/XUL/prefpane#Events

Comment 15

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

Remus, we really have to get in the preferences patch, so would you need any help on this bug?

Comment 16

[Remus Pop (:RemusPop)]

'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)]

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][⌚️UTC+1]]

Do you have a code snippet you can share with us? I would kinda like to check it myself.

Comment 19

[Remus Pop (:RemusPop)]

Attachment: WIP on pref lib

Added my work in progress for prefs.js on the setter of paneId.

Comment 20

[Remus Pop (:RemusPop)]

Attachment: skip test v1 (default)

We decided to skip the test and go forward with the refactoring of the prefs.

Comment 21

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

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 22

[Remus Pop (:RemusPop)]

Attachment: patch v1

Added a sleep call.

Also, the results for testruns on different systems.

Mac:
* http://mozmill-crowd.blargon7.com/#/functional/report/c67fb1fc2ea8384105b843e32c04970d
* http://mozmill-crowd.blargon7.com/#/functional/report/c67fb1fc2ea8384105b843e32c04871e

Linux:
* http://mozmill-crowd.blargon7.com/#/functional/report/c67fb1fc2ea8384105b843e32c0294bb
* http://mozmill-crowd.blargon7.com/#/functional/report/c67fb1fc2ea8384105b843e32c02747e

Windows:
* http://mozmill-crowd.blargon7.com/#/functional/report/c67fb1fc2ea8384105b843e32c0491b8
* http://mozmill-crowd.blargon7.com/#/functional/report/c67fb1fc2ea8384105b843e32c0480b2

Comment 23

[Remus Pop (:RemusPop)]

Attachment: patch v1 (default)

Sorry, used other machine to build the patch. Here is mine.

Comment 24

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

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)]

Attachment: Patch v2 (default)

Updated the comment before the sleep call.

Comment 26

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

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][⌚️UTC+1]]

Attachment: paneContent example

Comment 28

[Remus Pop (:RemusPop)]

Attachment: 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][⌚️UTC+1]]

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:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#858

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)]

Attachment: 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][⌚️UTC+1]]

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)]

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][⌚️UTC+1]]

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)]

I am waiting for the lastSelected to be the expected pane, but it changes very slow.

Comment 35

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

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)]

This is how the code looks like;
http://pastebin.mozilla.org/1680490

Comment 37

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

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)]

Attachment: 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][⌚️UTC+1]]

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)]

Attachment: patch v3 (all branches) [checked-in]

Addressed all requests.

Comment 41

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

Comment on attachment 637414 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Looks good now. Thanks Remus.

Comment 42

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

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/143b4191964e

Comment 43

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

Landed on older branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/bd6b42cf33c8 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/529c07d5febf (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/7dcba33db2ed (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/16f620a1eae2 (esr10)