518545 - middle click on scrollbar opens selected message in a new tab

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Gerrit Keller]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090903 SeaMonkey/2.0b2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090903 SeaMonkey/2.0b2

with the new tabbed mail/news feature a middle click on the scrollbar opens the selected message in a new tab.

Reproducible: Always

Steps to Reproduce:
1. open a folder with lots of messages to have a scrollbar visible
2. middle click on the scrollbar body
Actual Results:  
The selected message opens in a new tab AND the scrollbar moves to the selected position.

Expected Results:  
There shouldn't open any new tabs. It should just scroll to the selected position.

this problem still exists with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090924 SeaMonkey/2.0pre

Comment 1

[lenochod]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090924 Lightning/1.0pre SeaMonkey/2.0pre

Messages is open in new window, isn't open in new tab (Win XP SP3).

Please try in Safe mode ( http://kb.mozillazine.org/Talk:Safe_Mode#Safe_Mode_in_SeaMonkey_2 ).

Comment 2

[Gerrit Keller]

Also happens in safe-mode.
But I do have browser.tabs.opentabfor.middleclick set to true.

Comment 3

[lenochod]

Sorry, I'm confused doublclick and middleclick.

I can confirm it with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090925 Lightning/1.0pre SeaMonkey/2.0pre

Comment 4

[Robert Kaiser]

Not blocking on this, but a patch would be welcome.

Comment 5

[lenochod]

Kairo, bug is on Shredder today version too. Move to Core Component?

Comment 6

[Robert Kaiser]

Not sure, if it's tabmail code, it's actually forked code, so it might need to be patched independently on both sides. I think Andrew knows the TB side of tabmail, so CCing him.

Comment 7

[Gerrit Keller]

Eh, I just realized that this happens for the folder pane's scrollbar as well. There I do get the full 3-pane view opened in a new tab.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Unless this is fixed in XUL, this is forked code, yes.  I would not be surprised if XUL changed the behavior in 1.9.2 with the focus changes but do not know.  It seems unlikely we would ever actually want events on a scrollbar to bubble and be handled.

Comment 9

[Bill Davidsen]

I think I found the cause of this. If the config "browsertabs.opentabfor.middleclick" is set true (else middle is useless) middle click on any scrollbar opens a tab. Clearly this isn't the desired behavior, it looks as if the options are tested in the wrong order. Returning the config to the default "do nothing useful" state avoids the tab opening at the expense of making the scroll operation into a keyboard (left shift) plus left click.

Please fix so this works in a useful way for both uses of middle click.

Comment 12

[Karsten Düsterloh]

Attachment: ignore certain scrollbar clicks

Folderpane scrollbar clicks were already ignored, adding threadpane, also fixing folder selection.

Comment 13

[Karsten Düsterloh]

Middle-clicking the xul:thumb gives me

JavaScript strict warning: chrome://messenger/content/msgMail3PaneWindow.js, line 1221: reference to undefined property tree.view

I'll look into that.

Comment 14

[[e-mail]]

Comment on attachment 497011 [details] [diff] [review]
ignore certain scrollbar clicks

>+  if (aEvent.originalTarget.nodeName == "xul:slider" ||
>+      aEvent.originalTarget.nodeName == "xul:scrollbarbutton")
Why are you checking the nodeName instead of the localName?

You probably want to check for "thumb" as well.

Comment 15

[Karsten Düsterloh]

Attachment: better fix

(In reply to comment #14)
> >+  if (aEvent.originalTarget.nodeName == "xul:slider" ||
> >+      aEvent.originalTarget.nodeName == "xul:scrollbarbutton")
> Why are you checking the nodeName instead of the localName?

Because it's "more correct", imo. It may be highly unlikely but still possible that non-XUL elements show up here.

> You probably want to check for "thumb" as well.

I didn't want in my first patch, because one could not drag the thumb anymore then. This version does not have that issue anymore, hence I do. :)

Comment 16

[[e-mail]]

(In reply to comment #15)
>(In reply to comment #14)
>>(From update of attachment 497011 [details] [diff] [review])
>>>+  if (aEvent.originalTarget.nodeName == "xul:slider" ||
>>>+      aEvent.originalTarget.nodeName == "xul:scrollbarbutton")
>>Why are you checking the nodeName instead of the localName?
>Because it's "more correct", imo. It may be highly unlikely but still possible
>that non-XUL elements show up here.
Note that this relies on the fact that the scrollbar XBL uses the xul: prefix.

Comment 17

[Philip Chee]

Lets do a google test:
http://mxr.mozilla.org/comm-central/search?string=.nodename
Found 638 matching lines  in 313 files
http://mxr.mozilla.org/comm-central/search?string=.localName
Found 676 matching lines  in 233 files

localName wins (just!).

Comment 18

[[e-mail]]

Actually it's just occurred to me that what we want to do is to only consider mouse events targetted at the treechildren element. (The reason we don't set the handler on the treechildren element in the first place is that we can't stop the propagation to the existing treechildren event handlers.) So maybe we should check that the localName is treechildren instead.

A second collorary is that scrollbars already call stopPropagation in their event handlers so we don't actually need to do it for them ;-)

Comment 19

[Karsten Düsterloh]

Attachment: different approach

(In reply to comment #18)
> Actually it's just occurred to me that what we want to do is to only consider
> mouse events targetted at the treechildren element.

Although that's not the whole truth - ThreadPaneOnClick does some treecol handling...

Comment 20

[[e-mail]]

Comment on attachment 497288 [details] [diff] [review]
different approach

>+  // usually, we're only interested in tree content clicks, not scrollbars etc.
[Why the "usually," ?]

>+  if (event.originalTarget.localName != "treechildren")
>+    return;
>+
>     var treeBoxObj = tree.treeBoxObject;
>     var treeSelection = tree.view.selection;
[Bah, this function is incorrectly indented, so the new code looks wrong...]

>+  let t = event.originalTarget;
>-  var t = event.originalTarget;
[Grrr!]

Comment 21

[Karsten Düsterloh]

(In reply to comment #20)
>>+  // usually, we're only interested in tree content clicks, not scrollbars etc.
> [Why the "usually," ?]

Treecol isn't exactly "content", although that's probably a call for debate... ;-)


Landed as <http://hg.mozilla.org/comm-central/rev/e2cfef804b6d>.

Comment 22

[Matus UHLAR - fantomas]

If you ever plan to issue new 2.0 version, please backport this change to 2.0.
Thank you