middle click on scrollbar opens selected message in a new tab

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Gerrit Keller, Assigned: Karsten Düsterloh)

Tracking

Trunk
seamonkey2.1b2
Bug Flags:
wanted-seamonkey2.0 +
blocking-seamonkey2.0 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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

8 years ago
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 ).
Version: unspecified → Trunk
(Reporter)

Comment 2

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

Comment 3

8 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-seamonkey2.0?
Flags: blocking-seamonkey2.0?

Comment 4

8 years ago
Not blocking on this, but a patch would be welcome.
Flags: wanted-seamonkey2.0?
Flags: wanted-seamonkey2.0+
Flags: blocking-seamonkey2.0?
Flags: blocking-seamonkey2.0-

Comment 5

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

Updated

8 years ago
OS: Linux → All
Hardware: x86 → All

Comment 6

8 years ago
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.
(Reporter)

Comment 7

8 years ago
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.
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

8 years ago
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.

Updated

7 years ago
Duplicate of this bug: 550060
(Assignee)

Updated

7 years ago
Duplicate of this bug: 564218
(Assignee)

Comment 12

7 years ago
Created attachment 497011 [details] [diff] [review]
ignore certain scrollbar clicks

Folderpane scrollbar clicks were already ignored, adding threadpane, also fixing folder selection.
Assignee: nobody → mnyromyr
Attachment #497011 - Flags: superreview?(neil)
Attachment #497011 - Flags: review?(neil)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

7 years ago
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 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.
(Assignee)

Comment 15

7 years ago
Created attachment 497188 [details] [diff] [review]
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. :)
Attachment #497011 - Attachment is obsolete: true
Attachment #497188 - Flags: superreview?(neil)
Attachment #497188 - Flags: review?(neil)
Attachment #497011 - Flags: superreview?(neil)
Attachment #497011 - Flags: review?(neil)
(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

7 years ago
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!).
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 ;-)
(Assignee)

Comment 19

7 years ago
Created attachment 497288 [details] [diff] [review]
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...
Attachment #497188 - Attachment is obsolete: true
Attachment #497288 - Flags: review?(neil)
Attachment #497188 - Flags: superreview?(neil)
Attachment #497188 - Flags: review?(neil)
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!]
Attachment #497288 - Flags: review?(neil) → review+
(Assignee)

Comment 21

7 years ago
(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>.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
If you ever plan to issue new 2.0 version, please backport this change to 2.0.
Thank you
Target Milestone: --- → seamonkey2.1b2
You need to log in before you can comment on or make changes to this bug.