506175 - tests using synthesizeKey and expecting window focus changes need to wait for onfocus

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Karl Tomlinson (:karlt)]

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.

Comment 1

[Karl Tomlinson (:karlt)]

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.

Comment 2

[Neil Deakin]

Attachment: disable part of test on linux

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.

Comment 3

[Neil Deakin]

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.

Comment 4

[Karl Tomlinson (:karlt)]

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

Comment 5

[Nochum Sossonko [:Natch]]

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

Comment 6

[Karl Tomlinson (:karlt)]

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

Comment 7

[Neil Deakin]

Attachment: add more fixes

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

Comment 8

[Neil Deakin]

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

Comment 9

[Neil Deakin]

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

Comment 10

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix test_bug299673.html

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

Comment 12

[Mats Palmgren (inactive)]

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 13

[Mats Palmgren (inactive)]

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 14

[Neil Deakin]

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

I checked in this patch.

http://hg.mozilla.org/mozilla-central/rev/b4e828566c7d

Comment 15

[Mats Palmgren (inactive)]

Attachment: fix test_bug299673, rev. 2

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.

Comment 16

[Neil Deakin]

(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?

Comment 17

[Neil Deakin]

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

Comment 18

[Mats Palmgren (inactive)]

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 19

[Neil Deakin]

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 20

[Mats Palmgren (inactive)]

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

Comment 21

[Neil Deakin]

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

Comment 22

[Mats Palmgren (inactive)]

Yup, that was the last one AFAICT.

-> FIXED

Comment 23

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/838b26c49605

Comment 24

[Serge Gautherie (:sgautherie)]

(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.