Closed Bug 101955 Opened 23 years ago Closed 22 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, defect, P3)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: pratik.solanki, Assigned: jag+mozilla)

References

(Depends on 1 open bug)

Details

(Whiteboard: [ADT2 rtm])

Attachments

(8 files, 1 obsolete file)

Allows mailnews to open links in new tab
4.26 KB, patch
Details | Diff | Splinter Review
Adds menuitem to mailnew context menu to open links in new Tab
891 bytes, patch
Details | Diff | Splinter Review
Updated patch to allow mailnews to open links in a new tab
4.21 KB, patch
Details | Diff | Splinter Review
Updated patch to add menuitem to mailnews context menu to open links in new Tab
893 bytes, patch
Details | Diff | Splinter Review
Revised version of patch 68057 that deals with window focus better
4.24 KB, patch
Details | Diff | Splinter Review
Simplest fix
1.26 KB, patch
sdagley
: review+
hewitt
: superreview+
Details | Diff | Splinter Review
Simplest fix for branch (both paths)
2.01 KB, patch
Details | Diff | Splinter Review
Fix + refactoring code (TFFKABF)
3.80 KB, patch
caillon
: review+
hewitt
: superreview+
jesup
: approval+
Details | Diff | Splinter Review 
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"
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.
Status: UNCONFIRMED → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
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.
Ah, ok.  I misunderstood.  Yes, it should at least still open in a new window.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → mozilla0.9.7
Component: XP Toolkit/Widgets: XUL → Tabbed Browser
*** Bug 103841 has been marked as a duplicate of this bug. ***
*** Bug 104685 has been marked as a duplicate of this bug. ***
Fix for bug 106159 checked in. Re-test against a weekend or Monday build.

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 !?

*** Bug 107480 has been marked as a duplicate of this bug. ***
*** Bug 107557 has been marked as a duplicate of this bug. ***
*** Bug 107557 has been marked as a duplicate of this bug. ***
If bug 107557 is a dup, this guy must be All/All. Changing.
OS: Linux → All
Hardware: PC → All
*** Bug 109647 has been marked as a duplicate of this bug. ***
*** Bug 110065 has been marked as a duplicate of this bug. ***
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.)
*** Bug 111016 has been marked as a duplicate of this bug. ***
->mozilla0.9.9/p3, since this is only seen after setting a hidden pref.
Priority: -- → P3
Target Milestone: mozilla0.9.7 → mozilla0.9.9
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.
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.
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.
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 :)
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.
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
}

Reassigning to new component owner.
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
*** Bug 122323 has been marked as a duplicate of this bug. ***
*** Bug 122639 has been marked as a duplicate of this bug. ***
QA Contact: jrgm → sairuh
Matt, are you interested in updating your patch per hyatt's suggestions?
Yes, I'll factor in hyatt's suggestion and update my patch.
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.
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).
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 :)
It seems like that should follow the "Load links in background" pref.  If you
load in the background, don't focus, otherwise do.

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 :)
1.0
Target Milestone: mozilla0.9.9 → mozilla1.0
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?
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.
Keywords: nsbeta1
Summary: Middle-click -> Open in new tab fails for Mail/News URLs → Middle-click -> Open in new tab/window fails for Mail/News URLs
nsbeta1- per ADT triage team
Keywords: nsbeta1 → nsbeta1-
nsbeta1- per ADT triage team, ->1.2
Target Milestone: mozilla1.0 → mozilla1.2
*** Bug 129938 has been marked as a duplicate of this bug. ***
*** Bug 129997 has been marked as a duplicate of this bug. ***
*** Bug 130758 has been marked as a duplicate of this bug. ***
*** Bug 131726 has been marked as a duplicate of this bug. ***
*** Bug 133923 has been marked as a duplicate of this bug. ***
*** Bug 131177 has been marked as a duplicate of this bug. ***
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.
*** Bug 134717 has been marked as a duplicate of this bug. ***
*** Bug 135876 has been marked as a duplicate of this bug. ***
*** Bug 137120 has been marked as a duplicate of this bug. ***
*** Bug 133188 has been marked as a duplicate of this bug. ***
*** Bug 137809 has been marked as a duplicate of this bug. ***
*** Bug 140832 has been marked as a duplicate of this bug. ***
*** Bug 141486 has been marked as a duplicate of this bug. ***
*** Bug 141541 has been marked as a duplicate of this bug. ***
*** Bug 142684 has been marked as a duplicate of this bug. ***
*** Bug 142692 has been marked as a duplicate of this bug. ***
*** Bug 143068 has been marked as a duplicate of this bug. ***
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.
Keywords: nsbeta1- → nsbeta1
Attached patch Simplest fix — — Splinter Review 
This is the fix that will simply open a new window for middle-click or
ctrl-left-click in mail/news.
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.
Attachment #83444 - Flags: review+
nsbeta1+/adt2 per Nav triage, to fix regression only, not to open in new tab.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [ADT2]
*** Bug 144420 has been marked as a duplicate of this bug. ***
*** Bug 144478 has been marked as a duplicate of this bug. ***
Whiteboard: [ADT2] → [ADT2 rtm]
Ummm, don't mean to be overly sarcastic, but why hasn't this landed?
Because this escaped my attention.
Comment on attachment 83444 [details] [diff] [review]
Simplest fix

sr=hewitt
Attachment #83444 - Flags: superreview+
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.
Attached patch Better fix (obsolete) — — Splinter Review 
I forgot the middle click case. I've factored this code out.
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 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)
Attachment #85273 - Flags: review+
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,
You're right.
Attachment #85273 - Attachment is obsolete: true
Comment on attachment 85500 [details] [diff] [review]
Fix + refactoring code (TFFKABF)

r=caillon.  Either way is fine with me.
Attachment #85500 - Flags: review+
Comment on attachment 85500 [details] [diff] [review]
Fix + refactoring code (TFFKABF)

sr=hewitt

Brilliant, jag
Attachment #85500 - Flags: superreview+
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.
Status: NEW → RESOLVED
Closed: 23 years ago → 22 years ago
Keywords: adt1.0.1, mozilla1.0.1
Resolution: --- → FIXED
Summary: Middle-click -> Open in new tab/window fails for Mail/News URLs → Middle-click -> Open in new window fails for URLs in Mail/News
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.
> 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...
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.  
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
hmm, that's odd.  It set as re-opened.  Can someone re-close this thing?  My bad.
See comment 73. Any discussion on such an RFE in this bug will be ignored.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
*** Bug 148100 has been marked as a duplicate of this bug. ***
*** Bug 148776 has been marked as a duplicate of this bug. ***
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
Attachment #85500 - Flags: approval+
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
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.
Keywords: adt1.0.1 → adt1.0.1+
Checked in on branch.
Keywords: mozilla1.0.1+ → fixed1.0.1
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.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
*** Bug 156531 has been marked as a duplicate of this bug. ***
*** Bug 158556 has been marked as a duplicate of this bug. ***
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).
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
"
Depends on: 180042
QA Contact: sairuh → pmac
I'm not understanding why this bug was re-opened.  As far as I can tell, the
summary does not describe the current behavior.
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."
At the time comment 86 was written, that was the expected behavior.  Due to the
checkin for bug 148974, the expected behavior has changed.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
QA Contact: pmac → sairuh
vrfy'd fixed with 2003.03.13 comm trunk.
Status: RESOLVED → VERIFIED
Product: Core → SeaMonkey
Depends on: 1005452
No longer depends on: 1005452
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: