Cannot open attachments in new window or tab.

NEW
Assigned to

Status

SeaMonkey
MailNews: Message Display
--
enhancement
15 years ago
11 years ago

People

(Reporter: John Malmberg, Assigned: Manuel Reimer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

5.40 KB, patch
Manuel Reimer
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
5.54 KB, patch
Manuel Reimer
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; OpenVMS COMPAQ_AlphaServer_DS10_466_MHz; en-US; rv:1.3b) Gecko/20030207
Build Identifier: Mozilla/5.0 (X11; U; OpenVMS COMPAQ_AlphaServer_DS10_466_MHz; en-US; rv:1.3b) Gecko/20030207

I have mail and news configured not to automatically display images.

When I choose to view the images in the attachment window with open, an existing
browser window is hijacked and used.

The option to open in a new window is not present on the right click menu.

Reproducible: Always

Steps to Reproduce:
1.Connect to an SSL IMAP E-mail server.
2.Receive an attachment with a picture.
3.Right click on the attachment file name.
4.Click on Open.

Actual Results:  
An alert window pops up stating that I have requested an encrypted page.
A window that I wanted to be pointing to a specific web page was hijacked.
The URL field changed to the be for the picture.
No change to the display appeared.
Clicking on refresh causes the picture to be displayed.


Expected Results:  
The right click should give me an option to open in a new window.
I should not get a security alert for an SSL IMAP session.
I should be able to see the picture with out having to click on refresh.
valid as RFE (found only bug 142966)
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

15 years ago
I do not believe that this is an OpenVMS specific problem. Changing the OS from
"OpenVMS" to "all" so that people who might be able to work on this aren't
frightened away.
OS: OpenVMS → All
Hardware: DEC → All
Product: MailNews → Core

Updated

11 years ago
Assignee: mscott → nobody
QA Contact: stephend → attachments
Summary: Can not open attachments in new window. → Cannot open attachments in new window or tab.

Comment 3

11 years ago
*** Bug 260434 has been marked as a duplicate of this bug. ***

Comment 4

11 years ago
*** Bug 216073 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 5

11 years ago
*** Bug 363360 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

11 years ago
Is this really a problem related to "core" or is it a SeaMonkey problem? At least I'm unable to set target milestone to anything SeaMonkey relevant.

Maybe it's a good idea to use the value in browser.link.open_external to decide what we want to do with the attachment. Another solution may be a new pref like "browser.link.open_internal".

If someone can tell me, where I'll find the place where the attachment get's passed into the browser, I could try to fix this.

Comment 7

11 years ago
You're right, this would be better as a Suite bug, except there's really no good component under the suite to file this as.

Bug 361708 just had some work for a similar issue (getting clicked links in a mail message to follow the new window/new tab pref).  You could start by looking at the patch there.
(Assignee)

Comment 8

11 years ago
No, unfortunately this doesn't help as the whole process of opening the attachment continues on the C++ backend starting at this point:

http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/msgHdrViewOverlay.js#1010
--> messenger.openAttachment

"messenger" seems to be:

Components.classes["@mozilla.org/messenger;1"].createInstance().QueryInterface(Components.interfaces.nsIMessenger);

http://lxr.mozilla.org/seamonkey/source/mailnews/base/public/nsIMessenger.idl#105

Some time later I ended here:

http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#662

But at that point the whole thing gets too difficult for me. I don't
know what exactly happens in the backend here and where the URL finally gets passed to the last-active browser window.

Someone here who can give me a hint?

I think I could also fix this one in the JS-Part (msgHdrViewOverlay.js), but this would be a silly hack, I think ;-)

Comment 9

11 years ago
Before tabbed browsing things were nice and simple; if you had a URL you would try to open it; if it was a download you would launch the download helper otherwise if you said that you weren't a browser window then the system would find or create a browser window for you. You could of course Ctrl+Click the link which would force a new window to be created.

Of course the problems started there as windows had to explicitly exclude themselves from being browser windows to prevent links loading into them.

Mail furthered the rot by deciding that it took too long to decide whether a URL was a download or a page which was a problem because you were forced to wait for the URL to start (down)loading before you could move on to the next message. 

Now with we're continually having to add functions to the components (bookmarks, history, chatzilla, mail...) to allow links to be added in tabs. Actually clicking a link and letting itself load is practically unheard of these days.

