491624 - [SeaMonkey, Linux] mochitest-browser-chrome: intermittent ".../suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got [object HTMLInputElement], expected [object XULElement]"

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1241561224.1241566394.16268.gz
Linux comm-central unit test on 2009/05/05 15:07:04

{
chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab not activeElement
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab again activeElement
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got [object HTMLInputElement], expected [object XULElement]
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab while focused still activeElement
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on another tab while focused still activeElement
chrome://mochikit/content/browser/suite/browser/test/browser_feed_tab.js
}

Comment 1

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1241571349.1241576446.28456.gz
Linux comm-central unit test on 2009/05/05 17:55:49

Comment 2

[Robert Kaiser]

Gah, looks like bug 488025 with different symptoms :(

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> Gah, looks like bug 488025 with different symptoms :(

Indeed.

Comment 4

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1242073151.1242078292.26779.gz
Linux comm-1.9.1 unit test on 2009/05/11 13:19:11

Comment 5

[Serge Gautherie (:sgautherie)]

Last time it happened on regular box:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1242391906.1242397375.11138.gz
Linux comm-central unit test on 2009/05/15 05:51:46

whereas it seems to happen _very frequently_ on SeaMonkey-Ports, as in:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1242476841.1242480915.5765.gz
Linux comm-1.9.1 unit test on 2009/05/16 05:27:21

May be you could try to set an actual delay in |setTimeout(step3_5, 0);| and/or try bug 488025 comment 6 suggestion?
Or you could disable this test/check for the time being...

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> whereas it seems to happen _very frequently_ on SeaMonkey-Ports, as in:

Well, maybe it's more random than not:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1242586360.1242592285.397.gz
Linux comm-central unit test on 2009/05/17 11:52:40
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1242592185.1242596736.7857.gz
Linux comm-central unit test on 2009/05/17 13:29:45
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1242599198.1242603768.19991.gz
Linux comm-central unit test on 2009/05/17 15:26:38

Comment 7

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1242635218.1242639804.14918.gz
Linux comm-central unit test on 2009/05/18 01:26:58
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1242640801.1242645358.24218.gz
Linux comm-central unit test on 2009/05/18 03:00:01

Comment 8

[[e-mail]]

Attachment: Proposed patch [Checkin: Comment 10]

Changing the click position to 9, 9 seems to make the test more reliable.

I had to change the isnot to todo_is because we differ from Firefox (this difference is what bug 462289 is all about).

I added an extra test for good measure.

Comment 9

[Robert Kaiser]

Comment on attachment 378285 [details] [diff] [review]
Proposed patch
[Checkin: Comment 10]

Let's hope this actually clears this intermittent orange now, thanks for investigating!

Comment 10

[[e-mail]]

Pushed changeset bd555c5dc66f to comm-central.

Our linux test builds went green for a couple of cycles (before someone inconsiderately checked in some 1.9.1 bustage!)

Comment 11

[Serge Gautherie (:sgautherie)]

Well, same failure still happens :-/

For example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1243182039.1243186555.29946.gz
Linux comm-central unit test on 2009/05/24 09:20:39
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1243195135.1243199665.21947.gz
Linux comm-central unit test on 2009/05/24 12:58:55

Running chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js...
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab selects tab
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab not activeElement
TEST-KNOWN-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab again activeElement
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got [object HTMLInputElement], expected [object XULElement]
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on tab while focused still activeElement
TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | mouse on another tab while focused still activeElement
}

Comment 12

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1243197291.1243201411.24997.gz
Linux comm-1.9.1 unit test on 2009/05/24 13:34:51

Comment 13

[[e-mail]]

Sorry, I can't reproduce this locally :-(

Comment 14

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1243378847.1243383696.28403.gz
Linux comm-central unit test on 2009/05/26 16:00:47

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Use todo() ftb

This happen rather often: let's work around this ftb.

Comment 16

[Serge Gautherie (:sgautherie)]

The failing test does
{
EventUtils.synthesizeKey("VK_TAB", { });
}

Could it be related to bug 335936?

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 380297 [details] [diff] [review]
(Bv1) Use todo() ftb


NB: Actually, I could do this for Linux only...

Comment 18

[Robert Kaiser]

(In reply to comment #16)
> Could it be related to bug 335936?

Possibly, but then I still wonder why Firefox doesn't have the same problem.

Comment 19

[Robert Kaiser]

Comment on attachment 380297 [details] [diff] [review]
(Bv1) Use todo() ftb

1) It doesn't happen everywhere
2) We are not out of options yet, IMHO we should at least try the workaround I hadin place in some earlier iteration of the testcase, where I issued the tab key a second time if we were still on the input element.

