The default bug view has changed. See this FAQ.

When middle-clicking back/forward/reload, the new tab should inherit history from the tab that opened it (using duplicateTab)

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
Tabbed Browser
--
enhancement
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Klaas Heidstra, Assigned: Klaas Heidstra)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 4.0b7
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 -
wanted-firefox3.6 +
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 13 obsolete attachments)

8.18 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre

From bug 263942 comment #48 :
> Hang on - there are much better ways to duplicate a tab!
> 
> Bug 393716 added the duplicateTab method (documented at
> http://developer.mozilla.org/en/docs/nsISessionStore ) which will duplicate a
> tab including it's history, scroll position, form contents, etc.
> 
> This method can be used directly for the "tab" and "tabshifted" cases (for the
> foreground tab case you'll have to set selectedTab manually). For the window
> case, while duplicateTab can copy tabs to different windows, it currently
> requires the window to already be open. The cleanest approach is probably to
> extend duplicateTab so that it creates a new window if the passed window is
> null. This would also be useful to Bug 225680 / extensions wanting to allow
> tabs to be "detached" as new windows.
> 
> Note however that the session service requires the browser.sessionstore.enabled
> preference to be true (the default); if not we should fall back on your
> existing code which just copies the uri (and this can continue to handle the
> "save" case).
> 
> I'm happy to have a go at implementing this if you want.

After the tab is duplicated it should reload/go back/go forward(depends on which button was middle/ctrl-clicked). This behavior could also be applied to other items such as home-button/bookmarks/hyperlinks in webpages, but that is for another bug.  

Reproducible: Always

Steps to Reproduce:
1. Go to a page and click on a link.
2. Now middle-click the back-button.
Actual Results:  
A new tab is opened with the url of the "back"-page without restoring history, scroll position, form contents, etc.

Expected Results:  
Middle-click on the back-button should clone the current tab and then go back one page.
(Assignee)

Updated

9 years ago
Depends on: 263942, 440702
(Assignee)

Comment 1

9 years ago
When this bug gets fixed bug 225680 can very easily be fixed so I made that bug depend on this one.
Blocks: 225680

Updated

9 years ago
No longer blocks: 225680
Confirming as an enhancement.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All
Version: unspecified → Trunk
(Assignee)

Comment 3

9 years ago
Would it be an idea to add a "duplicateTabIn" wich works the same way as openUILinkIn in utilityOverlay.js, but instead duplicates a tab to a new window, tab etc...?
(Assignee)

Comment 4

9 years ago
Created attachment 332001 [details] [diff] [review]
patch - add a duplicateTabIn() to utilityOverlay.js for use with back/forward/reload

This adds a duplicateTabIn to utilityOverlay.js it works mostly the same as openUiLinkIn, but duplictaes a tab in a place specified by the parameter "where" (instead of just opening a link). After the tab is duplicated, a function is executed which can do "stuff" with the newly created tab (e.g. when you want to reload the new tab you can attach a function that does something like: getBrowserForTab(newTab).reload()). You can also provide a fallbackUrl which in this case is used for the back and forward buttons, when win.getBrowserForTab(newTab).goBack(); fails it opens the "back"-url in "fallbackUrl".

There is a problem when you middle-click reload pages with anchors in it's address(e.g. http://www.bla.com/#bla), it scrolls to the last position where it should scroll to the anchor. This is because the load event fires before page scrolling happens, I'm not sure how to solve this.
Attachment #332001 - Flags: ui-review?(beltzner)
Attachment #332001 - Flags: review?(dao)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch] [needs review dao]
(Assignee)

Comment 5

9 years ago
Dão could you say if I'm going somewhat in the right direction with this patch?
Comment on attachment 332001 [details] [diff] [review]
patch - add a duplicateTabIn() to utilityOverlay.js for use with back/forward/reload

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1345,37 +1345,45 @@ function gotoHistoryIndex(aEvent)

>     var sessionHistory = getWebNavigation().sessionHistory;
>     var entry = sessionHistory.getEntryAtIndex(index, false);
>     var url = entry.URI.spec;
>-    openUILinkIn(url, where);
>+    var gotoIndexDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).gotoIndex(index);
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, gotoIndexDuplicatedTab, url);

> function BrowserForward(aEvent) {

>     var sessionHistory = getWebNavigation().sessionHistory;
>     var currentIndex = sessionHistory.index;
>     var entry = sessionHistory.getEntryAtIndex(currentIndex + 1, false);
>     var url = entry.URI.spec;
>-    openUILinkIn(url, where);
>+    var goForwardDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).goForward();
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, goForwardDuplicatedTab, url);

