Closed Bug 133590 Opened 22 years ago Closed 21 years ago

[RFE] in history, add right-click > open in new tab


(Core Graveyard :: History: Global, enhancement)

Not set


(Not tracked)



(Reporter: earthsound, Assigned: durbacher)




(1 file, 4 obsolete files)

It would be nice to be able to open pages in history into a new tab :)
Ever confirmed: true
I assume you mean the sidebar here, so marking dependency.
Depends on: 131068
Yes, I was referring to the sidebar's History panel.
*** Bug 146511 has been marked as a duplicate of this bug. ***
I'd like to suggest that this also should apply to the normal "Back" button - a
middle click on the button opens in a new tab/new window, and a ctrl-middle
opens in a new tab (or vise-versa based on prefs setting for tabbed browsing).
On the drop-menu on the back button, middle clicking one of the entries opens
that entry as above.
*** Bug 173290 has been marked as a duplicate of this bug. ***
*** Bug 176392 has been marked as a duplicate of this bug. ***
Wrt comment 1, this is an RFE for both the sidebar history panel as well as the
history window as viewed by selecting Go>History or pressing Ctrl+H. As such, it
would not depend on bug 131068, and as that bug has been fixed, maybe someone
has an idea of the feasibility of this RFE coming into fruition. :)
*** Bug 164777 has been marked as a duplicate of this bug. ***
Shouldn't Hardware and OS be set to "All"?

This is fixed in firebird. Maybe one of you who still use mozilla could confirm
whether it's fixed there, too (i.e. in the history window, not just sidebar)...
No, neither fixed in history window, nor sidebar.
OS: Windows 98 → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
This patch fixes the bug and several other small issues I found. See next
comment for details.
Attached patch updated patch (obsolete) — Splinter Review
Umm... one small error corrected, details delayed until next comment...
Attachment #138582 - Attachment is obsolete: true
Assignee: blake → durbacher
Comment on attachment 138584 [details] [diff] [review]
updated patch

Requesting r= from Neil.

This patch introduces the additional context menu entry for opening new tabs
from history sidebar and history window.
Inspired by the patch for bug 103834 it changes the parameter of OpenURL from
boolean to string to allow for more than two possible values. First I didn't
like it, but hey, the other patch got review, so maybe it's ok to do this.
Then it introduces a new function "OpenNewTab".
And finally there are some changes that determine the state of the context menu
entry depending on what is selected.

Here the problems began: I discovered several glitches in the existing context
menu and fixed them:
- expand/collapse are not always hidden when several entries (but no folder)
are selected
- "Collapse" amd "Open in new window" can both be default (bold) at the same
- "Open in new window" might be hidden erroneously
- second separator may be missing
these do happen when the context menu entry has a certain state (e.g.
"collapse" is shown for a folder) but that state does not get changed in the
transition to the context menu for a different item (e.g. now several regular
entries are selected and "collapse" is not hidden)

I also found several other issues you might encounter when testing this (I'll
note them so you don't think it was my patch that caused them...):
- in history sidebar the "collapse"/"expand" entry is empty because of a
missing stringbundle. This is bug 131182.

- "gHistoryStatus has no properties": bug 227053 (some code even relies on it
being not defined, other code still happily uses it!)

- "JS Error: prefBranch.addObserver is not a function" this was fixed by the
last patch in bug 223908 , only a few hours ago, so chances are you still have
that one. (and introduced end-December)

- when several history entries are selected, the 'bookmark this' menu entry has
a misleading label, this is bug 230349.

- when you switch from grouping by days to "no grouping" in the "View" menu,
the view is updated, but the internal variable that is used for looking if view
is grouped is not updated. This eventually leads to wrong context menu entries
directly after switching view and the possibility to load history folders in
tabs (they are empty then). Closing and reopening the history window helps.
This is bug 230357.

Neil, this is my first patch of this complexity, so please do not be too harsh,
but I'm certainly prepared for several modifications. Just say what's not good.
In addition I have three questions for your review:

1) Do I need some sort of security check for opening new tabs or windows?
something like
? History already opens new windows without doing so, bookmarks do it too. So I
think I do not need it. On the other hand Firebird history has something like
I just do not want to introduce a security problem, so...

2) design decision: should the browser window with the newly opened tabs get
focus? Bookmarks do it:
But I decided not to, but read the next question first...

3) should the new tab be selected (i.e. in front)? Again, bookmarks do it:
and I think it makes sense because it is the thing that is probably looked at
next. And it turned out that this also automatically gives focus to the window
so it comes to front. Well, it's ok with me... so it's consistent with
Another possibility could be to ask the tabbed browsing preferences, but
bookmarks also don't do it and IF such a thing is desired, it could be a
beautiful new RFE.

Finally, I have noticed one problem with the patch:
If there is _no_ browser window open and you open multiple history pages "in
new tab" at once (by selecting more than one entry) then they are opened in new
windows instead of tabs. Might this happen because the WindowMediator is too
slow after opening a new window for the first tab? It is asked for each tab if
there is already a open browser window, but seems to say "no" all the time
because he is asked again and again very fast and has not yet noticed that
there is a new window open...
Could this be a separate bug? I don't know how to fix this...
Attachment #138584 - Flags: review?(
BTW: the last mentioned problem (multiple "new Tab" open new windows when no
browser window is open) also happens in the bookmark manager.
Comment on attachment 138584 [details] [diff] [review]
updated patch

>-        OpenURL(false);
>+        OpenURL("this_window");
I think bookmarks uses "current" (and "window" and "tab")...

