The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: RemusPop, Assigned: RemusPop)

Tracking

unspecified
All
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox16 fixed, firefox-esr10 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

5 years ago
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.
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?
(Assignee)

Comment 2

5 years ago
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.
Assignee: nobody → remus.pop
Status: NEW → ASSIGNED
Would you mind to put together a minimized testcase?
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
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?
Oh and what I missed, a sleep call of about 50ms before clicking the language button fixes it for me.
(Assignee)

Comment 8

5 years ago
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.
As mentioned please backout vlad's patch locally and check if that works for you.
(Assignee)

Comment 10

5 years ago
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?
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.
(Assignee)

Comment 12

5 years ago
'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.
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.
(Assignee)

Comment 14

5 years ago
Maybe we can find something useful here: https://developer.mozilla.org/en/XUL/prefpane#Events
Remus, we really have to get in the preferences patch, so would you need any help on this bug?
(Assignee)

Comment 16

5 years ago
'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.
(Assignee)

Comment 17

5 years ago
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.
Do you have a code snippet you can share with us? I would kinda like to check it myself.
(Assignee)

Comment 19

5 years ago
Created attachment 630881 [details]
WIP on pref lib

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

Comment 20

5 years ago
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.
Attachment #632214 - Flags: review?(hskupin)
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.
Attachment #632214 - Flags: review?(hskupin) → review-
(Assignee)

Comment 22

5 years ago
Created attachment 634409 [details] [diff] [review]
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
Attachment #632214 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #634409 - Flags: review?(hskupin)
(Assignee)

Comment 23

5 years ago
Created attachment 634413 [details] [diff] [review]
patch v1 (default)

Sorry, used other machine to build the patch. Here is mine.
Attachment #634409 - Attachment is obsolete: true
Attachment #634409 - Flags: review?(hskupin)
Attachment #634413 - Flags: review?(hskupin)
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.
Attachment #634413 - Flags: review?(hskupin) → review-
(Assignee)

Comment 25

5 years ago
Created attachment 635219 [details] [diff] [review]
Patch v2 (default)

Updated the comment before the sleep call.
Attachment #634413 - Attachment is obsolete: true
Attachment #635219 - Flags: review?(hskupin)
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.
Attachment #635219 - Flags: review?(hskupin) → review-
OS: Mac OS X → All
Created attachment 635224 [details] [diff] [review]
paneContent example
Attachment #630881 - Attachment is obsolete: true
Attachment #635219 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
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.
Attachment #635299 - Flags: feedback?(hskupin)
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.
OS: All → Mac OS X
Attachment #635299 - Flags: feedback?(hskupin)
Summary: Failure in testPrefferedLanguage | Modal dialog has been found and processed when using the refactored prefs → Preferences window uses an animation on OS X which causes a early return in the paneId setter
(Assignee)

Comment 30

5 years ago
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.
Attachment #635224 - Attachment is obsolete: true
Attachment #635299 - Attachment is obsolete: true
Attachment #635674 - Flags: review?(hskupin)
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!
Attachment #635674 - Flags: review?(hskupin) → review-
(Assignee)

Comment 32

5 years ago
I used lastSelected property but that gives huge transition times in the waitFor, ~3-4 seconds. Do we still want to use it?
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.
(Assignee)

Comment 34

5 years ago
I am waiting for the lastSelected to be the expected pane, but it changes very slow.
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.
(Assignee)

Comment 36

5 years ago
This is how the code looks like;
http://pastebin.mozilla.org/1680490
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.
(Assignee)

Comment 38

5 years ago
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.
Attachment #635674 - Attachment is obsolete: true
Attachment #637110 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Attachment #637110 - Flags: review?(dave.hunt)
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
Attachment #637110 - Flags: review?(hskupin)
Attachment #637110 - Flags: review?(dave.hunt)
Attachment #637110 - Flags: review-
(Assignee)

Updated

5 years ago
Blocks: 671382
(Assignee)

Comment 40

5 years ago
Created attachment 637414 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Addressed all requests.
Attachment #637110 - Attachment is obsolete: true
Attachment #637414 - Flags: review?(hskupin)
Comment on attachment 637414 [details] [diff] [review]
patch v3 (all branches) [checked-in]

Looks good now. Thanks Remus.
Attachment #637414 - Flags: review?(hskupin) → review+
Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/143b4191964e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → fixed
Resolution: --- → FIXED
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)
status-firefox-esr10: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
Depends on: 770116
(Assignee)

Updated

5 years ago
Attachment #637414 - Attachment description: patch v3 (default) → patch v3 (all branches) [checked-in]
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.