101955 - Middle-click -> Open in new window fails for URLs in Mail/News

Status: VERIFIED FIXED

(SeaMonkey :: Tabbed Browser, defect, P3)

Description

[Pratik]

Basically, I have enabled the hidden pref so that middle-click opens the link in
a new tab. Works great in Navigator but when I try to middle-click on a URL in a
Mail/News message nothing happens. I have click on it to have it open in an
existing tab.

Similar problems when I click on a Personal Toolbar Bookmarks folder and
right-click and say "Open in New Window"

Comment 1

[David Hyatt]

Personal toolbar has yet to be designed for this.  Mailnews is not going to open
links in new tabs.  It will always target and reuse a window just as it does now.

Comment 2

[Pratik]

Well, here's the thing. With the following option

user_pref("browser.tabs.opentabfor.middleclick", false);

middle-click in any mail/news URL -> link opens in new window
right click on a a bookmark link and say Open in New Window -> link opens in new
window

and if the option is set to true

middle-click in any mail/news URL -> nothing. It doesn't even open in any
existing window.
right click on a a bookmark link and say Open in New Window -> nothing happens.
Do you still think its not a bug? I mean something should happen when I right
click and say open in new window, right?  Even if Mail/News doesn't target new
tabs, at least it should open up a new window. Its not even doing that.

Comment 3

[David Hyatt]

Ah, ok.  I misunderstood.  Yes, it should at least still open in a new window.

Comment 4

[R.K.Aa.]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Brian Nesse (gone)]

Fix for bug 106159 checked in. Re-test against a weekend or Monday build.

Comment 7

[Niklas Mehner]

I'm using build 2001102721 and the bug is still there. 
Middle-Click does not work, even when browser.tabs.opentabfor.middleclick is set
to false (Middle-Click in a mail simply never works !). 

Is this a different bug or still the same !?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Cormac F]

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

Comment 10

[Eric Hedström]

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

Comment 11

[Jacek Piskozub]

If bug 107557 is a dup, this guy must be All/All. Changing.

Comment 12

[R.K.Aa.]

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

Comment 13

[Olav Vitters]

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

Comment 14

[Jerome Lacoste]

Question: is component Tabbed Browser correct? This seems to be for Mail/News.
(I am reporting bug 110065 and trying to prove it is not a duplicate of that one.)

Comment 15

[R.K.Aa.]

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

Comment 16

[Peter Trudelle]

->mozilla0.9.9/p3, since this is only seen after setting a hidden pref.

Comment 17

[Eric Hedström]

What hidden pref are you talking about? All you have to turn on is "Middle-click
or control-click of links in a Web page" on the Tabbed Browsing prefs pane.

Comment 18

[Pratik]

When the bug was filed, it was indeed a hidden pref but since then, the Tabbed
Browser panel in Preferences has landed and the pref no longer hidden.

Comment 19

[Matt Kennedy]

The middle click action to open in a new tab doesn't work in the mailnews 
window because the code that handles it calls getBrowser() which is not 
available if window is not a navigator:browser windowtype. Middle clicking on 
a link in the mailnews window when the 'browser.tabs.opentabfor.middleclick' 
pref is true causes a JS error because getBrowser is not defined. See:

http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/contentAreaClick.js#221

The absence of the 'Open Link in New Tab' item in the context menu when right 
clicking on a link in mailnews mentioned in bug 107557 (marked a dup of 
this one) is due to basically the same thing. The 'Open Link in New Window' 
item of the menu uses nsContextMenu.openLink(). If you used 
nsContextMenu.openLinkInTab() to implement the new tab item, you have the 
same problem because it calls openNewTabWith() which uses getBrowser() too.
See:

http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/contentAreaUtils.js#95

I've made a fix for this problem and will attach patches for feedback.

Comment 20

[Matt Kennedy]

Attachment: Allows mailnews to open links in new tab

Modifies contentArea JS to allow non browser windows to open links in a new tab
is the client has an open navigator window.

This was also erroneously posted to Bug 103720. Too much **** open, my
apologies. Please don't flog me :)

Comment 21

[Matt Kennedy]

Attachment: Adds menuitem to mailnew context menu to open links in new Tab

This patch adds a menu item to the mailnews context menu when right clicking on
a link that allows opening a link in a new browser tab if a navigator window is
open. It requires the changes made in patch 63611 to provide functionality. The
absence of this menuitem was noted in Bug 107557 which was marked a duplicate
of this one.

