649006 - Clicking any two mouse buttons opens new tab

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.2a1pre) Gecko/20110411 Firefox/4.2a1pre

Clicking any two mouse buttons (left + right, left + middle etc) opens a new empty tab

Reproducible: always

Steps to reproduce:
 1. Enter Panorama
 2. Click left and right buttons in fast succession

Actual results: 
 - a new empty tab is created

Expected results:
 - nothing should happen considering that it's not the same button that is pressed twice

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 525296 [details] [diff] [review]
v1

Sorry, but I think that is not the right place and way to fix this :)

1.) These are the lines that already fix the double click problem for other mouse buttons than the left (this concerns groupItems only).

>@@ -1670,26 +1671,29 @@ GroupItem.prototype = Utils.extend(new I
>     container.mousedown(function(e) {
>       if (!Utils.isLeftClick(e))
>         return;

2.) The same fix from (1) should go into "ui.js" somewhere at line 190+. That is the actual fix for this bug.
3.) As this bug points out with the fixes mentioned above we also consider the user made a double click when he quickly clicks left, right, left (for example). So we should make sure when the user does not click the left key we should reset all values like:

>@@ -1670,26 +1671,29 @@ GroupItem.prototype = Utils.extend(new I
>     container.mousedown(function(e) {
>      if (!Utils.isLeftClick(e) || self.$titlebar[0] == e.target ||
>          self.$titlebar.contains(e.target)) {
>         self._lastClick = 0;
>         self._lastClickPositions = null;
>         return;
>       }

4.) We need the check from (3) in ui.js, point (2), too.
5.) We should write tests for this :)

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 525651 [details] [diff] [review]
v2

Thanks for the test cleanup! Nits:

>+  showTabView(function() {
>+    contentWindow = document.getElementById("tab-view").contentWindow;
>+    test1();
>+  });

Please use TabView.getContentWindow().

>+function test1() {
>+  is(gBrowser.tabs.length, 1, 
>+     "Total number of tabs is 1 before right button double click");
>+  // first click
>+  mouseClick(contentWindow.document.getElementById("content"), 2);
>+  // second click
>+  mouseClick(contentWindow.document.getElementById("content"), 2);

We should save the #content node in a variable. Same in test2(). We could also fetch that right after TabView.getContentWindow() to make it available to all test functions.

Looks good, f+ with these nits :)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #4)
> Please use TabView.getContentWindow().
> 
> We should save the #content node in a variable. Same in test2(). We could also
> fetch that right after TabView.getContentWindow() to make it available to all
> test functions.

Please apply those changes to browser_tabview_bug604098.js, too.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v3

Updated the patch based on Tim's comments.

Comment 7

[Raymond Lee [:raymondlee]]

Comment on attachment 526037 [details] [diff] [review]
v3

Passed Try!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=92a2758cceb3

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 526037 [details] [diff] [review]
v3

Looks good!

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 11

[George Carstoiu]

Verified on Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110421 Firefox/6.0a1