Last Comment Bug 630140 - Port |Bug 595652 - fix hit testing for border-radius| to SeaMonkey
: Port |Bug 595652 - fix hit testing for border-radius| to SeaMonkey
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Serge Gautherie (:sgautherie)
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 595652
Blocks: 462289 597584 599745 706149
  Show dependency treegraph
 
Reported: 2011-01-31 01:34 PST by Serge Gautherie (:sgautherie)
Modified: 2012-08-01 20:37 PDT (History)
7 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1-WIP1) Just copy it (2.44 KB, patch)
2011-01-31 02:12 PST, Serge Gautherie (:sgautherie)
iann_bugzilla: feedback-
Details | Diff | Splinter Review
(Bv1-FF) Add little delays to be surer, Close tabs in reverse order (2.38 KB, patch)
2011-02-12 09:06 PST, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv2-FF) Add little delays to be surer, Add/Update some checks, Fix some nits (4.30 KB, patch)
2011-02-12 15:42 PST, Serge Gautherie (:sgautherie)
dao+bmo: review-
Details | Diff | Splinter Review
(Av2-SM) Port bug 595652, Add needed little delays, Add/Update some checks, Fix some nits [Checked in: Comment 20] (4.14 KB, patch)
2011-02-12 15:57 PST, Serge Gautherie (:sgautherie)
iann_bugzilla: review+
Details | Diff | Splinter Review
(Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too [Checked in: Comment 17] (2.25 KB, patch)
2011-02-13 03:36 PST, Serge Gautherie (:sgautherie)
neil: review+
iann_bugzilla: review+
misak.bugzilla: feedback+
Details | Diff | Splinter Review
(Dv1-SM) Increase delay to 250 ms from 25 [Checked in: See comment 30+31] (3.24 KB, patch)
2011-02-28 06:17 PST, Serge Gautherie (:sgautherie)
iann_bugzilla: review+
Details | Diff | Splinter Review
(Bv3-FF) Add/Update some checks, Fix some nits [Moved to bug 706149] (4.04 KB, patch)
2011-03-08 15:34 PST, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2011-01-31 01:34:35 PST

    
Comment 1 Serge Gautherie (:sgautherie) 2011-01-31 02:12:37 PST
Created attachment 508353 [details] [diff] [review]
(Av1-WIP1) Just copy it

Switching the last occurrence to synthesizeMouseAtCenter(), instead of '9, 30' causes
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/br
owser_bug462289.js | tab not focused via middle click - Didn't expect [object XU
LElement], but got it
}
:-<
Comment 2 neil@parkwaycc.co.uk 2011-01-31 02:55:16 PST
Comment on attachment 508353 [details] [diff] [review]
(Av1-WIP1) Just copy it