Comment 22

[David Hyatt]

You could factor this to first find an appropriate window and then only have one
copy of the code that actually opens the new tab, e.g.,

var w = (are we a nav window) ? the window : null;
if (!w) {
  find another nav window if possible.
}
if (w) {
  // code to open link in new tab in window w
}

Comment 23

[David Hyatt]

Reassigning to new component owner.

Comment 24

[R.K.Aa.]

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

Comment 25

[R.K.Aa.]

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

Comment 26

[jag (Peter Annema)]

Matt, are you interested in updating your patch per hyatt's suggestions?

Comment 27

[Matt Kennedy]

Yes, I'll factor in hyatt's suggestion and update my patch.

Comment 28

[Matt Kennedy]

Attachment: Updated patch to allow mailnews to open links in a new tab

Revised version of my patch that cleans up some code repetition per hyatt's
comments. I took it a step further and cleaned up some duplicated code in
handleLinkClick from contentAreaClick.js. This added an additional, optional
argument to openNewTabWith() from contentAreaUtils.js to receive an event
object. The only existing code I found that used this function was the browser
context menu and I found no regressions in its behavior from my changes while
testing.

This patch obsoletes patch 63611, but I don't have the priviledge to obsolete
it.

Comment 29

[Matt Kennedy]

Attachment: Updated patch to add menuitem to mailnews context menu to open links in new Tab

Updates patch 63613. Just adds a xul menuitem to the mailnews context menu.
Kept the patch separate since it effects a different module in the tree. This
update brings the patch current with the tree and fixes the element id for the
menuitem to behave with nsContextMenu (was context-openlinktab and should've
been context-openlinkintab).

Comment 30

[Travis Crump]

tried the patch, and the browser window isn't getting the focus when Open in new
tab is triggered(by either method)[an existing browser window is already open],
but this is still an improvement :)

Comment 31

[Brian Rogers]

It seems like that should follow the "Load links in background" pref.  If you
load in the background, don't focus, otherwise do.

Comment 32

[Matt Kennedy]

Attachment: Revised version of patch 68057 that deals with window focus better

New patch to handle window focus. It's a simple change that sets the focus on
the browser if the load in background pref is set to false. Thanks for catching
that guys :)

Comment 33

[jag (Peter Annema)]

1.0

Comment 34

[jag (Peter Annema)]

Matt, in your patch you need to incorporate recent changes made (e.g. the
loadInBackground) checks. Could you attach an updated patch that doesn't
(accidentily, I hope) get rid of that code?

Comment 35

[jag (Peter Annema)]

Oh, and nsbeta1. To summarize the issue, with the hidden pref to open links in a
new window on middle click, and the tabbrowser pref to open in a new tab instead
of a new window, nothing happens for mail/news, while it should either open in a
new window or find the top-most browser window and add a tab there.

Comment 36

[Peter Trudelle]

nsbeta1- per ADT triage team

Comment 37

[Peter Trudelle]

nsbeta1- per ADT triage team, ->1.2

Comment 38

[D Sen]

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

Comment 39

[R.K.Aa.]

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

Comment 40

[Tuukka Tolvanen (sp3000)]

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

Comment 41

[Tuukka Tolvanen (sp3000)]

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

Comment 42

[R.K.Aa.]

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

Comment 43

[Vadim Berezniker]

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

Comment 44

[Vadim Berezniker]

I agree with comment #14. 
The component should be changed MailNews.
Most of the duplicates are filed under MailNews.
I did not find this bug until I searched in all components, and not everyone
will do that.

I would change it myself, but I'm afraid that someone might have a good reason
why it wasn't supposed to be changed.

Either change it, or state a reason why it should not be changed.

Comment 45

[Matthias Versen [:Matti]]

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

Comment 46

[R.K.Aa.]

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

Comment 47

[Quinn Yost (mythdraug)]

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

Comment 48

[Ashley Bischoff (blog at handcoding.com)]

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

Comment 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[R.K.Aa.]

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

Comment 51

[R.K.Aa.]

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

Comment 52

[R.K.Aa.]

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

Comment 53

[R.K.Aa.]

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

Comment 54

[R.K.Aa.]

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

Comment 55

[R.K.Aa.]

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

