206092 - New window (Ctrl+N) in Mail/News or Composer only opens a blank page

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Skewer]

Procedure: Have a home page configured. (Tested with file:///d:/html/hom.html as
homepage.) Open the mail window. With the mail window in focus, press Ctrl+N or
File>New>Navigator Window.

Expected: New window opens with home page.

Actual: New window is blank.

Build: 2003051708 WXP

Comment 1

[Jo Hermans]

Reporter, check under Preferences->Navigator. You can choose what you want for a
new window, a new tab and for the very first window that you see (Navigator
Startup).

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030517

Comment 2

[wacheo]

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

Comment 3

[Skewer]

For "New Window" I have it set to open the home page. It does that as expected
if I hit Ctrl+N in a navigator window. But it incorrectly opens a blank page if
I hit Ctrl+N in the mail window.

Comment 4

[Jo Hermans]

Confirmed using build 2003051708 on Mac OS X 10.2.6
Openening a new browser-window from the mailer, always opens with a blank page,
ignoring all settings in Preferences -> Navigator (even when you set all 3 to
your homepage).

I looked for a duplicates, but I couldn't find one.

Comment 5

[Skewer]

OS -> All. Whose component is this?

Comment 6

[Jason Bassford]

Changing component to something appropriate.  (Same as bug 201177.)

Comment 7

[Jason Bassford]

Also tweaking Summary for clarity.

Comment 8

[sairuh (rarely reading bugmail)]

hrm, could this be a regression due to the fix for bug 204157?

-> ian, but punt as needed.

Comment 9

[sairuh (rarely reading bugmail)]

per bug 204157 comment 11, if a user opens a browser window (with the new
browser window pref set to blank) from a non-browser window && there are no
other browser windows, then that new browser window will load the home page.

however, this bug seems to be the converse. but is it valid behavior? i'm not
sure if it is...

this bug seems to be a result of the fix from bug 204157, since a build from 5/9
doesn't exhibit this particular problem.

Comment 10

[Ian Neal]

I've quickly tested on Linux using BuildID 2003052008 and the behaviour is as
designed. Ctrl-N from Mail when you don't have another browser window open makes
use of the Navigator Startup Pref. Ctrl-N when you do have another browser
window open makes use of the New Window Pref.

I will test on WinXP tomorrow but I have no access to test on a Mac.

Comment 11

[sairuh (rarely reading bugmail)]

while writing comment 9, i must've had my startup pref set to about:blank by
accident.

i doublechecked (on OS X too), and when there aren't any browser windows
present, opening a new browser will follow the startup pref (be it about:blank,
homepage or last-visited), not the new browser window pref.

Comment 12

[Ian Neal]

Tested on XP and I have same behaviour as I had for Linux.

Reporter: could you confirm:
a) What your prefs are for Navigator Startup
b) What your prefs are for New Window
c) Whether you have any other browser windows open when you hit Ctrl-N from
Mail&News

From those answers I should be able to refine what the problems is you are
experiencing.

Prior to the fix for bug 204157 hitting Ctrl-N within Mail/News would make use
of the Navigator Startup Pref regardless of whether there was a browser window
already open 

Comment 13

[sairuh (rarely reading bugmail)]

hrm, now i think i'm seeing this problem using 2003.05.22.08 comm bits on OS X
(10.2.6).

1. have the following set in the Navigator prefs panel:

a. startup page to "last-visited"
b. new browser window to "last-visited" (or even "homepage")
c. new tab to blank
d. homepage set to http://mozilla.org

2. click OK to save and dismiss prefs.
3. go to http://mozilla.org
4. click on a link, eg, "Development" which loads http://www.mozilla.org/catalog/
5. quit and restart, which will load the link in step 4.
6. close the window --on OS X this won't quit the app, so the menubar will persist.
7. open a new browser window, again, this will load the same url as in step 4.
8. open a composer window (cmd-shift-N or cmd-4).
9. while the composer window is frontmost, open another browser window.

expected: since there's already another browser window open (step 7), this 2nd
browser window should also load the last-visited link.

actual results: the 2nd browser window is blank (about:blank).

Comment 14

[sairuh (rarely reading bugmail)]

and here's a weird variation from the test in comment 13:

same steps, but at (7), don't open a browser window, instead open a composer window.
8. now, open a browser window.

results: the pref is followed, and the last-visited url is loaded in the browser
window. odd.

Comment 15

[sairuh (rarely reading bugmail)]

hrm, was able to reproduce the problem on linux (rh7.2, 2003.05.20.08 comm
build) following comment 13 and comment 14. (note: steps 6 and 7 are not
applicable to linux, so skipped them.)

Comment 16

[Ian Neal]

If you follow the steps in comment 14 on win32 the same things happens again.

What happens is that an error is occuring in the try/catch in the
OpenBrowserWindow function within tasksOverlay.js

One problem is that currently tasksOverlay.js makes use of getHomePage and
getWebNavigation from navigator.js and browser.js respectively and composer
doesn't load this script.

One of the other problems is that a mail/news compose window doesn't define pref
so again tasksOverlay.js errors.

