Last Comment Bug 726415 - Ctrl-T / New Tab since SeaMonkey v2.7 duplicates last fetched page instead of currently viewed page/tab
: Ctrl-T / New Tab since SeaMonkey v2.7 duplicates last fetched page instead o...
Status: RESOLVED FIXED
[mentor=Ratty][lang=js][good first bug]
: regression, relnote
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.12
Assigned To: bryant rojas
:
:
Mentors:
: 731529 (view as bug list)
Depends on:
Blocks: 692423
  Show dependency treegraph
 
Reported: 2012-02-12 02:50 PST by R.K.
Modified: 2012-06-05 09:10 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Duplicate tab (1.23 KB, patch)
2012-05-12 15:41 PDT, bryant rojas
no flags Details | Diff | Splinter Review
Patch #2: 'Last Page Viewed' duplicates currently viewed tab (1.55 KB, patch)
2012-05-13 16:34 PDT, bryant rojas
neil: review+
Details | Diff | Splinter Review
Patch #3: Took out else block (1.53 KB, patch)
2012-05-20 18:59 PDT, bryant rojas
no flags Details | Diff | Splinter Review
Patch #2a [for checkin] r=Neil (1.54 KB, patch)
2012-05-21 14:05 PDT, bryant rojas
philip.chee: review+
Details | Diff | Splinter Review

Description R.K. 2012-02-12 02:50:07 PST
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:10.0.1) Gecko/20120208 Firefox/10.0.1 SeaMonkey/2.7.1
Build ID: 20120208224119

Steps to reproduce:

open several different tab pages - or even different browser windows.
View a tab/windows which doesn't contain the last fetched page.
Press Ctrl-T or File/New/BrowserTab.



Actual results:

From SeaMonkey 2.7 (Firefox??) the new tab shows a duplicate of the last fetched page/URL (in whichever tab or separate window).
Instead of the currently viewed page/URL.


Expected results:

It should duplicate the currently viewed page/URL.
Or at least there should be an extra option in Preferences/Browser/DisplayonNewTab if that annoying change/bug is intentionally.

Really what you want much more is to fork the current view, than fork the (often forgotten/"random") last in time downloaded URL - which also enables only ONE option. That bug mode now stronly shrinks the usability and flexibilty.
In addition if the last fetched URL was a critical form POST - that unwanted (you get what you NOT see) repetition is a serious stability and security issue.
Comment 1 Philip Chee 2012-02-12 04:47:36 PST
CTRL-T should open a new blank tab. If you see something else you've probably got a extension installed like Duplicate This Tab or Tab Clicking Options. See if you can reproduce this in safe mode e.g. Help->Restart with Addons Disabled.
Comment 2 therube 2012-02-12 05:23:35 PST
What is your setting for:

> Edit | Preferences | Browser -> Display on [New Tab] (drop down)?

If it is set to Last Page Visited, then it may be doing as expected.

---

Confirmed.

Set New Window to Blank
Set New Tab to Last Page Visited
Quit

Start Browser

Open new window
Open a tab in that window & load this bug

Open a new window
(window should be blank)

Open a new tab in that window

Results:

In SM 2.6, a new blank tab is opened
In SM 2.7, this bug is loaded

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a2) Gecko/20120210 Firefox/12.0a2 SeaMonkey/2.9a2
Comment 3 therube 2012-02-12 05:30:47 PST
I was not looking for a regression range, but I did note that 2.7a1 works like 2.6

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110928 Firefox/10.0a1 SeaMonkey/2.7a1

& by the time we got to 2.7 release, the behavior had changed.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20120129 Firefox/10.0 SeaMonkey/2.7
Comment 4 R.K. 2012-02-12 07:12:41 PST
(of course Preferences/Browser/DisplayonNewTab/3rdOption as indicated.  I always forked browsing on NewTab/Ctrl-T, and started white on NewWindow/Ctrl-N, and Ctrl-Enter/Clicked for immediate new URL tab - Is something else reasonable/effective?)

"In SM 2.6, a new blank tab is opened":

I don't want to reinstall 2.6 and previous (perhaps 2.5?) parallel (don't know if this mixes up the email database and all), but I'm quite sure for all the years Mozilla/Seamonkey opened the _currently_viewed_ URL upon Ctrl-T  with the option which transited to the current 3rd option (where I didn't change anything manually). Maybe you still run the 1st option?