Comment 56

[Steve Dagley]

Removing the - from the nsbeta1 keyword so this will get some review.  The
number of dupes filed should indicate it is a very commonly encountered problem.
 While it may not make the beta I think it is something that absolutely should
be addressed before MachV beta.  Note that I don't think that the
middle-click/command click shortcut for opening a new link in a tab really
applies to links in mail/news windows since they can't be tabbed.  The behavior
should be to open the link in a new window (as hyatt states in his comment #3 on
this bug).  Somemebody puuuuuuhlease make this happen so I don't curse our
product several times a day when it just ignores my attempts to open links I
receive in mail messages.

Comment 57

[jag (Peter Annema)]

Attachment: Simplest fix

This is the fix that will simply open a new window for middle-click or
ctrl-left-click in mail/news.

Comment 58

[Steve Dagley]

Comment on attachment 83444 [details] [diff] [review]
Simplest fix

r=sdagley.  This patch does EXACTLY what I expect on a command-click on a link
in a mail/news message.

Comment 59

[Peter Trudelle]

nsbeta1+/adt2 per Nav triage, to fix regression only, not to open in new tab.

Comment 60

[R.K.Aa.]

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

Comment 61

[Kai Engert [:KaiE:]]

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

Comment 62

[Caspy7]

Ummm, don't mean to be overly sarcastic, but why hasn't this landed?

Comment 63

[jag (Peter Annema)]

Because this escaped my attention.

Comment 64

[Joe Hewitt (gone)]

Comment on attachment 83444 [details] [diff] [review]
Simplest fix

sr=hewitt

Comment 65

[Greg Onufer]

That patch doesn't address the problem described in the Summary.  The same
change needs to be made at line 224 for middle-clicking to work.  Without that
second change a JavaScript error still occurs when middle-clicking on a link in
a mail message.

Comment 66

[jag (Peter Annema)]

Attachment: Better fix

I forgot the middle click case. I've factored this code out.

Comment 67

[jag (Peter Annema)]

Attachment: Simplest fix for branch (both paths)

This is the branch version of "simplest fix" that fixes both paths. "Better
fix" essentially does the same thing, except I'm moving code around a bit to
get rid of duplication.

Comment 68

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 85273 [details] [diff] [review]
Better fix

r=caillon but remove those silly comments that basically reiterate the
function's name (or at the least fix the spacing or something)

Comment 69

[Joe Hewitt (gone)]

Comment on attachment 85273 [details] [diff] [review]
Better fix

I don't like this model of falling through to openLinkInNewWindow only if
openLinkInNewTab returns false. Personally I'm a fan of being more explicit. 
I'd have one function called openLinkWithMiddleClick which checks the pref and
then calls openLinkInNewTab/Window.    The name "openLinkInNewTab" doesn't
imply that it is only used in the middle click case.
Since openLinkInNewTab/Window are both related to middle-mouse,

Comment 70

[jag (Peter Annema)]

Attachment: Fix + refactoring code (TFFKABF)

You're right.

Comment 71

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 85500 [details] [diff] [review]
Fix + refactoring code (TFFKABF)

r=caillon.  Either way is fine with me.

Comment 72

[Joe Hewitt (gone)]

Comment on attachment 85500 [details] [diff] [review]
Fix + refactoring code (TFFKABF)

sr=hewitt

Brilliant, jag

Comment 73

[jag (Peter Annema)]

Checked in the last patch. If you want to have the ability to open in new tabs
from mail/news, please file a new bug/RFE.

Comment 74

[Steve Dagley]

I would request that nobody open an RFE such as jag describes.  There is no
contextual menu option to open a link on a mail/news page in a new tab since
mail/news windows don't have tabs.  If such an option was added we'd go down the
rathole of what window ends up with the new tab.  Let's just leave it at having
a working keyboard/mouse shortcut to open the link in a new window and let the
dead snake lie.

Comment 75

[Dennis Haney]

> mail/news windows don't have tabs.

No they dont, but why shouldn't middle click open the link in a new tab in the
browser window? Personally I hate that new window that pops up...

Comment 76

[Steve Dagley]

