196974 - Cannot open attachments in new window or tab.

Status: NEW

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[John Malmberg]

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.

Comment 1

[Matthias Versen [:Matti]]

valid as RFE (found only bug 142966)

Comment 2

[Colin Blake]

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.

Comment 3

[Mike Cowperthwaite]

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

Comment 4

[Mike Cowperthwaite]

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

Comment 5

[Manuel Reimer]

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

Comment 6

[Manuel Reimer]

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

[Mike Cowperthwaite]

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.

Comment 8

[Manuel Reimer]

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

[[e-mail]]

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.

Comment 10

[Manuel Reimer]

Attachment: 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.

Comment 11

[Manuel Reimer]

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

And please assign to me.

Comment 12

[[e-mail]]

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.

Comment 13

[Manuel Reimer]

(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".

Comment 14

[Manuel Reimer]

Attachment: 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

Comment 15

[[e-mail]]

(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

[[e-mail]]

(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

[[e-mail]]

Comment on attachment 250746 [details] [diff] [review]
Second patch

Given comment #16 and its predecessors I think IanN should review this.

Comment 18

[Robert Kaiser]

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.

Comment 19

[Manuel Reimer]

(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.

Comment 20

[Manuel Reimer]

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?

Comment 21

[Ian Neal]

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.

Comment 22

[[e-mail]]

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

Comment 23

[Manuel Reimer]

(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

Comment 24

[Manuel Reimer]

(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

[Ian Neal]

(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

[[e-mail]]

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.

Comment 27

[Manuel Reimer]

Attachment: Patch for trunk

This is the new patch for trunk

Comment 28

[Manuel Reimer]

Attachment: 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.

Comment 29

[[e-mail]]

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

Must have misclicked... meant to sr this one first ;-)

Comment 30

[Robert Kaiser]

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

[Ian Neal]

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

a=me for SM1.1.1 (2nd one that was needed)

Comment 32

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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

a=me for sm1.1.1

Comment 33

[Karsten Düsterloh]

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.