And yes definitely a regression caused by patch to bug 204157 because prior to
that these calls were only made when ctrl-n was used from a browser window.

Comment 17

[Ian Neal]

Attachment: First pass at a patch to deal with regression

Enables saving of last page visited in history no matter what the preference on
navigator startup. Also defines pref so function doesn't error when called from
mail compose window. Homepage determination is just copied across from
navigator.js

Comment 18

[Ian Neal]

Accepting

Comment 19

[Ian Neal]

Compiled and tested on Linux and the problem is fixed and no other regressions
have appeared.

Comment 20

[Ian Neal]

Updating summary, old one was "New window (Ctrl+N) in Mail/News should open home
page, but instead is blank"

Comment 21

[Ian Neal]

Attachment: Patch v1.1 moving getHomePage() from navigator.js to tasksOverlay.js

This patch moves getHomePage() from navigator.js to tasksOverlay.js so code
doesn't have to be repeated as tasksOverlay.js is always loaded when
navigator.js is loaded but not the other way round. The one exception is
hiddenWindow.xul but that doesn't make use of getHomePage directly or
indirectly.

Comment 22

[Ian Neal]

Also for last page visited to work need to keep track of pages visited at not
just times when navigator startup pref is set to last page visited.

Comment 23

[jag (Peter Annema)]

Comment on attachment 124071 [details] [diff] [review]
Patch v1.1 moving getHomePage() from navigator.js to tasksOverlay.js

Will this patch work? If tasksOverlay.js can't depend on |pref| being
available, the getHomePage code should get it for itself.

nsGlobalHistory wasn't storing the last visited url for privacy reasons.
Instead of looking at the startup page pref, should we perhaps look at all
three prefs, and only store this if any one of them is "previous page"?

I'm not sure I like the idea of moving getHomePage into tasksOverlay.js. It
just seems like the wrong place, though I can't think of a better place either.

Actually, we could clean this up a little by moving the loadOnNewWindow code
into navigator.js Startup and only using that code when e.g. arguments[0] ===
null or "" or something.

Then opening a new window with a URI or URIs is still the same as it is,
opening the browser window from startup is still the same (since it too passes
in a URI or URIs), and opening a new window has become simpler.

Just a suggestion, what do you think?

Comment 24

[Ian Neal]

Attachment: Patch v1.2

Leaves getHomePage() in navigator.js.
Moves the checking of loadOnNewWindow to navigator.js by passing null instead
of startpage if a new browser window is requested when there is one already in
existance.
Restricts the storing of last page visited to when at least one of the
Navigator Startup, New Window or New Tab is set to be last page visited.

Comment 25

[[e-mail]]

Comment on attachment 124498 [details] [diff] [review]
Patch v1.2

>+      var uriArray;
>+      if (window.arguments[0]) {
>+        uriArray = window.arguments[0].toString().split('\n'); // stringify and split
>+      } else {
>+        try {
>+          switch (pref.getIntPref("browser.windows.loadOnNewWindow"))
>+          {
>+            case -1:
>+              var handler = Components.classes['@mozilla.org/commandlinehandler/general-startup;1?type=browser']
>+                                      .getService(Components.interfaces.nsICmdLineHandler);
>+              uriArray = handler.defaultArgs;
>+              break;
>+            default:
>+              uriArray = "about:blank";
>+              break;
>+            case 1:
>+              uriArray = getHomePage();
>+              break;
>+            case 2:
>+              history = Components.classes["@mozilla.org/browser/global-history;1"]
>+                                  .getService(Components.interfaces.nsIBrowserHistory);
>+              uriArray = history.lastPageVisited;
>+              break;
>+          }
>+        } catch(e) {
>+          uriArray = "about:blank";
>+        }
>+      }
>       uriToLoad = uriArray.splice(0, 1)[0];
This isn't going to work, you need to split the array outside the if.

Comment 26

[[e-mail]]

Comment on attachment 124498 [details] [diff] [review]
Patch v1.2

My previous comment wasn't exactly correct, because getHomePage() does return
an array so in that one case the code would work. In the defaultArgs case you
probably need to split the value; in the other cases wrapping the value in []s
should work.

Comment 27

[Ian Neal]

Attachment: Patch v1.3

Now does arrays correctly as per Neil's comments

Comment 28

[Ian Neal]

Attachment: Patch v1.3a

Same as v1.3 except fixed one about:blank I missed - thanks for the catch Neil

Comment 29

[Jo Hermans]

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

Comment 30

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 31

[jag (Peter Annema)]

Comment on attachment 124548 [details] [diff] [review]
Patch v1.3a