What if I have 0 or >1 browser windows open?  Which window does the newly
created tab belong to?  Tabs are supposed to be created in the window containing
the link being clicked on.  That breaks in mail/news windows.  There is no
option to open a tab in some other browser window when I click on a link in a
browser window, just a new browser window with that link.  That's where we are
now with jag's fix to the keyboard/mouse shortcut for for mail/news windows and
that's exactly what I want/expect.  And this is my last comment on the topic
cause, like I said above, it's a rathole I don't think we should be crawling into.  

Comment 77

[Bishop Clark (LC957)]

In which window should that new tab open?  Hmm, tough question, one that's
*already been solved*!

Read this email with two other windows open, and single-click the link above in
order to go to the RFE page.  Oh, look!  It foregrounds the most recently-used
pane and opens the link within that pane.  Even better, set up for the same
test, but click the navigator icon in the bunch down at the bottom of the window
(you see it, beside ChatZilla).  Look, again!  The most recently used browser
window foregrounds!  Yay!  

Next question please.

Comment 78

[Bishop Clark (LC957)]

hmm, that's odd.  It set as re-opened.  Can someone re-close this thing?  My bad.

Comment 79

[jag (Peter Annema)]

See comment 73. Any discussion on such an RFE in this bug will be ignored.

Comment 80

[R.K.Aa.]

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

Comment 81

[Eric Vaandering (no email)]

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

Comment 82

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 85500 [details] [diff] [review]
Fix + refactoring code (TFFKABF)

please check into the 1.0.1 branch ASAP. once landed remove the 
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 83

[Peter Lairo]

RE comment #73: OK, I filed bug 148974 "Middle-click & CTRL+click on URL in
Mail/News should open a NEW TAB if "Tabbed Browsing Preference" is set to do so"

This seems to be what most people following this bug (including me) are actually
interested in. If you are interested, please CC yourself to (and vote for) bug
148974.

Matt Kennedy: Since you have been so diligent at fixing this bug, I would be
very grateful if you had interest in fixing the new bug (bug 148974). :-D

Comment 84

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
the patch in asap, then add the "fixed1.0.1" keyword.

Comment 85

[jag (Peter Annema)]

Checked in on branch.

Comment 86

[sairuh (rarely reading bugmail)]

tested the following using 2002.06.10 commercial branch and trunk builds on
linux rh7.2,
win2k and mac 10.1.5:

a. ctrl+click (cmd+click on Mac) link in mail or news msg
b. middle+click (linux and win2k) link in mail or news msg
c. (a) and (b) with the "open tabs instead of windows with middle/ctrl+click"
pref turned on or off.

results: as expected, all cases resulted in opening the link in a new browser
window.

Comment 87

[Matthias Versen [:Matti]]

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

Comment 88

[Matthias Versen [:Matti]]

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

Comment 89

[Ashley Bischoff (blog at handcoding.com)]

Reopening. Middle click now opens a new tab (instead of a new window) using
2002111404 on Win2k (I have the middle-click-opens-a-new-tab option set).

Comment 90

[Quinn Yost (mythdraug)]

Alex:  This was caused by the checkin for bug 148974.    Bug 180042 is opened to
discuss and resolve the the remaining UI issue.  

"
> in the browser I always want my tabs to open in the background,
> and in mail I almost never do.

The same here.

> What's the right solution here?  Add another pref?  Always open
> mail tabs in the foreground?

As far as I'm concerned, I would be satisfied with either solution. But if I had
to choose, I'd add a checkbox like "When triggered from outside the browser,
load links in the background" right below "Load links in the background". It's
probably not the best possible wording (and perhaps too long), someone who
actually can speak English would be a better person to formulate this. I
specifically vote against making the formulation or placement MailNews-specific,
because - I suppose - the same should apply to other not-in-browser links:
Calendar has been mentioned, and all other Mozilla components are also
candidates; and invocation by clicking a link on the desktop too.
"

Comment 91

[Dan Mosedale (:dmosedale, :dmose)]

I'm not understanding why this bug was re-opened.  As far as I can tell, the
summary does not describe the current behavior.

Comment 92

[Ashley Bischoff (blog at handcoding.com)]

Dan: I'm primarily going on comment #86 from sairuh, specifically:

"results: as expected, all cases resulted in opening the link in a new browser
window."

Comment 93

[Dan Mosedale (:dmosedale, :dmose)]

At the time comment 86 was written, that was the expected behavior.  Due to the
checkin for bug 148974, the expected behavior has changed.

Comment 94

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.03.13 comm trunk.