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

RESOLVED FIXED in mozilla1.9.3a1

Status

RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: karlt, Assigned: enndeakin)

Tracking

Trunk
mozilla1.9.3a1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Blocks: 506173
(Reporter)

Updated

9 years ago
Depends on: 497839
(Reporter)

Comment 1

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

Comment 2

9 years ago
Created attachment 395824 [details] [diff] [review]
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.
(Assignee)

Comment 3

9 years ago
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.
(Reporter)

Comment 4

9 years ago
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).
(Reporter)

Comment 6

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

Updated

9 years ago
Summary: tests using synthesizeKey and expecting focus changes need to wait for onfocus → tests using synthesizeKey and expecting window focus changes need to wait for onfocus
(Assignee)

Comment 7

9 years ago
Created attachment 399255 [details] [diff] [review]
add more fixes

This patch adds a bunch of changes to the download manager tests to use the new waitForFocus method.
Assignee: nobody → enndeakin
Attachment #395824 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #399255 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
Attachment #395824 - Attachment is obsolete: false
(Assignee)

Comment 8

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

Comment 9

9 years ago
Maybe Mats has some ideas about how to fix the test for bug 299673.
(Reporter)

Updated

9 years ago
Depends on: 513707
Comment on attachment 399255 [details] [diff] [review]
add more fixes

r=sdwilsh
Attachment #399255 - Flags: review?(sdwilsh) → review+
Created attachment 399381 [details] [diff] [review]
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...)
Attachment #399381 - Flags: review?(enndeakin)
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?
(Assignee)

Updated

9 years ago
Attachment #399381 - Flags: review?(enndeakin) → review+
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...
Attachment #399381 - Attachment is obsolete: true
Attachment #399381 - Flags: review+ → review-
(Assignee)

Comment 14

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

I checked in this patch.

http://hg.mozilla.org/mozilla-central/rev/b4e828566c7d
Created attachment 399500 [details] [diff] [review]
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.
Attachment #399500 - Flags: review?(enndeakin)
(Assignee)

Comment 16

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

Comment 17

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

Comment 19

9 years ago
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.
Attachment #399500 - Flags: review?(enndeakin) → review+
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
(Assignee)

Comment 21

9 years ago
OK, so this should be done and marked fixed right?
Yup, that was the last one AFAICT.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

9 years ago
Flags: in-testsuite+
Depends on: 553344
(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.
status1.9.2: --- → beta1-fixed
You need to log in before you can comment on or make changes to this bug.