343628 - Double-clicking a close button of a tab should never open a new tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[Asaf Romano (gone)]

Seen on Windows 1.8 branch build.

1. Open two tabs
2. double click the close button of the second tab

Actual results: the second tab is closed but another tab is added as well.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm marking this as a regression, since it changes the functionality of a double click on the closebox as opposed to Firefox 1.5 and will likely be a pretty large nuisance. Not urgent enough to rush for beta1, though, but needs to be there for beta2.

Comment 2

[(not reading, please use [e-mail] instead)]

taking and setting TM to FF2B2.

I have a feeling the fix will be to this code:

http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1461

1459             if (aEvent.button == 0 &&
1460                 aEvent.originalTarget.localName == "box") {
1461               // xxx this needs to check that we're in the empty area of the tabstrip
1462               var e = document.createEvent("Events");
1463               e.initEvent("NewTab", true, true);
1464               this.dispatchEvent(e);
1465             }

Comment 3

[Phil Ringnalda (:philor)]

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

Comment 4

[Asaf Romano (gone)]

-> me since I think I know what's going on here.

Comment 5

[Asaf Romano (gone)]

Attachment: patch

hacky, but AFAICT there's nothing else we can do.

This should also fix bug 343061.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 228286 [details] [diff] [review]
patch

mconnor's on vacation, please ask someone else for review (gavin? ben?)

Comment 7

[Asaf Romano (gone)]

Attachment: patch

This actually applies.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 228337 [details] [diff] [review]
patch

Note to self:
s/both/bothe
s/browser.xml/globalBindings.xml

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 228337 [details] [diff] [review]
patch

Mano, I was unable to think of a better solution for this than using mousemove as you have done. Do you think a better solution could be found before beta 2? Also, it seems that with this patch I no longer get tab close buttons on every tab though I didn't spend any time trying to figure out why.

Comment 10

[Asaf Romano (gone)]

I was not able to think of a better solution (not even in the long term), since there are separate OS events here :-/ One might think of a blocking timer as a click handler, but i think we're going to notice all sort of too-short-timer bugs if we do so.

As for close buttons on all tabs, did you make sure the xul.css change was packaged, on which OS did you test?

Comment 11

[Asaf Romano (gone)]

(In reply to comment #9)
> (From update of attachment 228337 [details] [diff] [review] [edit])
> it seems that with this patch I no longer get tab close buttons on every
> tab though I didn't spend any time trying to figure out why.

WFM on windows (w/o setting browser.tabs.closeButtons).

Comment 12

[Robert Strong (they/them - no direct email)]

Comment on attachment 228337 [details] [diff] [review]
patch

>Index: toolkit/content/widgets/tabbrowser.xml
...
>@@ -2586,16 +2589,61 @@
...
>+            /* XXXmano hack:
>+             * Since we're removing the event target, if the user
>+             * double-clicks this button, the dblclick event will be dispatched
>+             * with the tabbar as its event target (and explicit/originalTarget),
>+             * which treats that as a mouse gesture for opening a new tab.
>+             * In this context, there is no way to prevent the dispatching
>+             * of the dblclick event, so we're manually blocking it (see
>+             * onTabBarDblClick) until the mouse is moved.
>+             */
nit: add this bug number for reference and the fixes from your comment #8

I'm not thrilled with this fix but I don't see any other way of doing this without significantly restructuring the pre-existing content and code which wouldn't be a good thing for 1.8.1 IMO. Otherwise, looks good and good job with keeping this minimal considering the circumstances.

Comment 13

[Asaf Romano (gone)]

trunk:
mozilla/toolkit/content/xul.css 1.80
mozilla/toolkit/content/widgets/tabbrowser.xml 1.165
mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.9

Comment 14

[Asaf Romano (gone)]

1.8 branch:
mozilla/toolkit/content/xul.css 1.61.2.16
mozilla/toolkit/content/widgets/tabbrowser.xml 1.103.2.54
mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.3.12.3