Not sure what that's trying to test - tab2 was the activeElement and presumably therefore the selected tab and is thus always focusable.
Comment 3 Ian Neal 2011-02-08 07:42:54 PST
(In reply to comment #2)
> Comment on attachment 508353 [details] [diff] [review]
> (Av1-WIP1) Just copy it
> 
> Not sure what that's trying to test - tab2 was the activeElement and presumably
> therefore the selected tab and is thus always focusable.

Doesn't middle click on a tab do different things for FF and SM?
For FF it closes the tab you're middle clicking on, for SM it tries to load what is in the copy/paste buffer as a URL
Comment 4 Ian Neal 2011-02-10 17:43:58 PST
Comment on attachment 508353 [details] [diff] [review]
(Av1-WIP1) Just copy it

>-  EventUtils.synthesizeMouse(tab2, 9, 30, {button: 1, type: "mousedown"});
>+//  EventUtils.synthesizeMouse(tab2, 9, 30, {button: 1, type: "mousedown"});
>+  EventUtils.synthesizeMouseAtCenter(tab2, {button: 1, type: "mousedown"});
Remove the commented out line.

> function step6()
> {
>-  isnot(document.activeElement, tab2, "tab not focused via middle click");
>+  todo_isnot(document.activeElement, tab2, "tab not focused via middle click");
>+//  isnot(document.activeElement, tab2, "tab not focused via middle click");
As we don't seem to do the same thing for middle click as FF, what we get should be different, so the test/result should be amended - different button maybe?
Comment 5 Serge Gautherie (:sgautherie) 2011-02-12 08:48:07 PST
(In reply to comment #2)
> Not sure what that's trying to test - tab2 was the activeElement and presumably
> therefore the selected tab and is thus always focusable.

I tried to reproduce this test manually in both FF and SM.

At end of step 2, there is a click on tab1 which is already active;
on FF, this doesn't change anything: focus remains in the urlbar;
on SM, focus moves from the urlbar to tab1 itself.
Step 3 then does |document.getElementById("urlbar").focus();| (to resync').
This test currently passes, but after adding a little delay before step 2 the activeElement check fails :->

Something similar happens at step 5:
tab2 is active, step 5 does |content.focus();| then middle-click tab2.
FF: unchanged; SM: focus moves to tab2 (hence the step 6 check failure).

Iiuc, either SeaMonkey should stop focusing its tabs or this test should take that "specific" behavior into account.
Neil?


(In reply to comment #3)
> Doesn't middle click on a tab do different things for FF and SM?
> For FF it closes the tab you're middle clicking on, for SM it tries to load
> what is in the copy/paste buffer as a URL

Not on my Windows 2000:
new profiles, FF and SM both close the tab I'm middle-clicking on (at the center or not).

Moreover, step 5 sends a "mousedown" only, no "mouseup" so no tab closing.
Comment 6 Serge Gautherie (:sgautherie) 2011-02-12 09:06:20 PST
Created attachment 511962 [details] [diff] [review]
(Bv1-FF) Add little delays to be surer, Close tabs in reverse order

This doesn't change anything atm: it just makes sure not to miss a comment 5 -like issue. (Hoping 25ms is enough in "all" cases...)
Comment 7 Serge Gautherie (:sgautherie) 2011-02-12 09:09:09 PST
(In reply to comment #5)
> Iiuc, either SeaMonkey should stop focusing its tabs or this test should take
> that "specific" behavior into account.
> Neil?

Ftr, (FF) |Bug 462289 - Clicking the selected tab should not focus the tab bar|.
Comment 8 Serge Gautherie (:sgautherie) 2011-02-12 09:28:54 PST
(In reply to comment #1)
> Switching the last occurrence to synthesizeMouseAtCenter(), instead of '9, 30'

Fwiw, I still can't figure out why step 6 check always passes with '9, 30' :-<
Comment 9 neil@parkwaycc.co.uk 2011-02-12 10:48:54 PST
(In reply to comment #7)
> (In reply to comment #5)
> > Iiuc, either SeaMonkey should stop focusing its tabs or this test should take
> > that "specific" behavior into account.
> > Neil?
> Ftr, (FF) |Bug 462289 - Clicking the selected tab should not focus the tab
> bar|.
No, I don't want to port that.
Comment 10 Serge Gautherie (:sgautherie) 2011-02-12 15:42:45 PST
Created attachment 511993 [details] [diff] [review]
(Bv2-FF) Add little delays to be surer, Add/Update some checks, Fix some nits

Av1-FF, enhanced.

You may want to investigate/fix my 'XXX' comment based on observed behavior on my Windows 2000.
Comment 11 Serge Gautherie (:sgautherie) 2011-02-12 15:57:44 PST
Created attachment 511994 [details] [diff] [review]
(Av2-SM) Port bug 595652, Add needed little delays, Add/Update some checks, Fix some nits
[Checked in: Comment 20]

Av1-WIP1, sync'ed with Bv2-FF,
taking SMvsFF differences.
Comment 12 Serge Gautherie (:sgautherie) 2011-02-13 03:36:43 PST
Created attachment 512020 [details] [diff] [review]
(Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too
[Checked in: Comment 17]

(Nit) Fixes to bug 597584 and bug 599745.
Comment 13 Misak Khachatryan 2011-02-13 10:49:37 PST
Comment on attachment 512020 [details] [diff] [review]
(Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too
[Checked in: Comment 17]

>-    gBrowser.selectedTab  = gBrowser.addTab("about:blank", {skipAnimation: true});
>+    // Ftr, SeaMonkey doesn't support animation (yet).
>+    gBrowser.selectedTab  = gBrowser.addTab("about:blank");
> 

Neil asks me to use getBrowser() instead of gBrowser, as our gBrowser is a little bit different.
Comment 14 Neil Deakin (away until Oct 3) 2011-02-15 08:12:19 PST
Does executeSoon work better that having to use timeouts?
Comment 15 Serge Gautherie (:sgautherie) 2011-02-15 15:47:34 PST
(In reply to comment #14)
> Does executeSoon work better that having to use timeouts?

I don't think so in this file: the goal is not (only) to complete running the current test before something else runs, but rather to do "ensure" every(possible)thing has run before the next test starts.
Comment 16 Serge Gautherie (:sgautherie) 2011-02-15 15:54:58 PST
(In reply to comment #15)
> (In reply to comment #14)
> > Does executeSoon work better that having to use timeouts?
> 
> I don't think so in this file

As least, executeSoon() is no better than setTimeout(,0) with SeaMonkey on my Windows wrt comment 5.
Comment 17 Serge Gautherie (:sgautherie) 2011-02-15 16:08:21 PST
Comment on attachment 512020 [details] [diff] [review]
(Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/f06a82f647b9
Comment 18 Serge Gautherie (:sgautherie) 2011-02-15 16:36:02 PST
(In reply to comment #13)
> Neil asks me to use getBrowser() instead of gBrowser, as our gBrowser is a
> little bit different.

If we do want to do that, I assume that would deserve its own bug:
http://mxr.mozilla.org/comm-central/search?string=%5B%5Ea-z%5DgBrowser%5B%5EA-Z%5D&regexp=1&case=1&find=%2Fsuite%2F
"Found 315 matching lines in 42 files"
Comment 19 Ian Neal 2011-02-21 17:09:54 PST
Comment on attachment 511994 [details] [diff] [review]
(Av2-SM) Port bug 595652, Add needed little delays, Add/Update some checks, Fix some nits
[Checked in: Comment 20]

>-  tab1 = gBrowser.addTab();
>-  tab2 = gBrowser.addTab();
>+  // Ftr, SeaMonkey doesn't support animation (yet).
>+  tab1 = gBrowser.addTab("about:blank");
>+  tab2 = gBrowser.addTab("about:blank");
The behaviour is the same for no argument as it is for "about:blank", so no real need to change it.

r=me with either.
Comment 20 Serge Gautherie (:sgautherie) 2011-02-21 17:30:29 PST
Comment on attachment 511994 [details] [diff] [review]
(Av2-SM) Port bug 595652, Add needed little delays, Add/Update some checks, Fix some nits
[Checked in: Comment 20]

http://hg.mozilla.org/comm-central/rev/c8ef067543ad


(In reply to comment #19)
> The behaviour is the same for no argument as it is for "about:blank", so no
> real need to change it.

I know: I preferred to keep code closer to FF test.
Comment 21 Serge Gautherie (:sgautherie) 2011-02-28 05:19:31 PST
Linux and Windows (at least) report
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/browser/test/browser_bug462289.js | 2nd click on selected tab1 activates tab - Got [object XULElement @ 0x9bdda00 (native @ 0x9afe460)], expected [object XULElement @ 0x9be3f88 (native @ 0x9608ca0)]
}
Comment 22 Dão Gottwald [:dao] 2011-02-28 05:28:20 PST
Comment on attachment 511993 [details] [diff] [review]
(Bv2-FF) Add little delays to be surer, Add/Update some checks, Fix some nits

>-  setTimeout(step2, 0);
>+  // Here and after, don't use a delay of '0': give a little more time to be sure UI has fully updated.
>+  setTimeout(step2, 25);

What problem are you trying to solve here exactly?
Comment 23 Serge Gautherie (:sgautherie) 2011-02-28 06:17:43 PST
Created attachment 515598 [details] [diff] [review]
(Dv1-SM) Increase delay to 250 ms from 25
[Checked in: See comment 30+31]

+/- expected: I guess our Dbg slaves are slower than my Opt Windows 2000.

Ftr, 10 was not enough when I tested locally.
Comment 24 Dão Gottwald [:dao] 2011-02-28 06:21:38 PST
Comment on attachment 511993 [details] [diff] [review]
(Bv2-FF) Add little delays to be surer, Add/Update some checks, Fix some nits

Looks like SeaMonkey has a problem with the zero-timeout that Firefox doesn't have. I don't see why the test for Firefox should accommodate this.
Comment 25 Serge Gautherie (:sgautherie) 2011-02-28 07:03:58 PST
(In reply to comment #22)
> What problem are you trying to solve here exactly?

This test checks where focus is,
but, from what I observed with SeaMonkey, a delay of '0' is not enough to let UI fully update,
hence FF (would) always succeeds and SM always fails, which is a broken check for both applications.

(In reply to comment #24)
> Looks like SeaMonkey has a problem with the zero-timeout that Firefox doesn't
> have. I don't see why the test for Firefox should accommodate this.

I assumed the UI update "synchronization" issue was in the test, not in SeaMonkey, so Firefox test should be updated too.
Are you saying that Firefox test with current setTimeout(0) would be able to catch a focus regression?
If yes, would you have any idea what makes UI update different on SeaMonkey?
Comment 26 Dão Gottwald [:dao] 2011-02-28 07:09:38 PST
(In reply to comment #25)
> Are you saying that Firefox test with current setTimeout(0) would be able to
> catch a focus regression?

I wouldn't rule it out. Hard to tell without knowing what's currently going on with SeaMonkey.

> If yes, would you have any idea what makes UI update different on SeaMonkey?

No idea.
Comment 27 Justin Wood (:Callek) 2011-03-02 23:16:47 PST
(In reply to comment #21)
> Linux and Windows (at least) report
> {
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/suite/browser/test/browser_bug462289.js |
> 2nd click on selected tab1 activates tab - Got [object XULElement @ 0x9bdda00
> (native @ 0x9afe460)], expected [object XULElement @ 0x9be3f88 (native @
> 0x9608ca0)]
> }

before I am happy to take a simple timeout change here, what changed in m-c (*or* us) during this timeframe from "passing" to "failing".
Comment 28 Serge Gautherie (:sgautherie) 2011-03-03 00:17:48 PST
(In reply to comment #27)
> what changed in m-c
> (*or* us) during this timeframe from "passing" to "failing".

Nothing changed: this check has been failing since the changeset landed.
Comment 29 Ian Neal 2011-03-07 14:17:12 PST
Comment on attachment 515598 [details] [diff] [review]
(Dv1-SM) Increase delay to 250 ms from 25
[Checked in: See comment 30+31]

This does seem a big jump, could we start smaller (100 maybe) and increase until we get a constant green?
r=me with that answered/fixed.
Comment 30 Serge Gautherie (:sgautherie) 2011-03-08 03:35:36 PST
Comment on attachment 515598 [details] [diff] [review]
(Dv1-SM) Increase delay to 250 ms from 25
[Checked in: See comment 30+31]

http://hg.mozilla.org/comm-central/rev/2bb4e4f86e8d
Dv1, with comment 29 suggestion(s).
Comment 31 Serge Gautherie (:sgautherie) 2011-03-08 07:13:54 PST
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1299592533.1299595426.21028.gz
Linux comm-central-trunk debug test mochitest-other on 2011/03/08 05:55:33
still failed.

***

http://hg.mozilla.org/comm-central/rev/22e2a5cf34ab
(Ev1-SM) Increase delay to 250 ms from 100

Let's try to turn green first...
Comment 32 Serge Gautherie (:sgautherie) 2011-03-08 15:13:49 PST
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1299608236.1299610701.19157.gz
Linux comm-central-trunk debug test mochitest-other on 2011/03/08 10:17:16
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1299610302.1299613895.32705.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2011/03/08 10:51:42

V.Fixed
Comment 33 Serge Gautherie (:sgautherie) 2011-03-08 15:34:21 PST
Created attachment 517902 [details] [diff] [review]
(Bv3-FF) Add/Update some checks, Fix some nits
[Moved to bug 706149]

Bv2-FF, without increased delay :-|
Comment 34 Serge Gautherie (:sgautherie) 2011-03-08 16:14:35 PST
2 more green Linux runs, but 1 orange Windows run:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1299622063.1299626155.16075.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2011/03/08 14:07:43

I'll increase the delay some more if I notice this random-orange (too) often...
Comment 35 Serge Gautherie (:sgautherie) 2011-03-13 11:41:18 PDT
Windows orange happens quite often.

http://hg.mozilla.org/comm-central/rev/06d9e044fd7a
(Fv1-SM) Increase delay to 375 ms from 250
Comment 36 Philip Chee 2011-03-24 22:12:26 PDT
Comment on attachment 517902 [details] [diff] [review]
(Bv3-FF) Add/Update some checks, Fix some nits
[Moved to bug 706149]

Removing review request to take this bug off SeaMonkey bug review queries. Please file a separate bug in the Firefox component.
Comment 37 Philip Chee 2011-11-29 02:08:36 PST
Comment on attachment 512020 [details] [diff] [review]
(Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too
[Checked in: Comment 17]

Cancelling feedback request since patch has been checked in months ago.

Note You need to log in before you can comment on or make changes to this bug.