Last Comment Bug 649006 - Clicking any two mouse buttons opens new tab
: Clicking any two mouse buttons opens new tab
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: x86 All
: -- normal
: Firefox 6
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 638188
  Show dependency treegraph
 
Reported: 2011-04-11 08:14 PDT by George Carstoiu
Modified: 2016-04-12 14:00 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (11.08 KB, patch)
2011-04-11 21:45 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback-
Details | Diff | Splinter Review
v2 (12.87 KB, patch)
2011-04-13 03:07 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v3 (13.15 KB, patch)
2011-04-14 10:11 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (13.13 KB, patch)
2011-04-15 14:26 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description George Carstoiu 2011-04-11 08:14:36 PDT
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] 2011-04-11 21:45:13 PDT
Created attachment 525296 [details] [diff] [review]
v1
Comment 2 Tim Taubert [:ttaubert] 2011-04-12 05:15:33 PDT
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] 2011-04-13 03:07:05 PDT
Created attachment 525651 [details] [diff] [review]
v2
Comment 4 Tim Taubert [:ttaubert] 2011-04-14 07:10:00 PDT
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] 2011-04-14 07:12:35 PDT
(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] 2011-04-14 10:11:16 PDT
Created attachment 526037 [details] [diff] [review]
v3

Updated the patch based on Tim's comments.
Comment 7 Raymond Lee [:raymondlee] 2011-04-14 18:22:13 PDT
Comment on attachment 526037 [details] [diff] [review]
v3

Passed Try!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=92a2758cceb3
Comment 8 Ian Gilman [:iangilman] 2011-04-15 10:20:38 PDT
Comment on attachment 526037 [details] [diff] [review]
v3

Looks good!
Comment 9 Raymond Lee [:raymondlee] 2011-04-15 14:26:24 PDT
Created attachment 526381 [details] [diff] [review]
Patch for checkin
Comment 11 George Carstoiu 2011-04-22 02:35:22 PDT
Verified on Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110421 Firefox/6.0a1

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