Closed Bug 518545 Opened 13 years ago Closed 12 years ago
middle click on scrollbar opens selected message in a new tab
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168pre) Gecko/20090903 SeaMonkey/2.0b2 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124pre) 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:126.96.36.199pre) Gecko/20090924 SeaMonkey/2.0pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) 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
Also happens in safe-mode. But I do have browser.tabs.opentabfor.middleclick set to true.
Sorry, I'm confused doublclick and middleclick. I can confirm it with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:184.108.40.206pre) Gecko/20090925 Lightning/1.0pre SeaMonkey/2.0pre
Status: UNCONFIRMED → NEW
Ever confirmed: true
Not blocking on this, but a patch would be welcome.
Kairo, bug is on Shredder today version too. Move to Core Component?
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.
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.
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.
Folderpane scrollbar clicks were already ignored, adding threadpane, also fixing folder selection.
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.
(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. :)
(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.
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 ;-)
(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 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+
(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
Closed: 12 years ago
Resolution: --- → FIXED
If you ever plan to issue new 2.0 version, please backport this change to 2.0. Thank you
You need to log in before you can comment on or make changes to this bug.