244692 - Tabs bar, status bar, Download Manager, Extensions Manager and Themes Manager respond to all kinds of double-click (left, middle, right mouse buttons)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Daniel Luz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040525 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040525 Firefox/0.8.0+

The listed components will respond to any kind of two consecutive clicks
(including mixtures like left-right-click) as a double-click event.

Components:
- Tabs bar (when you don't have a replacement like Tabbrowser Extensions)
- Download Manager
- Extensions Manager (new extensions system from nightlies)
- Themes Manager (new themes system from nightlies)

Reproducible: Always
Steps to Reproduce:
1. Perform two consecutive clicks with any mouse button, even with different
buttons, on one of these components.
Actual Results:  
Double-click behavior is done: the tabs bar will open a new tab, the download
manager will open the selected download and so on.

Expected Results:  
Ignore any form of double-click other than the ones made only with the left
mouse button.

Comment 1

[Ali Ebrahim]

Confirming on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a)
Gecko/20040517 Firefox/0.8.0+.

Bug appears to be Windows only. WFM on Linux.

Comment 2

[Daniel Luz]

Issue also found on status bar (for example to double-click the popup blocker icon)

Comment 3

[Unknown W. Brackets]

Attachment: Simplified fix for the tab bar.

This patch doesn't do much, it just makes sure that the event fires with the
left mouse button.  I'm not nimble enough to effectively test "double clicking"
with separate buttons, but this should notably solve most of the issue there.

-[Unknown]

Comment 4

[Unknown W. Brackets]

Comment on attachment 150349 [details] [diff] [review]
Simplified fix for the tab bar.

Requesting review, as suggested on mozillazine's forum, from 
[e-mail].

-[Unknown]

Comment 5

[Daniel Luz]