Anyway enough of the rant; most of the time you want to call one of IanN's functions in contentAreaUtils.js on the attachment URL although watch out for the case where the attachment is a message in which case you want to open it in a message window. Note that a side-effect is that a browser window will be focused even if the attachment turns out to require an external viewer.
(Assignee)

Comment 10

11 years ago
Created attachment 249092 [details] [diff] [review]
First patch

This is my first try to fix the bug.

The modifications to the URL are the modifications also done by the "old code". I took the idea of not needing the url modifications, when not "this.isExternalAttachment", from "viewAttachment".

I'm unsure if the modifications are needed at all, as the "filename" is alredy in the URL, even without changing anything. Maybe all those modifications stuff is obsolete???

A second part of the patch is a minor modification to the file "contentAreaUtils.js", which makes the window, which opens our "external URLs", get the focus. I think this is a good idea, as you don't know where your attachment/link actually opens, if you have 5 or 6 windows open. Maybe it's a good idea to make those things pref-controllable at a later date.
Attachment #249092 - Flags: superreview?(neil)
Attachment #249092 - Flags: review?(mnyromyr)
(Assignee)

Comment 11

11 years ago
Please do the following modifications to this bug:
Product: Mozilla Application Suite
Component: MailNews.... something

And please assign to me.

Updated

11 years ago
Assignee: nobody → Manuel.Spam
Component: MailNews: Attachments → MailNews: Main Mail Window
Product: Core → Mozilla Application Suite

Comment 12

11 years ago
Comment on attachment 249092 [details] [diff] [review]
First patch

>+    var webNavigationInfo =
>+	    Components.classes["@mozilla.org/webnavigation-info;1"]
>+              .getService(Components.interfaces.nsIWebNavigationInfo);
Hmm... I'd increase a 2-space indent to 4 for a continuation; maybe 4 to 6 is ok too. But I'd align the .getService with the .classes either way.