>Index: components/history/src/nsGlobalHistory.cpp
>===================================================================

   if (gPrefBranch) {
     PRInt32 choice = 0;
     gPrefBranch->GetIntPref(PREF_BROWSER_STARTUP_PAGE, &choice);
+    if (choice != 2) {
+      gPrefBranch->GetIntPref(PREF_BROWSER_WINDOWS_LOADONNEWWINDOW, &choice);
+      if (choice != 2)
+	 gPrefBranch->GetIntPref(PREF_BROWSER_TABS_LOADONNEWTAB, &choice);
     }
     if (choice == 2) {
       rv = SaveLastPageVisited(aURL);
       NS_ENSURE_SUCCESS(rv, rv);

After the first |choice != 2| fails you know it's going to fail on the next
check too. Express that in your code by moving the second check inside the body
of the |if| as above.

sr=jag with that change.

Comment 32

[Ian Neal]

Attachment: Patch v1.3b

Same as 1.3a with alteration as suggested by jag

Comment 33

[Ian Neal]

Comment on attachment 124690 [details] [diff] [review]
Patch v1.3b

r=/sr= carried from 1.3a

Requesting approval from drivers for checkin to branch as fixes regression
caused by patch to bug 204157

Comment 34

[Ian Neal]

Confirmed r= okay from Neil via IRC, sr= from jag as per comment #31

Comment 35

[[e-mail]]

Fix checked in.

Comment 36

[Ian Neal]

Reopening as it still needs branch approval and checkin.

Comment 37

[Ian Neal]

Accepting bug

Comment 38

[Ian Neal]

Currently 3 options as far as the branch is concerned:
a) Don't apply this patch - will leave users unable to control what happens when
they create a new navigator window from anything other than another browser
window - it will always be a blank page.
b) Backout patch to bug 204157 - will mean Ctrl-N from a non-browser window will
follow Navigator Startup pref regardless of whether a browser window already
exists or not.
c) Checkin this patch - has been tested on win32/linux for about 3-4 days and is
now getting further testing on the trunk

Risks:
a) Very low risk but will probably generate alot of bug reports
b) Low risk and will probably need something put in the release notes
c) Low risk and will again need something in the release notes but will probably
have better logic behind it than (b)

Comment 39

[[e-mail]]

Fix checked into the 1.4 branch.

Comment 40

[Matthias Versen [:Matti]]

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

Comment 41

[sairuh (rarely reading bugmail)]

musta missed the 5am branch builds (today's 1.4 comm builds on win2k and OS X
still show this problem). will check with tomorrow's 1.4 builds (or today's
linux 1.4 builds, whichever comes first).

Comment 42

[Ian Neal]

Reopening due to problem with using history as a variable in navigator.js
Patch to fix very soon

Comment 43

[Ian Neal]

Attachment: Patch to change history to uaHistory

This patch changes history to uaHistory because history has special meaning,
also gets rid of JS strict warning by adding var

Comment 44

[sairuh (rarely reading bugmail)]

removing fixed1.4 since another issue has been found.

Comment 45

[jag (Peter Annema)]

Comment on attachment 124935 [details] [diff] [review]
Patch to change history to uaHistory

"oops". r+sr=jag, though I'd prefer |browserHistory| over |uaHistory|.

Hrm, wait. Doesn't saying |var history| ensure you're using a local variable
and not the special one?

Comment 46

[Ian Neal]

Yes doing |var history| does localise it, but I prefered to be cautious.

Comment 47

[Ian Neal]

Attachment: Using browserHistory

Comment 48

[Ian Neal]

Attachment: Using var history

One of my patches between 1.1 and 1.2 did use globalHistory but that is
sometimes declared elsewhere in the function so caused errors of its own. My
preference is to use |browserHistory| too now.

Comment 49

[Ian Neal]

jag: any preference on the variable?

Comment 50

[[e-mail]]

Ah, of course there was no JS strict warning to spot before :-/

Comment 51

[jag (Peter Annema)]

Comment on attachment 124947 [details] [diff] [review]
Using var history

r+sr=jag

Comment 52

[Ian Neal]

Accepting just need branch approval

Comment 53

[Ian Neal]

Checked into trunk by jag - thanks

Comment 54

[sairuh (rarely reading bugmail)]

tested using 2003.06.06.08 trunk builds on linux rh7.2 and mac os x 10.2.6.
strangely, i encountered this problem only once (on os x, initial session i ran
with today's build). but then i couldn't reproduce it again.

is there a case where this bug will crop up still?

at this point, however, it's just a mild bug, and how it is now is a definite
improvement.

Comment 55

[Ian Neal]

The only reason it would produce a blank page (other than the pref being set to
it) is for something within the try/catch to cause an error. It's possible
.lastPageVisited errors when there's no history.

Comment 56

[Skewer]

Verified fixed on all platforms; so at least it ought to be resolved.

Comment 57

[Skewer]

(Correction: Not verified on all platforms until now. Verified fixed with
2003060608 WinXP)

Comment 58

[Ian Neal]

Reopening as it's not been landed on the branch yet, please read through
comments before resolving a bug as fixed

Comment 59

[Ian Neal]

Requesting approval to checkin to branch, same options/risks as in comment #38.
It is low risk and it's been sitting in the trunk for about 5 days now without
any problems.

Comment 60

[Asa Dotzler [:asa]]

Comment on attachment 124947 [details] [diff] [review]
Using var history

a=asa (on behalf of drivers) for checkin to the 1.4 branch

Comment 61

[[e-mail]]

Fix checked into the branch.

Comment 62

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.06.10.0x-1.4 (branch comm), and carrying over trunk
testing from comment 54.