Closed
Bug 133590
Opened 22 years ago
Closed 21 years ago
[RFE] in history, add right-click > open in new tab
Categories
(Core Graveyard :: History: Global, enhancement)
Core Graveyard
History: Global
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: earthsound, Assigned: durbacher)
References
Details
Attachments
(1 file, 4 obsolete files)
10.36 KB,
patch
|
neil
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
It would be nice to be able to open pages in history into a new tab :)
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•22 years ago
|
||
I assume you mean the sidebar here, so marking dependency.
Depends on: 131068
Reporter | ||
Comment 2•22 years ago
|
||
Yes, I was referring to the sidebar's History panel.
*** Bug 146511 has been marked as a duplicate of this bug. ***
Comment 4•22 years ago
|
||
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•22 years ago
|
||
*** Bug 173290 has been marked as a duplicate of this bug. ***
Comment 6•22 years ago
|
||
*** Bug 176392 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 7•22 years ago
|
||
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•21 years ago
|
||
*** Bug 164777 has been marked as a duplicate of this bug. ***
Comment 9•21 years ago
|
||
Shouldn't Hardware and OS be set to "All"? Prog.
Reporter | ||
Comment 10•21 years ago
|
||
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)...
Assignee | ||
Comment 11•21 years ago
|
||
No, neither fixed in history window, nor sidebar.
Updated•21 years ago
|
OS: Windows 98 → All
Hardware: PC → All
Assignee | ||
Comment 12•21 years ago
|
||
This patch fixes the bug and several other small issues I found. See next comment for details.
Assignee | ||
Comment 13•21 years ago
|
||
Umm... one small error corrected, details delayed until next comment...
Assignee | ||
Updated•21 years ago
|
Attachment #138582 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Assignee: blake → durbacher
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•21 years ago
|
||
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...
Attachment #138584 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 15•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
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!)
Attachment #138584 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee | ||
Comment 18•21 years ago
|
||
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
Assignee | ||
Comment 19•21 years ago
|
||
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.
Attachment #138899 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 20•21 years ago
|
||
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;
Attachment #138899 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee | ||
Comment 21•21 years ago
|
||
Apart from one-line-if's only the OpenURL and OpenURLArrayInTabs parts have changed. But those a lot.
Assignee | ||
Updated•21 years ago
|
Attachment #138899 -
Attachment is obsolete: true
Assignee | ||
Comment 22•21 years ago
|
||
Comment on attachment 139068 [details] [diff] [review] next patch: comments from review and IRC talk addressed Requesting r= from Neil.
Attachment #139068 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 23•21 years ago
|
||
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.
Assignee | ||
Comment 24•21 years ago
|
||
Those two comments addressed.
Attachment #139068 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #139068 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•21 years ago
|
Attachment #139229 -
Flags: review?(neil.parkwaycc.co.uk)
Updated•21 years ago
|
Attachment #139229 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Comment 25•21 years ago
|
||
Comment on attachment 139229 [details] [diff] [review] updated patch Hooray! :-) Requesting sr= from alecf.
Attachment #139229 -
Flags: superreview?(alecf)
Comment 26•21 years ago
|
||
Comment on attachment 139229 [details] [diff] [review] updated patch neat! sr=alecf
Attachment #139229 -
Flags: superreview?(alecf) → superreview+
Comment 27•21 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 29•19 years ago
|
||
You forgot to add accel-click to open in new tab too. (Mozilla 1.7.6)
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•