Closed Bug 726415 Opened 12 years ago Closed 12 years ago

Ctrl-T / New Tab since SeaMonkey v2.7 duplicates last fetched page instead of currently viewed page/tab

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.12

People

(Reporter: kxroberto, Assigned: bryant.rjs)

References

Details

(Keywords: regression, relnote, Whiteboard: [mentor=Ratty][lang=js][good first bug])

Attachments

(1 file, 3 obsolete files)

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.
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.
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
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
(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_ ...
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 692423
Keywords: regression
OS: Windows XP → All
Hardware: x86 → All
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.
I wonder if we can map CTRL-SHIFT-T to do that?
(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.
"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.
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?
Sure, that sounds reasonable.
Whiteboard: [mentor=Ratty][lang=js][good first bug]
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.
Keywords: relnote
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.
(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.
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.
That's the "Reload" button not the tab button.
Ah, OK, the cmd reload works
(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.
Version: SeaMonkey 2.7 Branch → Trunk
Me and a buddy are going to take a shot at this :)
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
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 :}
> 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);
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 :}
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;
?
> 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.
Attached patch Duplicate tab (obsolete) — Splinter Review
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.
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.
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.
ohhh lol, because I'm on a Mac I just assumed all platforms had the same capability :p
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?
(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.
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.
Sounds good. Fixed neil's nit and this time around used hg export tip to create the changeset patch.
Attachment #623461 - Attachment is obsolete: true
Attachment #623537 - Flags: review?
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.
Attachment #623537 - Flags: review? → review?(neil)
Why thank you :}
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?
Attachment #623537 - Flags: review?(neil) → review+
> 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.
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!
Assignee: nobody → bryant.rjs
Status: NEW → ASSIGNED
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);
No longer blocks: 692423
Attached patch Patch #3: Took out else block (obsolete) — Splinter Review
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.
Yeah, I think we can stick with patch #2 after all. Sorry about that.
Blocks: 692423
checkin-needed

Nice :} no problemo
Attachment #623537 - Attachment is obsolete: true
Attachment #625537 - Attachment is obsolete: true
> checkin-needed
That should go in the keyword field.
Keywords: checkin-needed
Comment on attachment 625756 [details] [diff] [review]
Patch #2a [for checkin] r=Neil

Pushed to comm-central changeset fe7d4c09b0d7
Attachment #625756 - Attachment description: The Patch → Patch #2a [for checkin] r=Neil
Attachment #625756 - Flags: review+
Resolved FIXED. Thanks Bryant!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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
Target Milestone: --- → seamonkey2.12
You need to log in before you can comment on or make changes to this bug.