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

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, enhancement)

Description

[earthsound]

It would be nice to be able to open pages in history into a new tab :)

Comment 1

[[e-mail]]

I assume you mean the sidebar here, so marking dependency.

Comment 2

[earthsound]

Yes, I was referring to the sidebar's History panel.

Comment 3

[R.K.Aa.]

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

Comment 4

[david.hagood]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[earthsound]

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. :)

Comment 8

[Vidar Haarr (not reading bugmail)]

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

Comment 9

[Prognathous]

Shouldn't Hardware and OS be set to "All"?

Prog.

Comment 10

[earthsound]

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)...

Comment 11

[Andreas Kunz]

No, neither fixed in history window, nor sidebar.

Comment 12

[Andreas Kunz]

Attachment: patch

This patch fixes the bug and several other small issues I found. See next
comment for details.

Comment 13

[Andreas Kunz]

Attachment: updated patch

Umm... one small error corrected, details delayed until next comment...

Comment 14

[Andreas Kunz]

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
time
- "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
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/con
tentAreaUtils.js#54
? 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
that:
http://lxr.mozilla.org/mozilla/source/browser/components/history/content/histor
y.js#107
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:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/boo
kmarks.js#567
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:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/boo
kmarks.js#566
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
bookmarks...
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...

Comment 15

[Andreas Kunz]

BTW: the last mentioned problem (multiple "new Tab" open new windows when no
browser window is open) also happens in the bookmark manager.

Comment 16

[[e-mail]]

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 17

[[e-mail]]

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['@mozilla.org/appshell/window-mediator;1'].getService();
>+    gWindowManager = gWindowManager.QueryInterface( Components.interfaces.nsIWindowMediator);
Should be
].getService(Components.interfaces.nsIWindowMediator);
(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!)

Comment 18

[Andreas Kunz]

Attachment: new patch; review comments addressed, some ".hidden" cleanup

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.

Comment 19

[Andreas Kunz]

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
difficult...
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.

Comment 20

[[e-mail]]

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)
  stmt;

Comment 21

[Andreas Kunz]

Attachment: next patch: comments from review and IRC talk addressed

Apart from one-line-if's only the OpenURL and OpenURLArrayInTabs parts have
changed. But those a lot.

Comment 22

[Andreas Kunz]

Comment on attachment 139068 [details] [diff] [review]
next patch: comments from review and IRC talk addressed

Requesting r= from Neil.

Comment 23

[[e-mail]]

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['@mozilla.org/appshell/window-mediator;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.

Comment 24

[Andreas Kunz]

Attachment: updated patch

Those two comments addressed.

Comment 25

[Andreas Kunz]

Comment on attachment 139229 [details] [diff] [review]
updated patch

Hooray! :-)
Requesting sr= from alecf.

Comment 26

[Alec Flett]

Comment on attachment 139229 [details] [diff] [review]
updated patch

neat! sr=alecf

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

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
done
Checking in xpfe/components/history/resources/historyTreeOverlay.xul;
/cvsroot/mozilla/xpfe/components/history/resources/historyTreeOverlay.xul,v  <--
 historyTreeOverlay.xul
new revision: 1.35; previous revision: 1.34
done

Comment 28

[Andreas Kunz]

Verified using the latest nightly.

Comment 29

[Joachim Noreiko]

You forgot to add accel-click to open in new tab too. (Mozilla 1.7.6)