Whatsoever, I cannot imagine the rather random _last_downloaded_URL_ (independent of current view) on Ctrl-T is good for anything. If sb really wants such a weired random behavior at all, that should be offered as new (4th) option. Maybe even a JS somewhere in the background can cause a change on _last_downloaded_URL_ ...
Comment 5 Philip Chee 2012-02-12 08:50:49 PST
Ah Bug 692423 - Remove "Last page visited" options from Browser preferences, or reimplement functionality.

From Bug 692423 Comment 0

> Bug 691524 removed support for "Last page visited" for Gecko 10. Unless we somehow
> move and implement the feature in our code, we need to remove the corresponding
> Preferences options under Browser/Display on [Browser Startup/New Window/New Tab].
Blame -> cc Neil.
Comment 6 neil@parkwaycc.co.uk 2012-02-12 10:06:42 PST
If you want to open a copy of the current tab in a new tab, you can just Control+Click or middle-click the reload button.
Comment 7 Philip Chee 2012-02-12 10:11:25 PST
I wonder if we can map CTRL-SHIFT-T to do that?
Comment 8 neil@parkwaycc.co.uk 2012-02-13 01:44:10 PST
(In reply to Philip Chee from comment #7)
> I wonder if we can map CTRL-SHIFT-T to do that?
No, that's undo close tab.
Comment 9 R.K. 2012-02-13 04:57:08 PST
"Last page visited" should perhaps be reworked (as it was) to current_viewed_URL to keep it on key. I don't move the hand to the mouse often. mostly just Ctrl-L, keyword nav, Alt-B, Ctrl-N/T, tab, ...

just remove the option: I think there is not much reason to have Ctrl-T without actual function (new tab empty / homepage), as one does usually immediately target new tabs with Ctrl+Enter/Klick.
Fork browsing should be possible by the standard Ctrl-T, or some other prominent key.
Comment 10 Philip Chee 2012-02-13 10:03:41 PST
We can add a fourth option to the "Display on New Tab" e.g. "Duplicate Current tab". We already have the code to do this. Just a matter of hooking it up to the switch statement in BrowserOpenTab(). What do you think Neil?
Comment 11 neil@parkwaycc.co.uk 2012-02-13 11:44:33 PST
Sure, that sounds reasonable.
Comment 12 Philip Chee 2012-02-29 12:45:26 PST
*** Bug 731529 has been marked as a duplicate of this bug. ***
Comment 13 neil@parkwaycc.co.uk 2012-02-29 13:34:24 PST
I guess we ought to put this on the New Issues section of the release notes, including the workaround of duplicating the tab by middle-clicking Reload.
Comment 14 Joanna 2012-03-01 03:04:42 PST
How do you middle click from a mac? 

I like Philip Chee's "fourth option" solution.  I don't know why anyone wants the most recently fetched page ever (I'm personally often surprised what it even is, sequence of fetching just does not register in my brain when I'm working fast).  But the whole reason I use SeaMonkey not FireFox is it gives me more options on preferences to do what I need to do.
Comment 15 neil@parkwaycc.co.uk 2012-03-01 04:17:32 PST
(In reply to Joanna from comment #14)
> How do you middle click from a mac?
I think holding either the Cmd or the Ctrl key should work.

Also hold the Shift key if the tab focus behaviour is not what you want.
Comment 16 Joanna 2012-03-01 10:31:46 PST
Clicking the tab button while holding the Ctrl key (which I know is right mouse button click anyway) does nothing -- no new tab is made.

Clicking the tab button while holding thee cmd, shift or alt key is the same as not holding any key down -- I get the most-recently-fetched page again.
Comment 17 Philip Chee 2012-03-01 11:05:22 PST
That's the "Reload" button not the tab button.
Comment 18 Joanna 2012-03-01 11:09:08 PST
Ah, OK, the cmd reload works
Comment 19 Jens Hatlak (:InvisibleSmiley) 2012-03-07 14:10:53 PST
(In reply to neil@parkwaycc.co.uk from comment #13)
> I guess we ought to put this on the New Issues section of the release notes,
> including the workaround of duplicating the tab by middle-clicking Reload.

Alright, will include it in relnotes of both 2.7 and 2.8 (so it will remain for future releases until removed) starting with tomorrow's 2.8b6.
Comment 20 bryant rojas 2012-04-03 07:29:31 PDT
Me and a buddy are going to take a shot at this :)
Comment 21 Philip Chee 2012-05-11 10:53:28 PDT
Currently the code in navigator.js is:

http://hg.mozilla.org/comm-central/annotate/57eea920a92e/suite/browser/navigator.js#l1342

function BrowserOpenTab()
{
  if (!gInPrintPreviewMode) {
    var uriToLoad;
    switch (GetIntPref("browser.tabs.loadOnNewTab", 0))
    {
      default:
        uriToLoad = "about:blank";
        break;
      case 1:
        uriToLoad = GetLocalizedStringPref("browser.startup.homepage");
        break;
      case 2:
        uriToLoad = GetStringPref("browser.history.last_page_visited");
        break;
    }

Several iterations ago it was:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/suite/browser/navigator.js&rev=1.623&mark=1302-1306#1288


case 2:
  uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
                       : Components.classes["@mozilla.org/browser/global-history;2"]
                                   .getService(Components.interfaces.nsIBrowserHistory)
                                   .lastPageVisited;

The simplest fix would be to do something like:

case 2:
  uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
                       : GetStringPref("browser.history.last_page_visited");



Bonus points if in case 2, instead of just opening a new tab with the currentURI, you clone the current tab using OpenSessionHistoryIn() from:

http://hg.mozilla.org/comm-central/annotate/57eea920a92e/suite/browser/navigator.js#l882
Comment 22 bryant rojas 2012-05-11 11:35:12 PDT
hmm ok. I think I can get this by the end of the day. ATM I'm trying to figure out a little more about the structure of whats being called where. 

So I see in BrowserOpenTab(), theres this:
gBrowser.selectedTab = gBrowser.addTab(uriToLoad); Kinda followed that to tabbrowser.xml. 

Mm so if I'm understanding this right, uriToLoad is just holding a page with url info and things of that such?  Oh so then currentURI is the current page that is focused/viewed? 
Actually how can I find out more about getWebNavigation().currentURI.spec? I'll be looking in the meantime :}
Comment 23 Philip Chee 2012-05-11 13:04:59 PDT
> uriToLoad is just holding a page with url info and things of that such?
Right, it's just a javascript var.
> Oh so then currentURI is the current page that is focused/viewed?
Right again.

> how can I find out more about getWebNavigation().currentURI.spec?
Well I'd use MXR:
http://mxr.mozilla.org/comm-central/search?string=function+getWebNavigation

/mozilla/browser/.... This is the Firefox version
/suite/browser/browser.js This is the SeaMonkey version.

But you're going down a rabbit hole. I suggest something like:

    var tabPref = GetIntPref("browser.tabs.loadOnNewTab", 0);
    switch (tabPref)
    {
.......
    if (tabPref == 2)
      OpenSessionHistoryIn("tabfocused", 0);
    else
      gBrowser.selectedTab = gBrowser.addTab(uriToLoad);

    if (uriToLoad == "about:blank" && isElementVisible(gURLBar))
      setTimeout(WindowFocusTimerCallback, 0, gURLBar);
    else
      setTimeout(WindowFocusTimerCallback, 0, content);
Comment 24 bryant rojas 2012-05-11 15:54:00 PDT
This should work as well I think.
case 2:
        //uriToLoad = GetStringPref("browser.history.last_page_visited");
        uriToLoad = OpenSessionHistoryIn("current",0);
        break;

I'm currently getting a fresh build going to try it out. Was having some trouble earlier, but seems to be going smoothly now :}
Comment 25 bryant rojas 2012-05-12 00:12:15 PDT
Oh I believe I interpreted the problem wrong. I was simply changing the functionality of Last Page Visited.

Just to be clear, the end goal is to add a new option, Current Page, under: 
> Edit | Preferences | Browser -> Display on [New Tab] (drop down)

Current Options are: Blank Page, Home Page, Last Page Visited.

This way the switch statement can have a case 3 that looks something like: 
case 3:
        uriToLoad = OpenSessionHistoryIn("current",0);
        break;
?
Comment 26 Philip Chee 2012-05-12 03:10:00 PDT
> This should work as well I think.
> case 2:
>         //uriToLoad = GetStringPref("browser.history.last_page_visited");
>         uriToLoad = OpenSessionHistoryIn("current",0);
>         break;
This just reloads the current tab with the current tab which doesn't make any sense. And OpenSessionHistoryIn() returns void!

I've thought about it since. I currently think having just the current options is fine. My current plan is:

1. Fix the regression by opening a new tab with the URL of the current tab for "Last Page Visited"
2. Enhance "Last Page Visited" by not just loading the URL from the current tab but also load the associated session history.

For option (1) it would suffice to just do:
> case 2:
>   uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
>                        : GetStringPref("browser.history.last_page_visited");

For option (2) you'll need OpenSessionHistoryIn() but you'll need to use it correctly.
Comment 27 bryant rojas 2012-05-12 15:41:56 PDT
Created attachment 623461 [details] [diff] [review]
Duplicate tab

Ok! 
So I basically went back and looked at your past suggestions and they indeed seemed to be best to get the job done.

Altered the switch to save the tab preference int. So when it comes time to create the new tab, if Last Page Visited is selected, OpenSessionHistoryIn creates the new tab. Otherwise gBrowser.selectedTab = gBrowser.addTab(uriToLoad) does. 

Works nicely if no window is open and ctrl+t is used to create a tab, then indeed "last page visited" is called. I think thats what you intended.
Comment 28 neil@parkwaycc.co.uk 2012-05-12 16:41:28 PDT
Just to be awkward, the Mac allows you to open a new tab even when no windows are open. So you have to be careful not to break the Mac in that case.
Comment 29 neil@parkwaycc.co.uk 2012-05-12 16:43:41 PDT
Sorry, but because you only gave three lines of context, I couldn't see where the code that dealt with opening new tabs on the Mac fitted on.
Comment 30 bryant rojas 2012-05-12 16:49:13 PDT
ohhh lol, because I'm on a Mac I just assumed all platforms had the same capability :p
Comment 31 neil@parkwaycc.co.uk 2012-05-13 02:39:01 PDT
Comment on attachment 623461 [details] [diff] [review]
Duplicate tab

>-        uriToLoad = GetStringPref("browser.history.last_page_visited");
>+        uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
>+                             : GetStringPref("browser.history.last_page_visited");
No point changing this because you don't use it if there's a browser.

Should we change the label on the preference screen?
Comment 32 neil@parkwaycc.co.uk 2012-05-13 02:39:47 PDT
(In reply to bryant rojas from comment #30)
> I'm on a Mac
That's fortunate; the rest of us often forget that Mac has some edge cases.
Comment 33 Philip Chee 2012-05-13 04:43:11 PDT
Bryant, great patch. In order to make it better you should do something like the following:

<https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>

Except that since you already have a patch you can just do:
hg export tip
to create what we call a "changeset patch"
<http://mercurial.selenic.com/wiki/QuickStart#Exporting_a_patch>

Once you've done that (but fix Neil's nit see below) attach the changeset patch here and flag it for review.

>>-        uriToLoad = GetStringPref("browser.history.last_page_visited");
>>+        uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
>>+                             : GetStringPref("browser.history.last_page_visited");
> No point changing this because you don't use it if there's a browser.
Oops. I missed that. Mea Culpa.
Comment 34 bryant rojas 2012-05-13 16:34:04 PDT
Created attachment 623537 [details] [diff] [review]
Patch #2: 'Last Page Viewed' duplicates currently viewed tab

Sounds good. Fixed neil's nit and this time around used hg export tip to create the changeset patch.
Comment 35 Philip Chee 2012-05-16 08:40:44 PDT
Hi Bryant, Now that you've attached a patch you need to get it reviewed.

https://developer.mozilla.org/en/Introduction#Step_5_-_Get_your_code_reviewed

But to get it reviewed you need to send the review to someone specific. So you need to find an appropriate reviewer. You can look up the list of SeaMonkey Module owners/peers here:

https://wiki.mozilla.org/Modules/SeaMonkey#Browser

Our uber expert in all things browser is Neil Rashbrook so you need to edit the details of your patch/attachment and add Neil's bugzilla address to requestee field of the review request. See:

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews

If you're lucky you might get a review+ but if there are issues you need to address them.

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Addressing_review_comments.
Comment 36 bryant rojas 2012-05-16 17:02:09 PDT
Why thank you :}
Comment 37 neil@parkwaycc.co.uk 2012-05-16 17:08:24 PDT
Comment on attachment 623537 [details] [diff] [review]
Patch #2: 'Last Page Viewed' duplicates currently viewed tab

>+    if (tabPref == 2)
>+      OpenSessionHistoryIn("tabfocused", 0);
>+    else  
>+      gBrowser.selectedTab = gBrowser.addTab(uriToLoad);
>+      
>     if (uriToLoad == "about:blank" && isElementVisible(gURLBar))
>       setTimeout(WindowFocusTimerCallback, 0, gURLBar);
>     else
>       setTimeout(WindowFocusTimerCallback, 0, content);
OpenSessionHistoryIn already does its own focusing, so it would probably be better if you included this focusing in the else block.

Does anyone think that we should update the preference pane?
Comment 38 Philip Chee 2012-05-16 21:21:52 PDT
> Does anyone think that we should update the preference pane?
Good question. Back in SeaMonkey 1.1 when we were using just |getWebNavigation().currentURI.spec| in case 2 our string was "Last page visited".

Then in 2.0 we did this:

uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
                     : Components.classes["@mozilla.org/browser/global-history;2"]
                                 .getService(Components.interfaces.nsIBrowserHistory)
                                 .lastPageVisited;
And we didn't change our string.

Then later we did this:
uriToLoad = GetStringPref("browser.history.last_page_visited");

And we still didn't change our string.

With this patch, we are back to essentially 1.1 except that the new tab comes with session history attached.
Comment 39 Philip Chee 2012-05-16 21:31:39 PDT
Bryant you now have review+ but with a small suggestion from Neil.

> OpenSessionHistoryIn already does its own focusing, so it would probably be better if
> you included this focusing in the else block.

If you have checkin privileges you can update your patch and then check it in to the repository. You don't need another review because you already have one.

However if you don't have access rights, please attach an updated patch (with a comment that this is for checkin) and add "checkin-needed" in the keyword field.

Thanks!
Comment 40 bryant rojas 2012-05-20 18:58:39 PDT
Ok so I gave it some thought and for:
>    if (tabPref == 2)
>      OpenSessionHistoryIn("tabfocused", 0);
>    else  
>      gBrowser.selectedTab = gBrowser.addTab(uriToLoad);
both OpenSessionHistoryIn and gBrowser.selectedTab = gBrowser.addTab(..) focus onto the tab.

So if these lines are only used when creating a blank page to focus on the url bar:
>     if (uriToLoad == "about:blank" && isElementVisible(gURLBar))
>       setTimeout(WindowFocusTimerCallback, 0, gURLBar);

I'm thinking we dont need these lines at all: 
>     else
>       setTimeout(WindowFocusTimerCallback, 0, content);
Comment 41 bryant rojas 2012-05-20 18:59:44 PDT
Created attachment 625537 [details] [diff] [review]
Patch #3: Took out else block
Comment 42 neil@parkwaycc.co.uk 2012-05-21 00:37:54 PDT
Comment on attachment 625537 [details] [diff] [review]
Patch #3: Took out else block

Actually I was worried about the case where we're duplicating a tab but the uriToLoad is "about:blank", but I guess that can never happen.
Comment 43 neil@parkwaycc.co.uk 2012-05-21 00:39:34 PDT
Yeah, I think we can stick with patch #2 after all. Sorry about that.
Comment 44 bryant rojas 2012-05-21 14:05:09 PDT
Created attachment 625756 [details] [diff] [review]
Patch #2a [for checkin] r=Neil

checkin-needed

Nice :} no problemo
Comment 45 Philip Chee 2012-05-23 03:16:47 PDT
> checkin-needed
That should go in the keyword field.
Comment 46 Philip Chee 2012-05-23 03:40:09 PDT
Comment on attachment 625756 [details] [diff] [review]
Patch #2a [for checkin] r=Neil

Pushed to comm-central changeset fe7d4c09b0d7
Comment 47 Philip Chee 2012-05-23 03:41:45 PDT
Resolved FIXED. Thanks Bryant!
Comment 48 neil@parkwaycc.co.uk 2012-05-26 15:47:35 PDT
Comment on attachment 625756 [details] [diff] [review]
Patch #2a [for checkin] r=Neil

>+    else  
>+      gBrowser.selectedTab = gBrowser.addTab(uriToLoad);
>+      
Bah, I didn't notice the trailing whitespace :s

Note You need to log in before you can comment on or make changes to this bug.