231984 - [REGRESSION] Internet Shortcut Uses Current Window

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[David E. Ross]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6) Gecko/20040113

With Mozilla 1.5 and prior, selecting an Internet Shortcut (extension .url)
would open a new Mozilla window.  With Mozilla 1.6, selecting an Internet
Shortcut uses the current window (current tab of the current window if multiple
tabs are open).  

This wipes out the current page.  If the shortcut leads to a login page or
results in a redirect, the page being previously viewed can be restored only
with some effort; any inputs into a form on that previous page are lost.  

Reproducible: Always

Steps to Reproduce:
1.  Create an Internet Shortcut.  
2.  Open Mozilla to a Web page.  
3.  Select the Internet Shortcut.  

Actual Results:  
The page being viewed is replaced by the page resulting from the Internet
Shortcut.  

Expected Results:  
A new window (or at least a new tab) should open to the page resulting from the
Internet Shortcut.  

Work around:  Before selecting an Internet Shortcut, open a new Mozilla window
or a new tab.  Make sure the new window or tab is the current one.

Comment 1

[Chris Conboy]

I had the same problem, which brought me to this bugzilla report. I no longer
have this problem.

FIX: I had installed the 'multizilla' ('multiview' and 'googlebox') addins,
found at http://multizilla.mozdev.org. After removing the addins I am now able
to have internet shortcut url links open mozilla in a new window.

NOTE: I installed mozilla 1.6 two days ago, have not installed any other addons
except for multizilla. While removing multizilla I ended up removing and
reinstalling mozilla 1.6. This under winxp pro. I have done no other testing on
this. But now all files I 'dbl-clk' in windos will open in a new Mozilla window
(which includes, for example, xml & html files, as well as url shortcuts, none
of these worked since installing multizilla).

Comment 2

[David E. Ross]

Another workaround is to set 
    user_pref("advanced.system.supportDDEExec", false);
in user.js.  

