A number of tests are calling focus() on the window in the expectation that this will make the window respond to synthesizeKey(). synthesizeKey will only cause the event to be processed on the specified window when the focus manager thinks that the window is the active window (bug 497839). However window.focus() does not make the window active on X11. It merely issues a request that the window should become the active window. The window manager will decide whether to grant the request. If the request is granted, the activation will happen some finite time after the window.focus() call has completed, and the focus manager will find out about this when a focus notification event on the event queue is processed. It seems that the best way to know when the focus manager knows that the window is active is to wait for the onfocus event (when available). The window manager will not necessarily grant the request for activation but most window managers will probably grant the request if the currently active (different) window is part of the same app. This is good enough if tests are run as the active app. There are exceptions but tests already need to be run under certain window managers. Currently the GTK implementation of nsIWidget::SetFocus() is effectively emulating activation of the window but this is a bug (bug 506173 ). I asked Enn about whether tests could do the same emulation (instead), but he said that window.focus() is the means to activate a window. Things that rely on activation will need to wait for the activation to happen.

I sent attachment 390603 [details] [diff] [review] (which removes the emulation in SetFocus) to the try server and the only tests that failed were: browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js and tests/content/events/test/test_bug299673.html Those were the same two failures as with attachment 386618 [details] [diff] [review], but there may be other tests that start to fail intermittently.

The simplest thing to do here is disable part of the test on Linux. The bug the test is testing relies on the window activation being synchronous. Since that doesn't happen, trying to adjust the test wouldn't actually test the bug anyway.

That last patch was fixes the problem with test_bug299673.html. I can't reproduce the proble with browser_passwordmgrdlg.js. I guess we don't have the logs anymore though. Maybe we should test both attachment 390603 [details] [diff] [review] and the fix here on the tryserver and see what happens. If no errors, we should just review and check in.

I sent attachment 395824 [details] [diff] [review] and attachment 390603 [details] [diff] [review] to the tryserver, and test_esc_key_closes_clears.xul and browser_passwordmgrdlg.js had trouble. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1251150782.1251157206.1447.gz#err0 7508 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_esc_key_closes_clears.xul | Escape correctly emptied the search box - got "download manager escape test", expected "" 7509 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_esc_key_closes_clears.xul | Previous escape moved focus to list and now escape closed the window - got false, expected true This would be resurrection of bug 498805, which is expected from the way that was "fixed". Running chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js... TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | Timed out That's not so helpful. In the past I've seen these messages: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 4 logins should match 'pass' - Got 0, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 10 logins should match '' - Got 0, expected 10 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 10 logins should match '' - Got 0, expected 10 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 7 logins should match 'moz' - Got 10, expected 7 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 1 logins should match 'mozilla.com' - Got 7, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 4 logins should match 'user' - Got 1, expected 4 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 1 logins should match 'user ' - Got 4, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 2 logins should match ' user' - Got 1, expected 2 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 10 logins should match 'http' - Got 2, expected 10 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 1 logins should match 'https' - Got 10, expected 1 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | 0 logins should match 'secret' - Got 1, expected 0

This is only for |window.focus()|, not |element.focus()|, right? Otherwise, there are lots more tests under toolkit/content/tests/widgets that may need updating (and possibly others as well).

Right. element.focus() is synchronous, and the element will receive focus if the window is the active window.

This patch adds a bunch of changes to the download manager tests to use the new waitForFocus method.

With both of these changes and the patch in bug 506173, the tryserver passes all tests on Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252426588.1252433581.5116.gz

Maybe Mats has some ideas about how to fix the test for bug 299673.

Comment on attachment 399255 [details] [diff] [review] add more fixes r=sdwilsh

This works for me locally on Linux with the planned changes. (I'll submit it to TryServer but that may take a while to get results...)

The test_bug299673.html patch passed the TryServer on all platforms. But there's another test that failed on Linux and Windows: content/events/test/test_bug322588.html I'll take a stab at it tomorrow unless someone is already working on it?

Comment on attachment 399381 [details] [diff] [review] fix test_bug299673.html There's still something wrong with this test. The error I blamed on test_bug322588 (which follows test_bug299673) is a lingering error from test_bug299673, but mochitest reports it as a failure for test_bug322588. Here's the full log: http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1252461929.1252468869.2213.gz&buildtime=1252461929&buildname=Linux%20try%20hg%20unit%20test&fulltext=1#err0 I didn't expect that since test_bug299673 has a SimpleTest.waitForExplicitFinish()... Will investigate some more...

Comment on attachment 399255 [details] [diff] [review] add more fixes I checked in this patch. http://hg.mozilla.org/mozilla-central/rev/b4e828566c7d

There were a couple of problems with my first attempt - SimpleTest.finish() needs to be called at the end in the last callback of the SimpleTest.waitForFocus chain. It seems SimpleTest.waitForFocus only works once for each document, so I had to make separate "open in tab" / "open in window" test files. (I moved the JS into a external file too, shared by the two tests) This works for OSX and Windows. I disabled the test completely for Linux because with the changes in bug 506173 the sequence of events isn't predictable. (The tests runs a single todo() for Linux) Tested on TryServer with expected results.

(In reply to comment #15) > It seems SimpleTest.waitForFocus only works once for each document, If that's the case, then we should fix that instead. What problem occurs?

I just did a simple check with SimpleTest.waitForFocus and it worked ok being run multiple times.

You're right, it works. The problem was that the second part of the test (open in window) clobbered the waitForFocus status of the first part. After serializing the two parts, the test works correctly. I think splitting the two parts into separate files (rev. 2) simplifies things though, so I suggest we go ahead with that.

Comment on attachment 399500 [details] [diff] [review] fix test_bug299673, rev. 2 >+function doTest1_rest2(expectedEventLog,focusAfterCloseId) { >+ netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); >+ try { >+ is(document.activeElement, document.getElementById(focusAfterCloseId), "wrong element is focused after popup was closed"); >+ is(result, expectedEventLog, "unexpected events"); >+ SimpleTest.waitForFocus(done299673, window); Don't think this waitForFocus is needed. doTest1_rest2 was already called via waitForFocus and nothing of note has happened since.

Comment on attachment 399500 [details] [diff] [review] fix test_bug299673, rev. 2 Fixed the nit and landed: http://hg.mozilla.org/mozilla-central/rev/aaeac0263469

OK, so this should be done and marked fixed right?

Yup, that was the last one AFAICT. -> FIXED

(In reply to Mats Palmgren [:mats] from comment #20) > Comment on attachment 399500 [details] [diff] [review] > fix test_bug299673, rev. 2 > > Fixed the nit and landed: > http://hg.mozilla.org/mozilla-central/rev/aaeac0263469 Ftr, this changeset was not backported to m-1.9.2.