327034 - "Open in Tabs" gone from bookmark menus

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Pam Greene]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20060131 Firefox/1.5

The separator line and "Open in Tabs" no longer appear in bookmark toolbar menus.

Reproducible: Always

Steps to Reproduce:
1. Click on a folder in the bookmark toolbar, e.g. "Latest Headlines"

Actual Results:  
No "Open in Tabs"

Expected Results:  
"Open in Tabs" after separator line at bottom

This is the 20060131 Places build, posted as the 20060128 nightly.

Comment 1

[Jon Henry]

This is perhaps Mac only, I see Open in Tabs on a self-built 20060216 Windows build. However, the menu item doesn't actually work, which is another bug.

Comment 2

[pal-moz]

Same with Windows Build.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060216 Firefox/1.6a1 ID:2006021605

Comment 3

[Annie Sullivan]

Attachment: Implements "open in tabs" menuitem

When I was testing this patch, I realized that often the bookmarks menu and submenus get hidden when you select an item from their context menu.  This causes openContainer to get set to false, which helps performance, but the command event doesn't happen until after openContainer is already false. This causes an exception every time node.childCount is accessed.  So I added checks around accesses of childCount in controller.js.  If you know of a cleaner way to do this, please let me know!

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 213701 [details] [diff] [review]
Implements "open in tabs" menuitem

>Index: browser/components/places/content/menu.xml
>+              if (this._endMarker > 0)
>+                this._endMarker--;

nit: prefer preincrement to post, if possible. 