Comment 20

[Robert Kaiser]

Attachment: move setTimeout() to actually be between tab key and testingAdditional diagnostic [Backout: Comment 50]

So, I still could see the failure locally, and I noticed that when I did an if for the active element that tried re-sending a tab key when the input element is still focused, that was pretty consistently triggered but it didn't actually help. As soon as I changed it into a while loop, the test worked and the loop was never triggered.
Moving the key generation before the setTimeout() also helped and is the easier solution, so let's try that one. It looks like the focus works fine, the tab key needs to actually settle.

Comment 21

[Robert Kaiser]

Comment on attachment 380638 [details] [diff] [review]
move setTimeout() to actually be between tab key and testingAdditional diagnostic
[Backout: Comment 50]

Pushed as http://hg.mozilla.org/comm-central/rev/ea695ad6ee9f - let's see how this performs on the test boxes.

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> let's see how this performs on the test boxes.

Not enough, but made a difference:
the error changed (a little):
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1243751558.1243756173.22047.gz
Linux comm-1.9.1 unit test on 2009/05/30 23:32:38

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got [object XULElement], expected [object XULElement]
}

Comment 23

[Serge Gautherie (:sgautherie)]

Ah,
{
29   document.getElementById("urlbar").inputField.focus();
31   EventUtils.synthesizeKey("VK_TAB", { });
}
would very much look like newly filed Core bug 495751...

Comment 24

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1244704895.1244710227.7854.gz
Linux comm-1.9.1 unit test on 2009/06/11 00:21:35

Comment 25

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1244908544.1244913200.8473.gz
Linux comm-central unit test on 2009/06/13 08:55:44

This is happening rather often: workaround wanted.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 380297 [details] [diff] [review]
(Bv1) Use todo() ftb

Reviving this workaround, as the others were not enough.

Comment 27

[Robert Kaiser]

Comment on attachment 380297 [details] [diff] [review]
(Bv1) Use todo() ftb

I don't really like that "patch", it's too lame IMHO.

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)

What else then?

Comment 29

[Robert Kaiser]

(In reply to comment #28)
> (In reply to comment #27)
> 
> What else then?

Still somewhat lame, but IMHO better would be to only make it a todo() when things don't work and make it pass correctly when it does. Unfortunately, there's no maybe_todo() or so.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) Use todo() ftb

Bv1, with comment 17 suggestion(s).

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 386915 [details] [diff] [review]
(Bv1a) Use todo() ftb


PS: I'll remove the superfluous '{}'.

Comment 32

[Robert Kaiser]

Comment on attachment 386915 [details] [diff] [review]
(Bv1a) Use todo() ftb

better, but could we make it todo_is() and only in case the is() would fail? I know that's cheating the system and I'd like Neil to review that as well, but I think we should be green where we can.

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 386915 [details] [diff] [review]
(Bv1a) Use todo() ftb


(In reply to comment #32)
> better, but could we make it todo_is() and only in case the is() would fail? I

What would be the gain?
Also, as this bug seems to be some kind of "timing" issue, I have no way to know whether an if() could fail and the following todo() succeed...
I'd rather keep the code simple and safe ... and the reminder visible.

> know that's cheating the system and I'd like Neil to review that as well, but I

Asking him, too.

> think we should be green where we can.

The test will be green any way.

Comment 34

[[e-mail]]

Strictly speaking this bug is about Got [object HTMLInputElement], expected [object XULElement] but the current test failures are for Got [object XULElement], expected [object XULElement] which is completely different.

Comment 35

[[e-mail]]

Attachment: Extra diagnostic [Checkin: Comment 45]

This will help us pin down exactly where focus is going :-)

Comment 36

[Robert Kaiser]

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got browser, expected tab
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got [object XULElement], expected [object XULElement]

Comment 37

[[e-mail]]

(In reply to comment #36)
> TEST-UNEXPECTED-FAIL |
> chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab
> key to tab activeElement - Got browser, expected tab
> TEST-UNEXPECTED-FAIL |
> chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab
> key to tab activeElement - Got [object XULElement], expected [object
> XULElement]
Hmm... so, there are two possibilities:
1. Focus went straight from the URLbar to the browser (content) - unlikely?
2. Focus never went to the URLbar...

Comment 38

[[e-mail]]

Attachment: Additional diagnostic [Backout: Comment 50]

I hope I don't need too many of these, I'm going to run out of synonyms ;-)

Comment 39

[Robert Kaiser]

TEST-PASS | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | focus URL bar activeElement
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got browser, expected tab
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/suite/browser/test/browser_bug462289.js | tab key to tab activeElement - Got [object XULElement], expected [object XULElement]

Comment 40

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1248091605.1248097108.13715.gz
Linux comm-1.9.1 unit test on 2009/07/20 05:06:45

... | tab key to tab activeElement - Got input, expected tab
... | tab key to tab activeElement - Got [object HTMLInputElement], expected [object XULElement]

Comment 41

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1248393507.1248399099.20995.gz
Linux comm-1.9.1 unit test on 2009/07/23 16:58:27

... | tab key to tab activeElement - Got browser, expected tab
... | tab key to tab activeElement - Got [object XULElement], expected [object XULElement]
}