Note that, contrary to opinions expressed for other bugs (e.g., bug #105885),
the existence of a workaround -- especially workarounds involving extensions (as
cited in comment #1) -- does not resolve a bug.  Workarounds merely mitigate the
impact of a bug until a real resolution is implemented.  Yes, I know the other
bug I cite is for an enhancement; but if it's a valid enhancement or a real
discrepancy, users should not have to depend on an extension or user-pref
setting to resolve it.  

Comment 3

[David E. Ross]

Still a problem:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040910
The default value for advanced.system.supportDDEExec is still "true".  The
correction to this problem is to change the defaul of this pref to "false".  

The test case in the original Description is still valid.  

Comment 4

[David E. Ross]

Still a problem:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.5) Gecko/20041217

Note that bug #172962 does not apply since it was implemented only in Firefox. 
However, it would be nice if that enhancement were also implemented for the
Mozilla Suite.  It would close this bug.  

Someone please confirm this bug.  

Comment 5

[[e-mail]]

Attachment: Suite port of browser access backend

Comment 6

[Dan M]

Comment on attachment 172259 [details] [diff] [review]
Suite port of browser access backend

Well, OK, mostly. First off I'd like to whine about differences in coding style
between this version and the Firefox original. They'd be a lot easier to
compare and keep updated in the future if they were simply the same.

Some of the differences show that I was occasionally confused when writing the
Firefox version, and taken alone I applaud the simplifications. (I'm surprised
you don't have to cast into an nsIDOMChromeWindow! But is it really an
improvement to skip that? It's potentially useful as documentation, if nothing
else.) Some of the simplifications are erroneous. For example your vast
simplification of the OPEN_NEWTAB sequence will introduce bug 263689 to the
Suite and seems to arbitrarily use a different preference to control the
focusing behaviour. Oh and don't neglect the last appearance of the
browserDOMWindow property, in browser.js Shutdown(), unless you're quite
certain you can get away with it. I vote to just copy the Firefox code
verbatim, occasionally obtuse as it may be.

Not to forget the introduction of 4-space tabbing, and lines longer than 80
characters; annoying to those of us who still haven't been able to find a GUI
editor that can hold a candle to the old unix guard, and still against the
coding guidelines, I hope.

Comment 7

[[e-mail]]

(In reply to comment #6)
>First off I'd like to whine about differences in coding style
>between this version and the Firefox original. They'd be a lot easier to
>compare and keep updated in the future if they were simply the same.
I refuse to copy the bugs in the Firefox version.

>Some of the differences show that I was occasionally confused when writing the
>Firefox version, and taken alone I applaud the simplifications. (I'm surprised
>you don't have to cast into an nsIDOMChromeWindow! But is it really an
>improvement to skip that? It's potentially useful as documentation, if nothing
>else.)
I don't see people QI to nsIDOMChromeWindow to call getAttention. I don't see
why accession browserDOMWindow should be any different.

>Some of the simplifications are erroneous. For example your vast
>simplification of the OPEN_NEWTAB sequence will introduce bug 263689 to the
>Suite
I was not able to reproduce with the given testcases because they all opened in
new windows. I don't know why, but I'll copy and paste the FF code to see. Nor
was I able to reproduce with the testcases converted to <a target="_blank">.

>and seems to arbitrarily use a different preference to control the
>focusing behaviour.
We already have a focusing behaviour preference. I didn't see the need to create
a new one.

>Oh and don't neglect the last appearance of the
>browserDOMWindow property, in browser.js Shutdown(), unless you're quite
>certain you can get away with it.
Sorry, I have multiple patches applied to my tree and I have to try and cut the
relevant portions out of the diffs.

>I vote to just copy the Firefox code
>verbatim, occasionally obtuse as it may be.
Anywhere between unnecessarily complex and just plain incorrect.

>Not to forget the introduction of 4-space tabbing, and lines longer than 80
>characters; annoying to those of us who still haven't been able to find a GUI
>editor that can hold a candle to the old unix guard, and still against the
>coding guidelines, I hope.
Sorry about the 4-space tabbing, the code was originally created in a file with
a 4-space modeline and I forgot to reindent it when I copied it. I'll also
rewrap the long lines to 80 clumns for the next patch.

Comment 8

[[e-mail]]

Dan, I copied and pasted the FireFox front end code and the test case from bug
263689 still opens in new windows; any ideas?

Comment 9

[[e-mail]]

Attachment: Possible fix

Comment 10

[Dan M]

Comment 7: Dang! I see where I stand. Well, you've got three classes of change
here. Simplifications obvious in retrospect, scary omissions of code written
explicitly to fix bugs, and intentional functional alterations. None are
necessary, since the Firefox code is known to work. But if you insist, I can
work with it. The first class is fine, the second class I can accept if you'll
warrant I'll never have to come track down an introduced bug, but I can't learn
to love the third. With the preference change, you're second guessing
conclusions of a more thorough development process and introducing an
unnecessary tech support headache.

Comment 8: that problem seems likely to be caused by bug 278143.

Comment 9: If the |try| clause in tabbrowser is an alternate fix for the one
implemented by the |try| clause in browser.js, I considered that path and
decided the catch was safer downstream. I'm really not eager to start
development all over again on a piece of code that's known to work as-is.

Comment 11

[[e-mail]]

Attachment: Fixed patch

nsGlobalWindow::WindowExists() seems to return true for a null or empty window;
if this is intentional then nsGlobalWindow::OpenInternal() probably needs to
add an extra aName.IsEmpty() || test to initialize divertOpen. I was then able
to use the test case in the bug you mentioned to demonstrate the bug in my
code.

I also added the new pref in this version.

Comment 12

[[e-mail]]

(In reply to comment #10)
>scary omissions of code written explicitly to fix bugs
Sorry about that.

>and intentional functional alterations.
There aren't supposed to be any of these.

>With the preference change, you're second guessing
>conclusions of a more thorough development process and introducing an
>unnecessary tech support headache.
browser-prefs.js didn't have that preference so I just picked the nearest one.

>Comment 9: If the |try| clause in tabbrowser is an alternate fix for the one
>implemented by the |try| clause in browser.js, I considered that path and
>decided the catch was safer downstream. I'm really not eager to start
>development all over again on a piece of code that's known to work as-is.
There was method to my madness... the idea was that adding the try clause to
tabbrowser means that the URL bar holds the attempted URL. But I've just
realised that if this was useful that I can emulate it by setting
browser.userTypedValue manually. Would you like to see that change?

Comment 13

[Dan M]

Comment on attachment 172353 [details] [diff] [review]
Fixed patch

Fine. I'm still surprised you can skip the QueryInterface function and I don't
get why the prefs need to be duplicated from all.js (nor do I get why Ben put
the new one in the Firefox-only file).

Comment 14

[[e-mail]]

Comment on attachment 172353 [details] [diff] [review]
Fixed patch

You don't need to implement QueryInterface because it never gets called by the
xpconnect stub, because the interface is already known to xpconnect.

As for the prefs, they apparently don't really belong in all.js (at least
according to a comment I saw there). I forgot to diff from the correct point to
catch the changes to all.js - do you want a new patch or will you assume I'll
remember to change it?

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 172353 [details] [diff] [review]
Fixed patch

sr=jst

Comment 16

[[e-mail]]

Fix checked in.

Comment 17

[David E. Ross]

Reference comment #16:  For which version?  1.7.6?  1.8?

Comment 18

[Mike Calmus]

What preference do I need to set to get this to work? Should I be able also to
set Mozilla to open these files in a new tab? I'm using 1.8b1, but all links I
open outside Mozilla use the current window/tab.

Comment 19

[Mike Calmus]

What preference do I need to set to get this to work? Should I be able also to
set Mozilla to open these files in a new tab? I'm using 1.8b1, but all links I
open outside Mozilla use the current window/tab.

Comment 20

[[e-mail]]

(In reply to comment #19)
> What preference do I need to set to get this to work?

browser.link.open_external
1: Open in current window (default)
2: Open in new window
3: Open in new tab (this is the one you want for comment #19)

browser.link.open_newwindow
1: Open in current window
2: Open in new window (default)
3: Open in new tab

browser.link.open_newwindow.restriction
0: Divert everything (default)
1: Divert target="_blank" etc. but not window.open
2: Divert everything expect window.open with three parameters

browser.tabs.loadDivertedInBackground
true: new tabs load in the background
false: new tabs are selected

Comment 21

[irongut]

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