>@@ -168,16 +176,17 @@
>               }
>               else if (PlacesController.nodeIsContainer(child)) {
>                 element = document.createElementNS(XULNS, "menu");
>                 element.setAttribute("type", "menu");
>                 element.setAttribute("container", "true");
>                 element.setAttribute("label", child.title);
>                 var popup = document.createElementNS(XULNS, "menupopup");
>                 popup.setAttribute("type", "places");
>+                popup.setAttribute("openInTabs", "true"); // Include menu option to open in tabs.
>                 element.appendChild(popup);

Aha! You should only set openInTabs for child elements _if_ you yourself have openInTabs set. Otherwise, what will happen is that if you create a view elsewhere for another purpose, if there are submenus, they will have the item at the base even if the root doesn't. 

>+            var f = function(event) {
>+              if (event.target.nodeName == "menuitem" &&
>+                  event.target.parentNode == t) {
>+                t._selection = t._resultNode;
>+                PlacesController.activeView = t;
>+                PlacesController.openLinksInTabs();

You can't make the view call the controller directly to perform this action, because you prevent this view from being used for anything else, e.g. in the Places Browse Popup, where the user selects what the view will be rooted on, we don't want the selection to be opened in tabs. 

Instead, the person who instantiates the view needs to add a command handler and get the selection and dispatch that way. That way the bookmarks menus in the browser can do one thing and views elsewhere can do another. 

>Index: browser/components/places/content/toolbar.xml
>+              popup.setAttribute("openInTabs", "true"); // Include menu option to open in tabs.

This is fine, I think. 

>Index: browser/components/places/content/controller.js
>   openLinksInTabs: function PC_openLinksInTabs() {
>     var node = this._activeView.selectedNode;
>     if (this._activeView.hasSingleSelection && this.nodeIsFolder(node)) {
>       asFolder(node);
>+      var wasOpen = node.containerOpen;
>+      node.containerOpen = true;
>       var cc = node.childCount;
>       for (var i = 0; i < cc; ++i) {
>         var childNode = node.getChild(i);
>         if (this.nodeIsURI(childNode))
>           this._activeView.browserWindow.openNewTabWith(childNode.uri,
>               null, null);
>       }
>+      node.containerOpen = wasOpen;

What's the effect of this change?

Comment 5

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060301 Firefox/1.6a1 ID:2006030202

Bookmarks>folder
right-click>Open in Tabs

this does not work.

Comment 6

[Annie Sullivan]

> Aha! You should only set openInTabs for child elements _if_ you yourself have
> openInTabs set. Otherwise, what will happen is that if you create a view
> elsewhere for another purpose, if there are submenus, they will have the item
> at the base even if the root doesn't. 

The reason I did this was for the Bookmarks Menu -- it doesn't have an Open in Tabs at the root (and I didn't think it should), but the subfolders should have them, right?  How should we handle this case?

> >Index: browser/components/places/content/controller.js
> >   openLinksInTabs: function PC_openLinksInTabs() {
> >     var node = this._activeView.selectedNode;
> >     if (this._activeView.hasSingleSelection && this.nodeIsFolder(node)) {
> >       asFolder(node);
> >+      var wasOpen = node.containerOpen;
> >+      node.containerOpen = true;
> >       var cc = node.childCount;
> >       for (var i = 0; i < cc; ++i) {
> >         var childNode = node.getChild(i);
> >         if (this.nodeIsURI(childNode))
> >           this._activeView.browserWindow.openNewTabWith(childNode.uri,
> >               null, null);
> >       }
> >+      node.containerOpen = wasOpen;
> 
> What's the effect of this change? 

See comment 3.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Maybe change openInTabs="true" to openInTabs="all|children"

all = this and children
children = child folders only...

?

Comment 8

[Annie Sullivan]

Attachment: Addresses Ben's comments

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 213830 [details] [diff] [review]
Addresses Ben's comments

>Index: browser/components/places/content/menu.xml
>+        // Only handle middle-click; left-click is handled by command event.
>+        if (this.eventValid(event) && event.button == 1) {
>+          PlacesController.mouseLoadURI(event);
>+          // Menus selected with middle click must be closed manually.

You should not be calling mouseLoadURI from within menu.xml. That was the point of my comment last time. You need to attach this handler at the root level in the browser.xul code that instantiates the root level menu, or toolbar. People may want to use these menus for other purposes than just loading links, and by merging the controller with the view you break the architectural advantage of MVC. 

>Index: browser/components/places/content/commands.inc
>+      <command id="placesCmd_open:tabsEnabled" label="&cmd.open_tabs.label;" accesskey="&cmd.open_tabs.accesskey;"
>+               oncommand="PlacesController.openLinksInTabs(); return true;"/>

Was the trailing return true; from our failed debugging jaunt? Or really necessary? If it's not necessary remove it. 

Also, what does the id "placesCmd_open:tabsEnabled" imply? I'm confused. It sounds like a command that is always enabled to me. If this is correct, you should document why there is another command that does this function, and why it exists, so no one accidentally removes it later thinking they're simplifying. 

>Index: browser/base/content/browser-menubar.inc
>-    <menupopup id="bookmarksMenuPopup" type="places" context="placesContext">
>+    <menupopup id="bookmarksMenuPopup"
>+               type="places"
>+               context="placesContext"
>+               openInTabs="children">

THIS is where you should have something that calls mouseLoadURI, e.g.:

>+    <menupopup id="bookmarksMenuPopup"
>+               type="places"
>+               context="placesContext"
>+               openInTabs="children"
>+               oncommand="PlacesController.mouseLoadURI(event.target);">

... since that's application/controller specific. Same for the toolbar instantiation. 

Not yet ready for r+. Sorry this is dragging but I want to make sure the architecture is clean so that we can easily meet our a2 deliverables.

Comment 10

[Annie Sullivan]

Attachment: Implements "open in tabs" menuitem, and suggested architecture changes

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 213873 [details] [diff] [review]
Implements "open in tabs" menuitem, and suggested architecture changes

r=[e-mail]

You rock, Annie!

I'm going to file a separate bug to look into moving the Open in Tabs function out of the view binding altogether, since that's a cleaner representation of our MVC architecture... but it doesn't really matter for right now.

Comment 12

[pal-moz]

"Open in Tabs" work again, thnks.

but first window still there,not replaced.
ie:
open one window/tab called (a1)
and open (a2)(a3)(a4) by "Open in Tabs",
tabs are (a1)(a2)(a3)(a4)

before these are (a2)(a3)(a4)

Comment 13

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv