Closed Bug 630140 Opened 13 years ago Closed 13 years ago

Port |Bug 595652 - fix hit testing for border-radius| to SeaMonkey

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.1b3

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(3 files, 4 obsolete files)

      No description provided.
Attached patch (Av1-WIP1) Just copy it (obsolete) — Splinter Review
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
}
:-<
Attachment #508353 - Flags: feedback?(neil)
Attachment #508353 - Flags: feedback?(iann_bugzilla)
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.
(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 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?
Attachment #508353 - Flags: feedback?(iann_bugzilla) → feedback-
(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.
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...)
Attachment #511962 - Flags: review?(enndeakin)
(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|.
Blocks: 462289
(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' :-<
(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.
Av1-FF, enhanced.

You may want to investigate/fix my 'XXX' comment based on observed behavior on my Windows 2000.
Attachment #511962 - Attachment is obsolete: true
Attachment #511993 - Flags: review?(enndeakin)
Attachment #511962 - Flags: review?(enndeakin)
Attachment #511962 - Attachment description: (Av1-FF) Add little delays to be surer, Close tabs in reverse order → (Bv1-FF) Add little delays to be surer, Close tabs in reverse order
Attachment #511962 - Attachment filename: 630140-Av1-FF_delays.diff → 630140-Bv1-FF_delays.diff
Attachment #511993 - Attachment description: (Av2-FF) Add little delays to be surer, Add/Update some checks, Fix some nits → (Bv2-FF) Add little delays to be surer, Add/Update some checks, Fix some nits
Attachment #511993 - Attachment filename: 630140-Av2-FF_delays.diff → 630140-Bv2-FF_delays.diff
Av1-WIP1, sync'ed with Bv2-FF,
taking SMvsFF differences.
Attachment #508353 - Attachment is obsolete: true
Attachment #511994 - Flags: review?(iann_bugzilla)
Attachment #508353 - Flags: feedback?(neil)
(Nit) Fixes to bug 597584 and bug 599745.
Attachment #512020 - Flags: review?(neil)
Attachment #512020 - Flags: review?(iann_bugzilla)
Attachment #512020 - Flags: feedback?(misak.bugzilla)
Attachment #512020 - Flags: feedback?(aqualon)
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.
Attachment #512020 - Flags: feedback?(misak.bugzilla) → feedback+
Does executeSoon work better that having to use timeouts?
Attachment #512020 - Flags: review?(neil) → review+
Attachment #512020 - Flags: review?(iann_bugzilla) → review+
(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.
(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 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
Attachment #512020 - Attachment description: (Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too → (Cv1-SM) Remove unsupported tab 'skipAnimation' param in other tests too [Checked in: Comment 17]
(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 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.
Attachment #511994 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #511994 - Attachment description: (Av2-SM) Port bug 595652, Add needed little delays, Add/Update some checks, Fix some nits → (Av2-SM) Port bug 595652, Add needed little delays, Add/Update some checks, Fix some nits [Checked in: Comment 20]
Blocks: 597584, 599745
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b3
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)]
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
+/- expected: I guess our Dbg slaves are slower than my Opt Windows 2000.

Ftr, 10 was not enough when I tested locally.
Attachment #515598 - Flags: review?(iann_bugzilla)
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.
Attachment #511993 - Flags: review?(enndeakin) → review-
(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?
(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.
(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".
(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 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.
Attachment #515598 - Flags: review?(iann_bugzilla) → review+
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).
Attachment #515598 - Attachment description: (Dv1-SM) Increase delay to 250 ms from 25 → (Dv1-SM) Increase delay to 250 ms from 25 [Checked in: See comment 30]
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...
Attachment #515598 - Attachment description: (Dv1-SM) Increase delay to 250 ms from 25 [Checked in: See comment 30] → (Dv1-SM) Increase delay to 250 ms from 25 [Checked in: See comment 30+31]
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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Bv2-FF, without increased delay :-|
Attachment #511993 - Attachment is obsolete: true
Attachment #517902 - Flags: review?(dao)
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...
Windows orange happens quite often.

http://hg.mozilla.org/comm-central/rev/06d9e044fd7a
(Fv1-SM) Increase delay to 375 ms from 250
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.
Attachment #517902 - Flags: review?(dao)
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.
Attachment #512020 - Flags: feedback?(aqualon)
Blocks: 706149
Attachment #517902 - Attachment description: (Bv3-FF) Add/Update some checks, Fix some nits → (Bv3-FF) Add/Update some checks, Fix some nits [Moved to bug 706149]
Attachment #517902 - Attachment is obsolete: true
Blocks: 779720
No longer blocks: 779720
You need to log in before you can comment on or make changes to this bug.