>+        if (!this.isExternalAttachment) {
>+            url += "&type=" + this.contentType;
>+            url += "&filename=" + encodeURIComponent(this.displayName);
>+        }
>+        openAsExternal(url);
The type and filename appear to be unnecessary (particularly the filename; for some reason we get this automatically now, although we didn't use to, which explains the viewAttachment code), so you can use this.url directly.

>   // Open link in an existing window.
>   if (aType == kExistingWindow) {
>     browser.loadURI(aURL);
>     browserWin.content.focus();
>+    browserWin.focus();
Surely browserWin.content.focus(); suffices to raise the window?

>   browser.addTab(aURL, referrer, originCharset, !loadInBackground);
>+  browserWin.focus();
You shouldn't do this at all for a tab loaded in the background, and for a tab loaded in the foreground you should focus the new content frame as above.
Attachment #249092 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 13

11 years ago
(In reply to comment #12)
> (From update of attachment 249092 [details] [diff] [review])
> >+    var webNavigationInfo =
> >+	    Components.classes["@mozilla.org/webnavigation-info;1"]
> >+              .getService(Components.interfaces.nsIWebNavigationInfo);
> Hmm... I'd increase a 2-space indent to 4 for a continuation; maybe 4 to 6 is
> ok too. But I'd align the .getService with the .classes either way.

OK, this minor change should be no problem ;-)

> >+        if (!this.isExternalAttachment) {
> >+            url += "&type=" + this.contentType;
> >+            url += "&filename=" + encodeURIComponent(this.displayName);
> >+        }
> >+        openAsExternal(url);
> The type and filename appear to be unnecessary (particularly the filename; for
> some reason we get this automatically now, although we didn't use to, which
> explains the viewAttachment code), so you can use this.url directly.

I'll do so.

> >   // Open link in an existing window.
> >   if (aType == kExistingWindow) {
> >     browser.loadURI(aURL);
> >     browserWin.content.focus();
> >+    browserWin.focus();
> Surely browserWin.content.focus(); suffices to raise the window?

I don't know, but I'll try.

> >   browser.addTab(aURL, referrer, originCharset, !loadInBackground);
> >+  browserWin.focus();
> You shouldn't do this at all for a tab loaded in the background, and for a tab
> loaded in the foreground you should focus the new content frame as above.

But here I have a problem...

What will happen, if you have ten windows open. In each window you have some tabs and you don't know which window has been the last window. Now you open an URL from Mail/News, which will open in a new tab in the last active window without activating the tab. Now you are forced to search for the window, where your link had opened.

I think what we want to do is to clone the behaviour, we would get if we would open a link using an external application. I tried this using

seamonkey -remote 'openurl(http://www.seamonkey.at/)'

This seems to ignore the setting about opening tabs in background or foreground. If I open a link from external, then the tab always gets the focus and the window always gets pushed to foreground. I think we should do so, too.

It would be possible to hack something like this using the "aReverseBackgroundPref" in "openAsExternal" (this is set to "false" currently). If I pass the value of pref.getBoolPref("browser.tabs.loadInBackground") here, then we would always have "false" for "loadInBackground", as a "true" for "loadInBackground" would cause the function "openNewTabWindowOrExistingWith" to invert the value to false.

Yes, this is just a hack. Maybe it would be better to add an optional fifth paramter to "openNewTabWindowOrExistingWith" called, for example, "forceTabToForeground".
(Assignee)

Comment 14

11 years ago
Created attachment 250746 [details] [diff] [review]
Second patch

This is my second patch.

- Moved the stuff around "browser.tabs.loadInBackground" into "openNewTabWith"
- Replaced "aReverseBackgroundPref" with "aLoadInBackground" in "openNewTabWindowOrExistingWith".

Now the tab will always get the focus when the URL is opened using "openAsExternal".

- Added "browserWin.focus", again, next to "browserWin.content.focus()". This *is* required to get a focus on the browser window!

Modified my mailnews code as suggested in Comment #12
Attachment #249092 - Attachment is obsolete: true
Attachment #250746 - Flags: superreview?(neil)
Attachment #250746 - Flags: review?(neil)
Attachment #250746 - Flags: approval-seamonkey1.1?
Attachment #249092 - Flags: review?(mnyromyr)

Comment 15

11 years ago
(In reply to comment #14)
>- Added "browserWin.focus", again, next to "browserWin.content.focus()". This
>*is* required to get a focus on the browser window!
Strange; the Window menu doesn't need to do this, for instance.

Comment 16

11 years ago
(In reply to comment #13)
>This seems to ignore the setting about opening tabs in background or foreground.
We have a separate "hidden" pref for this, browser.tabs.loadDivertedInBackground

Comment 17

11 years ago
Comment on attachment 250746 [details] [diff] [review]
Second patch

Given comment #16 and its predecessors I think IanN should review this.
Attachment #250746 - Flags: review?(neil) → review?(iann_bugzilla)

Comment 18

11 years ago
Comment on attachment 250746 [details] [diff] [review]
Second patch

1.1 is done code-wise, we won't take anthing there unless it's something that breaks smoketests.
We can consider this on 1.1.1 as even though it could be seen as a feature-addition which shouldn't be taken on post-beta or even stability releases, it seems to be a fairly small code-change and can have high reward for those people who want it.
This needs to be reviewed and baked on trunk before we can consider it for any branch though.
Attachment #250746 - Flags: approval-seamonkey1.1?
Attachment #250746 - Flags: approval-seamonkey1.1.1?
Attachment #250746 - Flags: approval-seamonkey1.1-
(Assignee)

Comment 19

11 years ago
(In reply to comment #15)
> >- Added "browserWin.focus", again, next to "browserWin.content.focus()". This
> >*is* required to get a focus on the browser window!
> Strange; the Window menu doesn't need to do this, for instance.

It even doesn't work from there for me with the current trunk. Do we have a bug somewhere else, too? For me the last used browser window doesn't get the focus when using the "navigator"-Entry from Mail/News in the window menu. Tried with Linux/IceWM.

(Assignee)

Comment 20

11 years ago
Hmmm. Since my last checkout, I even not longer get a focus for "seamonkey -remote openurl". Could someone check about which checkin could have caused this problem?
(Assignee)

Updated

11 years ago
Depends on: 366365
(Assignee)

Updated

11 years ago
Depends on: 361272
No longer depends on: 366365

Comment 21

11 years ago
Comment on attachment 250746 [details] [diff] [review]
Second patch

With the changes you suggest mailWindow.js' call to the function openAsExternal would ignore the pref setting for browser.tabs.loadInBackground which I think would be wrong. So perhaps your suggestion of an additional parameter to openNewTabWindowOrExistingWith (and also openAsExternal) would be a better solution.
Attachment #250746 - Flags: review?(iann_bugzilla) → review-

Comment 22

11 years ago
(In reply to comment #19)
>Do we have a bug somewhere else, too? For me the last used browser window
>doesn't get the focus when using the "navigator"-Entry from Mail/News in the
>window menu. Tried with Linux/IceWM.
Possibly fallout from bug 330006.

(In reply to comment #21)
>(From update of attachment 250746 [details] [diff] [review])
>With the changes you suggest mailWindow.js' call to the function openAsExternal
>would ignore the pref setting for browser.tabs.loadInBackground which I think
>would be wrong.
Do you mean that pref or do you mean the one we use for real external links?
(Assignee)

Comment 23

11 years ago
(In reply to comment #21)
> (From update of attachment 250746 [details] [diff] [review])
> With the changes you suggest mailWindow.js' call to the function
> openAsExternal
> would ignore the pref setting for browser.tabs.loadInBackground which I think
> would be wrong.

It wouldn't be wrong as this is exactly what would happen when opening an URL from external.

> So perhaps your suggestion of an additional parameter to
> openNewTabWindowOrExistingWith (and also openAsExternal) would be a better
> solution.

???

I don't understand...

There is no need for any more parameters to "openAsExternal" as, as I think, this function just has to open an URL the same way as it would have been opened if opened from any external application.

But I think I should add handling for the pref, pointed out by Neil in Comment #16
(Assignee)

Comment 24

11 years ago
(In reply to comment #22)
> (In reply to comment #19)
> >Do we have a bug somewhere else, too? For me the last used browser window
> >doesn't get the focus when using the "navigator"-Entry from Mail/News in the
> >window menu. Tried with Linux/IceWM.
> Possibly fallout from bug 330006.

Definetly.

Also see bug 361272. I've added you to the CC list.

Comment 25

11 years ago
(In reply to comment #23)
> There is no need for any more parameters to "openAsExternal" as, as I think,
> this function just has to open an URL the same way as it would have been opened
> if opened from any external application.
> 
> But I think I should add handling for the pref, pointed out by Neil in Comment
> #16
> 
Yes, that is probably better, use browser.tabs.loadDivertedInBackground in openAsExternal in a similar way to what you have done for browser.tabs.loadInBackground in openNewTabWith

Comment 26

11 years ago
Comment on attachment 250746 [details] [diff] [review]
Second patch

sr=me if you add support for the external pref.

Note that bug 361272 may end up changing the way we fix the raise issue.
Attachment #250746 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 27

11 years ago
Created attachment 253073 [details] [diff] [review]
Patch for trunk

This is the new patch for trunk
Attachment #250746 - Attachment is obsolete: true
Attachment #253073 - Flags: superreview?(neil)
Attachment #253073 - Flags: review+
Attachment #250746 - Flags: approval-seamonkey1.1.1?
(Assignee)

Comment 28

11 years ago
Created attachment 253074 [details] [diff] [review]
Same patch for 1.1.1

Same patch, but in 1.1 we don't get the "filename" into URL, so I'm adding it there.
Attachment #253074 - Flags: superreview?(neil)
Attachment #253074 - Flags: review+
Attachment #253074 - Flags: approval-seamonkey1.1.1?
(Assignee)

Updated

11 years ago
Attachment #253074 - Attachment is patch: true
Attachment #253074 - Attachment mime type: application/octet-stream → text/plain

Updated

11 years ago
Attachment #253074 - Flags: superreview?(neil) → superreview+

Comment 29

11 years ago
Comment on attachment 253073 [details] [diff] [review]
Patch for trunk

Must have misclicked... meant to sr this one first ;-)
Attachment #253073 - Flags: superreview?(neil) → superreview+

Comment 30

11 years ago
Comment on attachment 253074 [details] [diff] [review]
Same patch for 1.1.1

sounds good. first-a=me for 1.1.1, still one needed to go.

Comment 31

11 years ago
Comment on attachment 253074 [details] [diff] [review]
Same patch for 1.1.1

a=me for SM1.1.1 (2nd one that was needed)
Attachment #253074 - Flags: approval-seamonkey1.1.1? → approval-seamonkey1.1.1+
Comment on attachment 253074 [details] [diff] [review]
Same patch for 1.1.1

a=me for sm1.1.1

Comment 33

11 years ago
Comment on attachment 253073 [details] [diff] [review]
Patch for trunk

Landed on trunk.
The "branch patch" for SM 1.1.1 does not apply - /suite doesn't contain the file and the xpfe version does not match.
You need to log in before you can comment on or make changes to this bug.