(In reply to comment #4)
> (From update of attachment 150349 [details] [diff] [review])
> Requesting review, as suggested on mozillazine's forum, from 
> [e-mail].
> 
> -[Unknown]
> 

Your code seems alright to me to fix the new tab behavior.
As for the double-clicking with different mouse buttons, I believe the GUI
internals should be modified, as IMHO no control should raise a double-click
event in such situation.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Some work in this area is being done by cusser in bug 246279.

Comment 7

[Ben Basson]

Attachment: Fixes some more

This should patch browser.xul for the statusbar icons and the download manager
in  XPFE and the Toolkit.

Comment 8

[Ben Basson]

<< As for the double-clicking with different mouse buttons, I believe the GUI
internals should be modified, as IMHO no control should raise a double-click
event in such situation. >>
Ideally you are right, but mconner said that it may not be feasible. In any
case, that is bug 217560 which would require more work and more complex changes.
These patches should be sufficient until a better solution can be found.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 150534 [details] [diff] [review]
Fixes some more

>Index: browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.315
>diff -u -r1.315 browser.js
>--- browser.js	1 Jun 2004 14:55:10 -0000	1.315
>+++ browser.js	11 Jun 2004 15:26:28 -0000
>@@ -2558,14 +2558,18 @@
>   document.persist(toolbar.id, "collapsed");
> }
> 
>-function displaySecurityInfo()
>+function displaySecurityInfo(aEvent)
> {
>+    if (aEvent.type == "dblclick" && aEvent.button != 0)
>+      return;

why are we checking if the event is a double click?  this function is only
called from a double click.

>-function displayPageReportFirstTime()
>+function displayPageReportFirstTime(aEvent)
> {
>+    if (aEvent.type == "dblclick" && aEvent.button != 0)
>+      return;

same here.  Also, I think you wanted displayPageReport() instead

>Index: downloadmanager.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/download-manager/resources/downloadmanager.js,v
>retrieving revision 1.35
>diff -u -r1.35 downloadmanager.js
>--- downloadmanager.js	17 Apr 2004 16:51:23 -0000	1.35
>+++ downloadmanager.js	11 Jun 2004 15:32:20 -0000
>@@ -139,7 +139,9 @@
>   window.updateCommands("tree-select");
> }
>   
>-function onDoubleClick() {
>+function onDoubleClick(aEvent) {
>+  if (aEvent.type == "dblclick" && aEvent.button != 0)
>+    return;

The function is called onDoubleClick(), you don't need the check here either :)

looks good otherwise.

Comment 10

[Ben Basson]

I think the logic was that potentially these functions could be called from
other user events, such as keypresses, which should not have a button attribute.
Since we are using that to determine whether to return; or not, it was something
which I put in to avoid potential future breakage. I can pull it out if you prefer.

As for displayPageReport(), yes, you're correct. I'll make the changes now.

Comment 11

[Ben Basson]

Attachment: Fixes some more v2

This should do it.

Comment 12

[Blake Ross]

*** This bug has been marked as a duplicate of 217560 ***

Comment 13

[Ian Neal]

As per bug 217560 comment 13

Comment 14

[Ian Neal]

*** Bug 240658 has been marked as a duplicate of this bug. ***

Comment 15

[Ian Neal]

Comment on attachment 150349 [details] [diff] [review]
Simplified fix for the tab bar.

Requesting sr for firefox only patch

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 150616 [details] [diff] [review]
Fixes some more v2

sorry this took so long.  r=[e-mail]

Comment 17

[Steffen Wilberg]

*** Bug 246279 has been marked as a duplicate of this bug. ***

Comment 18

[Steffen Wilberg]

Attachment: all-in-one

This is a unbitrotted, corrected and extended patch of all patches here and in
bug 246279.

Comment 19

[Steffen Wilberg]

Comment on attachment 150349 [details] [diff] [review]
Simplified fix for the tab bar.

no sr needed in browser or toolkit

Comment 20

[Steffen Wilberg]

Comment on attachment 153879 [details] [diff] [review]
all-in-one

Mike, you already reviewed most of the stuff, but there are some changes and
some new parts I'd like to have a review on.

>Index: mozilla/toolkit/content/widgets/tabbrowser.xml
>+ ondblclick="if (event.originalTarget.localName != 'tab')
>+               if (event.button == 0)
>+                 this.parentNode.parentNode.parentNode.selectedTab = this.parentNode.parentNode.parentNode.addTab();"

This is weird. Using && like |if (1 && 1)| results in Firefox not starting (no
XBL binding for browser).

Comment 21

[Steffen Wilberg]

Oh and cusser, your patches do not contain the file paths. I suggest to make
patches like this, from above mozilla/:
cvs diff -u8  mozilla/toolkit/mozapps/extensions/content/extensions.xul
mozilla/toolkit/mozapps/extensions/content/extensions.js
mozilla/toolkit/mozapps/downloads/content/downloads.js
mozilla/toolkit/mozapps/update/content/updates.xml
mozilla/toolkit/content/widgets/tabbrowser.xml
mozilla/browser/base/content/browser.xul mozilla/browser/base/content/browser.js
> 244692-dblclick.diff

Comment 22

[Ian Neal]

Patch v2 did contain some non-firefox fixes which, now they have been dropped
from the new patch, are over in a patch on bug 211177 (hence why there was a
dependency previously).

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 153879 [details] [diff] [review]
all-in-one

I just _know_ that'll get broken by someone :(

make that line			  
ondblclick="this.parentNode.parentNode.parentNode.onTabBarDblClick(event);"

and add the following method after reloadTab and we're golden :)

      <method name="onTabBarDblClick">
	<parameter name="aEvent"/>
	<body>
	  <![CDATA[
	     if (aEvent.originalTarget.localName != 'tab' && aEvent.button ==
0)
	       this.selectedTab = this.addTab();
	  ]]>
	</body>
      </method>

Comment 24

[Steffen Wilberg]

Attachment: as requested

done.

Comment 25

[Steffen Wilberg]

Comment on attachment 153975 [details] [diff] [review]
as requested

This is bitrotten, and I found some glitches with the security buttons. I'll
attach a new patch once bug 245406 is fixed.

Comment 26

[Steffen Wilberg]

Attachment: updated [checked in: comment 32]

Bug 245406 turned out to be unrelated.

I've moved the button check out of displaySecurityInfo() since it stopped
working somehow. The check is now in browser.xul and autocomplete.xml.

Comment 27

[Steffen Wilberg]

Comment on attachment 154453 [details] [diff] [review]
updated [checked in: comment 32]

diff -up8 for your convenience :)

Comment 28

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 154453 [details] [diff] [review]
updated [checked in: comment 32]

> function onDownloadOpen(aEvent)
> {
>+  if (aEvent.type == "dblclick" && aEvent.button != 0)
>+    return;

Why not make the attachment point use ondblclick, instead of having an onclick
listener?

Comment 29

[Steffen Wilberg]

Attachment: comment adressed

onDownloadOpen() is already called by an ondblclick listener, but the function
also triggered by the "Open" button and the context menuitem. But I moved the
button check into a new onDownloadDblClick() function, which then calls
onDownloadOpen().

Comment 30

[Steffen Wilberg]

Comment on attachment 154554 [details] [diff] [review]
comment adressed

Ben, I addressed your comment. Please have a look.

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 154554 [details] [diff] [review]
comment adressed

>+function onDownloadDblClick(aEvent)
>+{
>+  if (aEvent.button == 0)
>+    onDownloadOpen(aEvent);
>+}
>+

oh! I see what you're doing now. onDownloadOpen is called with different event
types?... lack of type info caused me to not anticipate that. Given this, I
actually prefer your first patch to adding an extra function.

r=[e-mail] on your previous patch, a=[e-mail] for branch.

Comment 32

[Steffen Wilberg]

Checked into the branch 07/29/2004 04:54.

Trunk checkin has to wait until a couple of changes to the status notification
area (especially bug 250716, but also bug 245406 and bug 253288) have landed
there. Setting dependency on bug 250716 therefore.

Removing now obsolete dependency on bug 246279.

Comment 33

[Steffen Wilberg]

Comment on attachment 154453 [details] [diff] [review]
updated [checked in: comment 32]

Noting r/a=ben.

Comment 34

[José Jeria]

*** Bug 253910 has been marked as a duplicate of this bug. ***

Comment 35

[Steffen Wilberg]

Finally checked into the trunk as well on 2004-09-19 10:31.
Removing bogus dependency on bug 250716.