Already much time spent on this and no real solution, (I even wonder whether it got worse).

Still suggesting a todo() for the time being.!.

Comment 42

[[e-mail]]

Comment on attachment 386915 [details] [diff] [review]
(Bv1a) Use todo() ftb

Assuming the test really is intermittent, I would like an easier way to notice that the test passed or failed without reading the log. Two alternatives spring to mind: a) if (!linux || document.activeElement == tab1) is(document.activeElement, tab1, ...); b) if (!linux) is(document.activeElement, tab1, ...); else if (document.activeElement != tab1) todo_is(document.activeElement, tab1, ...);

Comment 43

[[e-mail]]

Also we should back out all the hacks, since they weren't needed on Mac/Win.

Comment 44

[Serge Gautherie (:sgautherie)]

(In reply to comment #41)
> I even wonder whether it got worse.

(In reply to comment #42)
> Assuming the test really is intermittent

It is (still), though "lately" failure is the rule and success the exception:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1249506837.1249513101.1291.gz
Linux comm-1.9.1 unit test on 2009/08/05 14:13:57
mochitest-browser-chrome 444/2/8

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.0/1249513083.1249518978.32511.gz
Linux comm-1.9.1 unit test on 2009/08/05 15:58:03
mochitest-browser-chrome 446/0/8

Comment 45

[Serge Gautherie (:sgautherie)]

Comment on attachment 387183 [details] [diff] [review]
Extra diagnostic
[Checkin: Comment 45]


http://hg.mozilla.org/comm-central/rev/1a171a63a9a3

Comment 46

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2) Undo 2 non-helping hacks, Use todo_is() ftb

Bv1a, with comment 43 and comment 43 suggestion(s).

Past: I assume executeSoon() would have made no difference.
Future: if todo_is() does intermittently fail then we'll know it's a timing issue.

Comment 47

[[e-mail]]

Do we even need the 3_5 hack?

Comment 48

[Serge Gautherie (:sgautherie)]

Attachment: (Bv2a) Undo 3 non-helping hacks, Use todo_is() ftb [Checkin: Comment 50]

Bv2, plus undo bug 488025 hack too.

Comment 49

[[e-mail]]

Comment on attachment 393159 [details] [diff] [review]
(Bv2a) Undo 3 non-helping hacks, Use todo_is() ftb
[Checkin: Comment 50]

>+    // Check local name too to help diagnose bug 491624.
>+    todo_is(document.activeElement.localName, "tab",
>+            "tab key to tab activeElement (bug 491624: name = " +
>+              document.activeElement.localName + ")");
>+    todo_is(document.activeElement, tab1,
>+            "tab key to tab activeElement (bug 491624: object = " +
>+              document.activeElement + ")");
It's annoying that todo_is doesn't do that for you :-(

Comment 50

[Serge Gautherie (:sgautherie)]

Comment on attachment 393159 [details] [diff] [review]
(Bv2a) Undo 3 non-helping hacks, Use todo_is() ftb
[Checkin: Comment 50]


http://hg.mozilla.org/comm-central/rev/9baa4ac307b3


(In reply to comment #49)
> It's annoying that todo_is doesn't do that for you :-(

Yeah, no support for "random" behaviors in the mochitest (SimpleTest) harness:
Maybe this is not as needed as for the reftests?

Comment 51

[Serge Gautherie (:sgautherie)]

Fwiw, see bug 507046 as a possible workaround...

Comment 52

[Robert Kaiser]

(In reply to comment #51)
> Fwiw, see bug 507046 as a possible workaround...

Doesn't make much sense, as what we want to test is that navigation with the tab key gets us to the correct tab.

Comment 53

[Serge Gautherie (:sgautherie)]

Fwiw, note
http://hg.mozilla.org/comm-central/diff/e29bae488b4d/suite/browser/test/browser/browser_bug462289.js
we do have some "timing weirdness" here...

Comment 54

[Robert Kaiser]

Does this still happen?

I think I give up on it as long as I have no clue what's going on there.

Comment 55

[Serge Gautherie (:sgautherie)]

(In reply to comment #54)
> Does this still happen?

I don't know: I'm not checking random failures anymore until bug 534647 is fixed :-|

Comment 56

[Ian Neal]

Attachment: Port changes from FF patch v0.1

This patch:
* Ports changes made in Firefox's version of this test and removed all todos.

Comment 57

[[e-mail]]

Comment on attachment 471305 [details] [diff] [review]
Port changes from FF patch v0.1

>-  tab1 = gBrowser.addTab();
>-  tab2 = gBrowser.addTab();
>+  tab1 = gBrowser.addTab("about:blank", {skipAnimation: true});
>+  tab2 = gBrowser.addTab("about:blank", {skipAnimation: true});
We don't actually support animation...

>-  is(gBrowser.selectedTab, tab1, "mouse on tab selects tab");
Why has this been removed?

>   document.getElementById("urlbar").inputField.focus();
>-  EventUtils.synthesizeKey("VK_TAB", { });
>+  while (focus_in_navbar())
>+    EventUtils.synthesizeKey("VK_TAB", { });
Out of interest, do we still need the .inputField. bit?

Comment 58

[Ian Neal]

(In reply to comment #57)
> Comment on attachment 471305 [details] [diff] [review]
> Port changes from FF patch v0.1
> 
> >-  tab1 = gBrowser.addTab();
> >-  tab2 = gBrowser.addTab();
> >+  tab1 = gBrowser.addTab("about:blank", {skipAnimation: true});
> >+  tab2 = gBrowser.addTab("about:blank", {skipAnimation: true});
> We don't actually support animation...
Hmmm, I think I got confused about which bindings we extend and which we replace.
> 
> >-  is(gBrowser.selectedTab, tab1, "mouse on tab selects tab");
> Why has this been removed?
To match toolkit, but I can re-instate.
> 
> >   document.getElementById("urlbar").inputField.focus();
> >-  EventUtils.synthesizeKey("VK_TAB", { });
> >+  while (focus_in_navbar())
> >+    EventUtils.synthesizeKey("VK_TAB", { });
> Out of interest, do we still need the .inputField. bit?
I will double check.
Somehow I managed to upload an old version of the patch, so will upload an updated one tomorrow.

Comment 59

[Ian Neal]

Attachment: Port changes from FF patch v0.2 [Checkin: Comment 62]

Changes since v0.1:
* Removed animation changes;
* Re-added "mouse on tab selects tab" code;
* Fixed last todo - this works on linux, might need tweaking for other platforms.

Comment 60

[[e-mail]]

Comment on attachment 471469 [details] [diff] [review]
Port changes from FF patch v0.2 [Checkin: Comment 62]

I happened to notice that some of the todo_is are now isnot. Does that mean that we've decided that the tested behaviour is now correct?

Comment 61

[Ian Neal]

(In reply to comment #60)
> Comment on attachment 471469 [details] [diff] [review]
> Port changes from FF patch v0.2
> 
> I happened to notice that some of the todo_is are now isnot. Does that mean
> that we've decided that the tested behaviour is now correct?

Well it matches what Firefox have as the result of tested behaviour...

Comment 62

[Ian Neal]

Comment on attachment 471469 [details] [diff] [review]
Port changes from FF patch v0.2 [Checkin: Comment 62]

http://hg.mozilla.org/comm-central/rev/4b9cb609548d

Comment 63

[Serge Gautherie (:sgautherie)]

Good :-) Would want this test update on c-1.9.1 too...

Comment 64

[Ian Neal]

(In reply to comment #63)
> Good :-) Would want this test update on c-1.9.1 too...

Could we get a tracking bug to cover those that we want to backport to c-1.9.1 so that we don't forget them?

Comment 65

[Serge Gautherie (:sgautherie)]

(In reply to comment #64)

Why not use this very bug?