>+      sep2.setAttribute("hidden", "true");
It would be neater to use .hidden = true; instead...

>+            onkeypress="if (event.keyCode == 13) { 
>+                          if (event.ctrlKey || event.metaKey) 
>+                            OpenUrl('new_window');
>+                          else 
>+                            OpenUrl('this_window'); 
>+                        }" 
It would be neater to use ?: instead i.e.
OpenUrl(event.ctrlKey || event.metaKey ? "window" : "current");
Comment on attachment 138584 [details] [diff] [review]
updated patch

>+          if (aTarget == "new_window") {
>+            openDialog( getBrowserURL(), "_blank", "chrome,all,dialog=no", url );
>+          } else {
>+            OpenNewTab(url);
>+          }
No need for {}s for a single line, at least in this file...

>+    gWindowManager = Components.classes[';1'].getService();
>+    gWindowManager = gWindowManager.QueryInterface( Components.interfaces.nsIWindowMediator);
Should be
(and fix the incorrect code you copied too ;-)

>+    browser = browserWin.getBrowser();
Undeclared variable, surely?

>         openItem.setAttribute("hidden", "true");
>         openItem.removeAttribute("default");
>         openItemInNewWindow.setAttribute("default", "true");
>+        openItemInNewTab.removeAttribute("hidden");
>     }
>     else {
>       openItem.removeAttribute("hidden");
>       if (!openItem.getAttribute("default"))
>         openItem.setAttribute("default", "true");
>       openItemInNewWindow.removeAttribute("default");
>+      openItemInNewWindow.removeAttribute("hidden");
>+      openItemInNewTab.removeAttribute("hidden");
You seem to be doing this in both the if and the else, but so is the existing
code... sigh...

A "workaround" for open multiple tabs in a new window is to join all the URLs
together with "\n"s and open a new window with the joined string, that should
then make the browser open the group in a new window (which could be useful for
bookmark groups too!)
Attachment #138584 - Flags: review?( → review-
Well tested, no doubt about its functionality, more about its niftyness...

When testing with current builds, don't thy bringing up the context menu for
history entries using that "Windows" key on your keyboard: it will crash
Mozilla due to bug 229624 / bug 230380.
Attachment #138584 - Attachment is obsolete: true
Comment on attachment 138899 [details] [diff] [review]
new patch; review comments addressed, some ".hidden" cleanup

Requesting Neil's comments.

This "several new tabs in new window" workaround really makes things more
I have to know if there is an open browser window _before_ I look at the
selected history entries for opening them in new tabs. So all that
WindowManager stuff has to be done early in OpenURL.
My new OpenNewTab does it again because of users that don't check for an
available window. I could do the WindowManager things very early in OpenURL and
avoid it in OpenNewTab. Actually I could do all the things of OpenNewTab in
OpenURL and I have a version of the patch that does this.
But OpenURL gets quite ugly this way.
Attachment #138899 - Flags: review?(
Comment on attachment 138899 [details] [diff] [review]
new patch; review comments addressed, some ".hidden" cleanup

Your attempt to integrate the new tab code with the new window code is just too
messy. Please put the new tab code in its own else if, i.e.
-    if (aInNewWindow) {
+    if (aTarget == "window") {
+    else if (aTarget == "tab") {

Also, only the first new tab should be selected.

You might find that getTopWin() is available to find a browser window.

You should stick to this form for one-line if statements:

if (cond)
Attachment #138899 - Flags: review?( → review-
Apart from one-line-if's only the OpenURL and OpenURLArrayInTabs parts have
changed. But those a lot.
Attachment #138899 - Attachment is obsolete: true
Comment on attachment 139068 [details] [diff] [review]
next patch: comments from review and IRC talk addressed

Requesting r= from Neil.
Attachment #139068 - Flags: review?(
Comment on attachment 139068 [details] [diff] [review]
next patch: comments from review and IRC talk addressed

+  } else
Nit: I don't like } else, please put in some {}s.
(On the other hand, I have no problem with else {. Go figure :-)

>+    if (!gWindowManager)
>+      gWindowManager = Components.classes[';1'].getService(Components.interfaces.nsIWindowMediator);
>     var topWindowOfType = gWindowManager.getMostRecentWindow("navigator:browser");
>     if (!topWindowOfType) {
Sorry for not spotting this before, would you mind changing this to if
(!getTopWin()) and removing gWindowManager? Thanks.
Attached patch updated patchSplinter Review
Those two comments addressed.
Attachment #139068 - Attachment is obsolete: true
Attachment #139068 - Flags: review?(
Attachment #139229 - Flags: review?(
Attachment #139229 - Flags: review?( → review+
Comment on attachment 139229 [details] [diff] [review]
updated patch

Hooray! :-)
Requesting sr= from alecf.
Attachment #139229 - Flags: superreview?(alecf)
Comment on attachment 139229 [details] [diff] [review]
updated patch

neat! sr=alecf
Attachment #139229 - Flags: superreview?(alecf) → superreview+
Checking in xpfe/components/history/resources/history.js;
/cvsroot/mozilla/xpfe/components/history/resources/history.js,v  <--  history.js
new revision: 1.62; previous revision: 1.61
Checking in xpfe/components/history/resources/historyTreeOverlay.xul;
/cvsroot/mozilla/xpfe/components/history/resources/historyTreeOverlay.xul,v  <--
new revision: 1.35; previous revision: 1.34
Closed: 21 years ago
Resolution: --- → FIXED
Verified using the latest nightly.
You forgot to add accel-click to open in new tab too. (Mozilla 1.7.6)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.