> function BrowserBack(aEvent) {

>     var sessionHistory = getWebNavigation().sessionHistory;
>     var currentIndex = sessionHistory.index;
>     var entry = sessionHistory.getEntryAtIndex(currentIndex - 1, false);
>     var url = entry.URI.spec;
>-    openUILinkIn(url, where);
>+    var goBackDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).goBack();  
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, goBackDuplicatedTab, url);

You should be able to share more of this stuff in the helper function. I think this should also eliminate the need to pass a callback function.

Always use gBrowser instead of getBrowser().

> function BrowserHandleBackspace()

>-    openUILinkIn(getWebNavigation().currentURI.spec, where);
>+    var reloadDuplicatedTab = function(newTab) {
>+      var win = newTab.ownerDocument.defaultView.getBrowser();
>+      win.getBrowserForTab(newTab).reload();
>+    };
>+    duplicateTabIn(getBrowser().selectedTab, where, reloadDuplicatedTab);

Why are you reloading here?
(In reply to comment #6)
> > function BrowserHandleBackspace()
> 
> >-    openUILinkIn(getWebNavigation().currentURI.spec, where);
> >+    var reloadDuplicatedTab = function(newTab) {
> >+      var win = newTab.ownerDocument.defaultView.getBrowser();
> >+      win.getBrowserForTab(newTab).reload();
> >+    };
> >+    duplicateTabIn(getBrowser().selectedTab, where, reloadDuplicatedTab);
> 
> Why are you reloading here?

Oops. That's BrowserReloadOrDuplicate, not BrowserHandleBackspace. Still, why are you reloading? It's reload /or/ duplicate.
(In reply to comment #6)
So... pass an index instead of a callback (null/undefined for doing nothing).
Then, if you drop the "save" case, you can remove the url parameter.
(Assignee)

Comment 9

9 years ago
> Oops. That's BrowserReloadOrDuplicate, not BrowserHandleBackspace. Still, why
> are you reloading? It's reload /or/ duplicate.

When people middle-click the reload button they expect that a duplicated "reloaded" tab will appear. Just duplicating a tab is something else, that can be provide by a shortcut key. 

> So... pass an index instead of a callback (null/undefined for doing nothing).
> Then, if you drop the "save" case, you can remove the url parameter.

Besides the save case a fallback url is also needed when gotoIndex fails (e.g. when browser.sessionstore.enabled is false), otherwise it would just load the current url in a new tab instead of the url from the index.

The main reason I'm using a callback function is because in the future duplicateTabIn could also be used for other ui-links, for example when you middle-click on a bookmark you could use something like this:
> var loadURIDuplicatedTab = function(newTab) {
>   var win = newTab.ownerDocument.defaultView.gBrowser;
>   win.getBrowserForTab(newTab).loadURI(bookmarkUrl);
> };
> duplicateTabIn(gBrowser.selectedTab, where, loadURIDuplicatedTab);
(In reply to comment #9)
> > Oops. That's BrowserReloadOrDuplicate, not BrowserHandleBackspace. Still, why
> > are you reloading? It's reload /or/ duplicate.
> 
> When people middle-click the reload button they expect that a duplicated
> "reloaded" tab will appear.

Well, I wouldn't. I don't think users would be too fond of the word "load" when intentionally using a modifier key or the middle mouse button to duplicate a tab. In fact, making the page not reload seems to be the best part of this bug to me; I'm not sure I like the idea of duplicating the history.

> Besides the save case a fallback url is also needed when gotoIndex fails (e.g.
> when browser.sessionstore.enabled is false), otherwise it would just load the
> current url in a new tab instead of the url from the index.

In that case you can still determine the url in the function as currently done outside of it, right?

> The main reason I'm using a callback function is because in the future
> duplicateTabIn could also be used for other ui-links, for example when you
> middle-click on a bookmark you could use something like this:
> > var loadURIDuplicatedTab = function(newTab) {
> >   var win = newTab.ownerDocument.defaultView.gBrowser;
> >   win.getBrowserForTab(newTab).loadURI(bookmarkUrl);
> > };
> > duplicateTabIn(gBrowser.selectedTab, where, loadURIDuplicatedTab);

I don't see why someone would want to have the history of a potentially unrelated tab inherited when opening a bookmark in a new tab. Let's not try to make this work for use cases that don't exist yet.
Anyway, as for the implementation details, I don't think now is the time to discuss this any further. Please get ui-review first.
Keywords: uiwanted
Whiteboard: [has patch] [needs review dao] → [has patch]

Updated

9 years ago
Attachment #332001 - Flags: review?(dao)
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> Anyway, as for the implementation details, I don't think now is the time to
> discuss this any further. Please get ui-review first.

You're right, implementation details can be discussed here:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/76f02c8fb692701b#
(Assignee)

Updated

9 years ago
Whiteboard: [has patch] → [263942 must land first] [has patch] [needs review beltzner]
(Assignee)

Updated

9 years ago
Attachment #332001 - Flags: ui-review?(beltzner)
(Assignee)

Updated

9 years ago
Whiteboard: [263942 must land first] [has patch] [needs review beltzner] → [263942 must land first] [has patch]
(Assignee)

Comment 13

9 years ago
Created attachment 334785 [details] [diff] [review]
patch v2 - add a historyIndex to the duplicateTabIn from bug 225680

This patch applies on top of the one from bug 225680, I'm not sure this functionality is really wanted (maybe I should start a discussion about that somewhere).
Attachment #332001 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Depends on: 225680
Whiteboard: [263942 must land first] [has patch] → [263942 must land first] [225680 must land first] [has patch]
(Assignee)

Comment 14

9 years ago
For some reason bugs that ask for the same thing as this bug were RESOLVED DUPLICATE of bug 18808 (which is in fact about cloning an entire window), so I made that bug depend on this one.
Blocks: 18808
(Assignee)

Updated

9 years ago
Blocks: 271851
(Assignee)

Updated

9 years ago
Attachment #334785 - Flags: ui-review?(beltzner)
(Assignee)

Comment 15

9 years ago
There is a discussion about wether or not this is a good idea in:
http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/701b4f54f135573d#
(Assignee)

Updated

9 years ago
Whiteboard: [263942 must land first] [225680 must land first] [has patch] → [263942 must land first] [225680 must land first] [has patch] [needs review beltzner]
(Assignee)

Updated

9 years ago
Summary: When middle-clicking back/forward/reload, the new tab should inherit history from the tab that opened it(using duplicateTab. → When middle-clicking back/forward/reload, the new tab should inherit history from the tab that opened it (using duplicateTab)
(Assignee)

Updated

9 years ago
No longer depends on: 440702
Whiteboard: [263942 must land first] [225680 must land first] [has patch] [needs review beltzner] → [225680 must land first] [has patch] [needs review beltzner]
(Assignee)

Updated

9 years ago
No longer blocks: 271851
Duplicate of this bug: 271851
(Assignee)

Comment 17

9 years ago
Created attachment 337354 [details] [diff] [review]
patch v2.1 - update to latest patch in 225680

This is just an update to the new patch in bug 225680, there are no functional changes. Oh, and Gavin Sharp thanks for the editbugs rights :).
Assignee: nobody → klaas1988
Attachment #334785 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #337354 - Flags: ui-review?(beltzner)
Attachment #334785 - Flags: ui-review?(beltzner)
Duplicate of this bug: 474962

Updated

8 years ago
Whiteboard: [225680 must land first] [has patch] [needs review beltzner] → [has patch] [needs review beltzner]
Duplicate of this bug: 477947

Updated

8 years ago
Flags: blocking-firefox3.6?
Comment on attachment 337354 [details] [diff] [review]
patch v2.1 - update to latest patch in 225680

Yeah, this makes sense to me.
Attachment #337354 - Flags: ui-review?(beltzner) → ui-review+
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
Keywords: uiwanted
Whiteboard: [has patch] [needs review beltzner] → [has patch]
(Assignee)

Comment 21

8 years ago
Created attachment 400317 [details] [diff] [review]
patch v3 - updated patch opens new tabs next to current

It seems this bug got ui-review+, so here is a new patch.

The duplicateTabIn function from some patch in bug 225680 is now in this patch. Instead of opening newly created tabs on the last position of the tabbar, they now get moved next to the current one (that's the current behaviour for new tabs in the latest nightlies).
Attachment #337354 - Attachment is obsolete: true
Attachment #400317 - Flags: review?(dao)
(Assignee)

Comment 22

8 years ago
Created attachment 400463 [details] [diff] [review]
patch v4- use _lastRelatedTab from bug 465673 for new tabs

Instead of just placing the tab next to the current one, this patch now places it next to the last related tab (see: bug 465673).
Attachment #400317 - Attachment is obsolete: true
Attachment #400463 - Flags: review?(dao)
Attachment #400317 - Flags: review?(dao)
(Assignee)

Updated

8 years ago
Whiteboard: [has patch] → [has patch][needs review dao]
Comment on attachment 400463 [details] [diff] [review]
patch v4- use _lastRelatedTab from bug 465673 for new tabs

>+function duplicateTabIn(aTab, where, historyIndex) {
>+  var browserWin; // parent window of the new tab
>+  var win; // browserWin.gBrowser

This variable is unused. Also, it doesn't make sense to call a reference to gBrowser "win".

>+    var newTabPos = win.mTabs.length - 1;
>+    // New tabs get moved next to last related tab.
>+    if (win.mPrefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent") &&
>+        (where != "window"))
>+      newTabPos = (win._lastRelatedTab || aTab)._tPos + 1;
>+    win.moveTabTo(newTab, newTabPos);
>+    win._lastRelatedTab = newTab;

This shouldn't be implemented here. Instead, I think duplicateTab should pass {relatedToCurrent: true} to addTab, in case the original tab is selected.

>+      newTab.addEventListener("load", newTabOnload, false);

This doesn't make sense. The tab doesn't have a load event, except maybe for the favicon.


>+    case "tab":
>+      browserWin = aTab.ownerDocument.defaultView;
>+      duplicateATab(browserWin.gBrowser, loadInBackground);

Any reason why this isn't just duplicateATab(gBrowser, loadInBackground);?
Attachment #400463 - Flags: review?(dao) → review-
(Assignee)

Comment 24

8 years ago
Created attachment 401581 [details] [diff] [review]
patch v5 - address review comments

(In reply to comment #23)
> (From update of attachment 400463 [details] [diff] [review])
> >+function duplicateTabIn(aTab, where, historyIndex) {
> >+  var browserWin; // parent window of the new tab
> >+  var win; // browserWin.gBrowser
> 
> This variable is unused. Also, it doesn't make sense to call a reference to
> gBrowser "win".

This is removed now, as it was indeed useless.  
 
> This shouldn't be implemented here. Instead, I think duplicateTab should pass
> {relatedToCurrent: true} to addTab, in case the original tab is selected.

relatedToCurrent is now passed via duplicateTab.

> >+      newTab.addEventListener("load", newTabOnload, false);
> 
> This doesn't make sense. The tab doesn't have a load event, except maybe for
> the favicon.

The load eventListener is now attached to the gBrowser object of the new tabs parent window.

> >+    case "tab":
> >+      browserWin = aTab.ownerDocument.defaultView;
> >+      duplicateATab(browserWin.gBrowser, loadInBackground);
> 
> Any reason why this isn't just duplicateATab(gBrowser, loadInBackground);?

There is not a real good reason, so this is now changed to "duplicateATab(gBrowser, loadInBackground);" in this patch.
Attachment #400463 - Attachment is obsolete: true
Attachment #401581 - Flags: review?(dao)
(Assignee)

Comment 25

8 years ago
Created attachment 401585 [details] [diff] [review]
patch v5.1 - forgotten to add aRelatedToCurrent to nsISessionStore.idl

In the previous patch I didn't add the new aRelatedToCurrent param to duplicateTab in nsISessionStore.idl. This is fixed with this patch.
Attachment #401581 - Attachment is obsolete: true
Attachment #401585 - Flags: review?(dao)
Attachment #401581 - Flags: review?(dao)
(Assignee)

Updated

8 years ago
Blocks: 455722
(Assignee)

Comment 26

8 years ago
(In reply to comment #25)
> Created an attachment (id=401585) [details]
> patch v5.1 - forgotten to add aRelatedToCurrent to nsISessionStore.idl

> +          let aTabGBrowser = aTab.ownerDocument.defaultView.gBrowser;
> +          let sessionHistory = aTabWin.webNavigation.sessionHistory;
> +          let entry = sessionHistory.getEntryAtIndex(historyIndex, false);

In the above code aTabWin ofcourse must be aTabGBrowser.
Comment on attachment 401585 [details] [diff] [review]
patch v5.1 - forgotten to add aRelatedToCurrent to nsISessionStore.idl

>       <method name="duplicateTab">
>         <parameter name="aTab"/><!-- can be from a different window as well -->
>+        <parameter name="aRelatedToCurrent"/>

>-  duplicateTab: function sss_duplicateTab(aWindow, aTab) {
>+  duplicateTab: function sss_duplicateTab(aWindow, aTab, aRelatedToCurrent) {

I don't think these changes are needed. Just decide that it's related when aTab == selectedTab.

>+function duplicateTabIn(aTab, where, historyIndex) {

That function belongs in browser.js, I think.

>+  switch (where) {
>+    case "window":
>+      let sa = Components.classes["@mozilla.org/supports-array;1"].
>+               createInstance(Components.interfaces.nsISupportsArray);
>+      sa.AppendElement(null); // wuri
>+      sa.AppendElement(null); // charset
>+      sa.AppendElement(null); // referrerUrl
>+      sa.AppendElement(null); // postData
>+      sa.AppendElement(null); // allowThirdPartyFixup
>+
>+      let ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"].
>+               getService(Components.interfaces.nsIWindowWatcher);
>+      const browserWin = ww.openWindow(getTopWin(),
>+                                       getBrowserURL(),
>+                                       null,
>+                                       "chrome,dialog=no,all",
>+                                       sa);
>+
>+      function browserWinOnload(event) {
>+        browserWin.removeEventListener("load", browserWinOnload, false);
>+        setTimeout(function() {
>+          duplicateATab(browserWin.gBrowser);
>+        }, 0);
>+      }
>+      browserWin.addEventListener("load", browserWinOnload, false);

This is all a huge hack. I think we should either duplicate the tab and then use gBrowser.replaceTabWithWindow or pass the tab to the new window and let that window duplicate it.
Attachment #401585 - Flags: review?(dao) → review-
(Assignee)

Comment 28

8 years ago
Created attachment 403151 [details] [diff] [review]
patch v6 - addresses issues from comment 27

(In reply to comment #27)
> (From update of attachment 401585 [details] [diff] [review])
> I don't think these changes are needed. Just decide that it's related when aTab
> == selectedTab.
Done.

> >+function duplicateTabIn(aTab, where, historyIndex) {
> 
> That function belongs in browser.js, I think.
The reason I put it in utilityOverlay.js is because similar functions like openUILinkIn are also in there. But I now have moved it to browser.js.

> This is all a huge hack. I think we should either duplicate the tab and then
> use gBrowser.replaceTabWithWindow or pass the tab to the new window and let
> that window duplicate it.
In this patch I have chosen for the "gBrowser.replaceTabWithWindow" approach.
Attachment #401585 - Attachment is obsolete: true
Attachment #403151 - Flags: review?(dao)
(Assignee)

Comment 29

8 years ago
Created attachment 403156 [details] [diff] [review]
patch v6.1 - fix problem with new windows

The previous patch didn't go back when you shift middle clicked the back button, to open in a new window. To solve this problem I now call the replaceTabWithWindow function after the tab has navigated to a history index.
Attachment #403151 - Attachment is obsolete: true
Attachment #403156 - Flags: review?(dao)
Attachment #403151 - Flags: review?(dao)
Comment on attachment 403156 [details] [diff] [review]
patch v6.1 - fix problem with new windows

>+    if (where != "window" && !loadInBackground) {
>+      // If the new tab gets closed, select the previous selected tab.
>+      newTab.owner = gBrowser.selectedTab;
>+      function attrChanged(event) {
>+        if (event.attrName == "selectedIndex" &&
>+            event.prevValue != event.newValue)
>+          gBrowser.resetOwner(parseInt(event.prevValue));
>+      }
>+      if (!gBrowser.mTabChangedListenerAdded) {
>+        gBrowser.mTabBox.addEventListener("DOMAttrModified", attrChanged,
>+                                          false);
>+        gBrowser.mTabChangedListenerAdded = true;
>+      }
>+      // Select the newly created tab.
>+      gBrowser.selectedTab = newTab;
>+    }

This whole block looks like it shouldn't be needed...

>--- a/browser/components/sessionstore/src/nsSessionStore.js	Thu Sep 10 09:10:30 2009 +0100
>+++ b/browser/components/sessionstore/src/nsSessionStore.js	Sun Sep 27 18:59:51 2009 -0500
>@@ -947,8 +947,9 @@
>     var tabState = this._collectTabData(aTab, true);
>     var sourceWindow = aTab.ownerDocument.defaultView;
>     this._updateTextAndScrollDataForTab(sourceWindow, aTab.linkedBrowser, tabState, true);
>-    
>-    var newTab = aWindow.getBrowser().addTab();
>+
>+    let aRelatedToCurrent = aTab == aWindow.gBrowser.selectedTab ? true : false;
>+    var newTab = aWindow.gBrowser.addTab(null, {relatedToCurrent: aRelatedToCurrent});

I think you need to set the owner here, e.g. like this:

if (aTab == aWindow.gBrowser.selectedTab) {
  var newTab = aWindow.gBrowser.addTab(null, {relatedToCurrent: true,
                                              ownerTab: aTab});
} else {
  newTab = aWindow.gBrowser.addTab();
}
Attachment #403156 - Flags: review?(dao) → review-

Comment 31

7 years ago
Does the patch here also fix bug 18808 for the case of opening links in a new tab and not just for back/forward? Or it's for a separate ui-review decision for that behavior?

Klaas, do you need any help updating the patch perhaps from some sessionstore people? What's the status of it now?

"Tab History" add-on https://addons.mozilla.org/en-US/firefox/addon/1859 seems to implement this pretty briefly with a loop to to.addEntry(from.getEntry())
(Assignee)

Comment 32

7 years ago
(In reply to comment #31)
> Does the patch here also fix bug 18808 for the case of opening links in a new
> tab and not just for back/forward? Or it's for a separate ui-review decision
> for that behavior?
No it doesn't. That behavior could be achieved with something like ctrl+shift+n, see: bug 455722.

> Klaas, do you need any help updating the patch perhaps from some sessionstore
> people? What's the status of it now?
> 
> "Tab History" add-on https://addons.mozilla.org/en-US/firefox/addon/1859 seems
> to implement this pretty briefly with a loop to to.addEntry(from.getEntry())
The patch should be updated with the comments from comment #30 addressed. Unfortunately I haven't really got time to work on this anytime soon, so maybe someone else could finish this patch?
Blocks: 271851
Whiteboard: [has patch][needs review dao]

Updated

7 years ago
Blocks: 565517

Updated

7 years ago
Duplicate of this bug: 583970
(Assignee)

Comment 34

7 years ago
Created attachment 463323 [details] [diff] [review]
patch v7 - address review comments and update to tip

(In reply to comment #30)
> This whole block looks like it shouldn't be needed...
This is removed now. Also "gBrowser.addEventListener("load", gBrowserOnload, false);" isn't needed anymore, so that is removed too. 

> I think you need to set the owner here, e.g. like this:
> 
> if (aTab == aWindow.gBrowser.selectedTab) {
>   var newTab = aWindow.gBrowser.addTab(null, {relatedToCurrent: true,
>                                               ownerTab: aTab});
> } else {
>   newTab = aWindow.gBrowser.addTab();
> }
Done (although I've used a little different syntax).

DuplicateTab in tabbrowser.xml is modified in a similar way:
> +              return aTab == this.mCurrentBrowser.selectedTab ?
> +                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
> +                                {relatedToCurrent: true, ownerTab: aTab}) :
> +                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);

The helper function from the previous patch (duplicateATab) wasn't really needed and is removed.

There are some problems caused by limitations of SessionStore:
* The state of HTML5 video isn't restored when duplicating a tab (the same applies to plugins)
* Only the scroll position of the current page gets restored. This means that when you middle-click the back button, the new tab's scroll position will always be at the top
Attachment #403156 - Attachment is obsolete: true
Attachment #463323 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review dao]
Comment on attachment 463323 [details] [diff] [review]
patch v7 - address review comments and update to tip

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1881,22 +1881,26 @@
>       </method>
> 
>       <method name="duplicateTab">
>         <parameter name="aTab"/><!-- can be from a different window as well -->
>         <body>
>           <![CDATA[
>             // try to have SessionStore duplicate the given tab
>             try {
>-              var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>-                                 .getService(Components.interfaces.nsISessionStore);
>+              const ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>+                                   .getService(Components.interfaces.nsISessionStore);
>               return ss.duplicateTab(window, aTab);
>-            } catch (ex) {
>+            }
>+            catch (ex) {

Avoid these changes.

>               // fall back to basic URL copying
>-              return this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>+              return aTab == this.mCurrentBrowser.selectedTab ?
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
>+                                {relatedToCurrent: true, ownerTab: aTab}) :
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);

aTab == this.mCurrentBrowser.selectedTab is never going to be true, as mCurrentBrowser doesn't have such a property. Also, loadOneTab doesn't have an "ownerTab" parameter. It sets the owner tab automatically instead.
(Assignee)

Comment 36

7 years ago
Created attachment 464388 [details] [diff] [review]
patch v7.1 - address issues from comment #35

(In reply to comment #35)

> Avoid these changes.
Okay.

> aTab == this.mCurrentBrowser.selectedTab is never going to be true, as
> mCurrentBrowser doesn't have such a property. Also, loadOneTab doesn't have an
> "ownerTab" parameter. It sets the owner tab automatically instead.
I've changed it to:
+              return aTab == this.selectedTab ?
+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
+                                {relatedToCurrent: true}) :
+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
Attachment #463323 - Attachment is obsolete: true
Attachment #464388 - Flags: review?(dao)
Attachment #463323 - Flags: review?(dao)
(Assignee)

Comment 37

7 years ago
Dão what's your opinion about the latest patch?
No longer blocks: 565517
Comment on attachment 464388 [details] [diff] [review]
patch v7.1 - address issues from comment #35

>+function duplicateTabIn(aTab, where, historyIndex) {
>+  let newTab = gBrowser.duplicateTab(aTab);
>+
>+  // Go to index if it's provided, fallback to loadURI if there's no history.
>+  if (historyIndex != null) {
>+      try {

fix indentation

>+      catch (ex) {
>+        let aTabGBrowser = aTab.ownerDocument.defaultView.gBrowser;
>+        let sessionHistory = aTabGBrowser.webNavigation.sessionHistory;

aTab.linkedBrowser.sessionHistory

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -1886,17 +1886,20 @@
>           <![CDATA[
>             // try to have SessionStore duplicate the given tab
>             try {
>               var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>                                  .getService(Components.interfaces.nsISessionStore);
>               return ss.duplicateTab(window, aTab);
>             } catch (ex) {
>               // fall back to basic URL copying
>-              return this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>+              return aTab == this.selectedTab ?
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec,
>+                                {relatedToCurrent: true}) :
>+                this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>             }
>           ]]>
>         </body>
>       </method>

It doesn't look like this should be using loadOneTab at all, as this would select the tab depending on browser.tabs.loadInBackground, whereas ss.duplicateTab would never select the tab.
More importantly, the whole try/catch thing seems bogus -- ss.duplicateTab shouldn't fail here.

Updated

7 years ago
blocking2.0: --- → ?
(Assignee)

Comment 39

7 years ago
Created attachment 469242 [details] [diff] [review]
patch v8 - address issues from comment #38

(In reply to comment #38)
> fix indentation
Done.

> aTab.linkedBrowser.sessionHistory
Done.

> It doesn't look like this should be using loadOneTab at all, as this would
> select the tab depending on browser.tabs.loadInBackground, whereas
> ss.duplicateTab would never select the tab.
> More importantly, the whole try/catch thing seems bogus -- ss.duplicateTab
> shouldn't fail here.
I've removed the try/catch and only kept the part that was in the try block.
Attachment #464388 - Attachment is obsolete: true
Attachment #469242 - Flags: review?(dao)
Attachment #464388 - Flags: review?(dao)

Comment 40

7 years ago
does this work also for middle-clicking on a link from the back-forward drop-down history menu?
(Assignee)

Comment 41

7 years ago
(In reply to comment #40)
> does this work also for middle-clicking on a link from the back-forward
> drop-down history menu?

Yes, it does.
Comment on attachment 469242 [details] [diff] [review]
patch v8 - address issues from comment #38

>+function gotoHistoryIndex(aEvent) {
>+  let index = aEvent.target.getAttribute("index");
>   if (!index)
>     return false;
> 
>-  var where = whereToOpenLink(aEvent);
>+  let where = whereToOpenLink(aEvent);
> 
>   if (where == "current") {
>-    // Normal click.  Go there in the current tab and update session history.
>+    // Normal click. Go there in the current tab and update session history.
> 
>     try {
>       gBrowser.gotoIndex(index);
>     }
>     catch(ex) {
>       return false;
>     }
>     return true;
>   }
>   else {
>-    // Modified click.  Go there in a new tab/window.
>-    // This code doesn't copy history or work well with framed pages.
>-
>-    var sessionHistory = getWebNavigation().sessionHistory;
>-    var entry = sessionHistory.getEntryAtIndex(index, false);
>-    var url = entry.URI.spec;
>-    openUILinkIn(url, where, {relatedToCurrent: true});
>+    // Modified click. Go there in a new tab/window.
>+
>+    duplicateTabIn(gBrowser.selectedTab, where, index);
>     return true;
>   }

Remove 'else {' here, as the function exits in the if branch.

>+function duplicateTabIn(aTab, where, historyIndex) {
>+  let newTab = gBrowser.duplicateTab(aTab);
...
>+  switch (where) {
>+    case "window":
>+      gBrowser.replaceTabWithWindow(newTab);

This performs pretty badly. Can you call gBrowser.hideTab before calling replaceTabWithWindow so that the tab won't sit in the current window apparently waiting for the new window to open?

>       <method name="duplicateTab">
>         <parameter name="aTab"/><!-- can be from a different window as well -->
>         <body>
>           <![CDATA[
>-            // try to have SessionStore duplicate the given tab
>-            try {
>-              var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>-                                 .getService(Components.interfaces.nsISessionStore);
>-              return ss.duplicateTab(window, aTab);
>-            } catch (ex) {
>-              // fall back to basic URL copying
>-              return this.loadOneTab(this.getBrowserForTab(aTab).currentURI.spec);
>-            }
>+            var ss = Components.classes["@mozilla.org/browser/sessionstore;1"]
>+                               .getService(Components.interfaces.nsISessionStore);
>+            return ss.duplicateTab(window, aTab);

You can still trim this a bit:

return Cc["@mozilla.org/browser/sessionstore;1"]
         .getService(Ci.nsISessionStore)
         .duplicateTab(window, aTab);
Attachment #469242 - Flags: review?(dao) → review+
(Assignee)

Comment 43

7 years ago
Created attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

Requesting approval for Firefox 4.
Attachment #469242 - Attachment is obsolete: true
Attachment #470611 - Flags: approval2.0?
(Assignee)

Comment 44

7 years ago
Comment on attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

(In reply to comment #42)
> This performs pretty badly. Can you call gBrowser.hideTab before calling
> replaceTabWithWindow so that the tab won't sit in the current window apparently
> waiting for the new window to open?

Apparently you can't move a hidden (by gBrowser.hideTab(newTab)) tab to a new window. Maybe the performance problem could be handled in a different bug (by making it possible to replace a hidden tab with a window)?

This means the latest patch should be without "gBrowser.hideTab(newTab)".
Attachment #470611 - Flags: approval2.0?
(In reply to comment #44)
> Apparently you can't move a hidden (by gBrowser.hideTab(newTab)) tab to a new
> window.

Why would it fail? It worked for me when I tested it.
(Assignee)

Comment 46

7 years ago
Comment on attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

(In reply to comment #45)
> (In reply to comment #44)
> > Apparently you can't move a hidden (by gBrowser.hideTab(newTab)) tab to a new
> > window.
> 
> Why would it fail? It worked for me when I tested it.

I've just discovered something went wrong with building the source. Which meant that I was somehow testing with the previous patch applied. But still strange shift+back button didn't create a new window. It did duplicate the tab cause I could see it using the new tab groups icon.

But re-asking approval because you've tested it :P.
Attachment #470611 - Flags: approval2.0?
Still not a blocker, no.
blocking2.0: ? → -
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review dao] → [has patch][has review]

Updated

7 years ago
No longer blocks: 455722

Updated

7 years ago
Blocks: 455722

Updated

7 years ago
Blocks: 594217
(Assignee)

Comment 48

7 years ago
Is there still a chance to get this in for Firefox 4?
Comment on attachment 470611 [details] [diff] [review]
patch v8.1 - final patch address issues from comment #42

a=beltzner, but it'll bounce if there's a regression

Klaas: can you write tests for this, too?
Attachment #470611 - Flags: approval2.0? → approval2.0+
Flags: in-testsuite?
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 50

7 years ago
Created attachment 474161 [details] [diff] [review]
Updated to tip for check-in [r=dao, ui-r+a=beltzner]

(In reply to comment #49)
> Comment on attachment 470611 [details] [diff] [review]
> patch v8.1 - final patch address issues from comment #42
> 
> a=beltzner, but it'll bounce if there's a regression
> 
> Klaas: can you write tests for this, too?

Dão, do you have an idea what sort of tests need to be written for this. Setting checkin-needed, but this bug shouldn't be resolved until tests are added.
Attachment #470611 - Attachment is obsolete: true

Comment 51

7 years ago
Bug 529240 does somethign for SeaMonkey, but slightly different way. You might be interested how duplicateTab was modified in this patch https://bugzilla.mozilla.org/attachment.cgi?id=470672

Comment 52

7 years ago
Sorry s/somethign/same thing/
http://hg.mozilla.org/mozilla-central/rev/dfbf5d853b89

(In reply to comment #50)
> Dão, do you have an idea what sort of tests need to be written for this.

A basic test would create a tab with history, call duplicateTabIn with tab/tabshifted and see what it does.

(In reply to comment #51)
> Bug 529240 does somethign for SeaMonkey, but slightly different way. You might
> be interested how duplicateTab was modified in this patch

Might be worth filing a new bug to see if we can simplify our code.
Whiteboard: [has patch][has review]
Target Milestone: --- → Firefox 4.0b6

Updated

7 years ago
Blocks: 595483

Updated

7 years ago
Keywords: checkin-needed
Should this be marked as fixed?
It hasn't landed has it?
(Assignee)

Comment 56

7 years ago
(In reply to comment #54)
> Should this be marked as fixed?

I guess not until tests are added, see comment #50.

(In reply to comment #55)
> It hasn't landed has it?

Yes it has.
Generally we put the hg links in the bug.

http://hg.mozilla.org/mozilla-central/rev/dfbf5d853b89
(In reply to comment #57)
> Generally we put the hg links in the bug.
> 
> http://hg.mozilla.org/mozilla-central/rev/dfbf5d853b89

Oh, apparently I'm just blind :-)
Is someone working on tests for this? Leaving the bug open indefinitely while the patch has landed is confusing. We need to resolve this bug, and also get some tests landed.

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Blocks: 593673

Updated

7 years ago
Depends on: 609700

Updated

6 years ago
Depends on: 623893
You need to log in before you can comment on or make changes to this bug.