Middle clicking on a tag in the Library left pane does not open in tabs

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
--
major
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: nightstalkerz, Assigned: mak)

Tracking

({regression, verified1.9.1})

Trunk
Firefox 3.6a1
regression, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre

Middle clicking on a tag in the Library cause the current tab to become blank

Reproducible: Always

Steps to Reproduce:
1. Open the Library (Bookmarks > Organize Bookmarks or Ctrl+Shift+B)
2. Middle click on a tag
Actual Results:  
The current tab in the browser becomes blank.

Expected Results:  
Items in the tag are opened as tabs like middle clicking on a folder.

The back button still works on the blank tab.
Component: Bookmarks → Places
QA Contact: bookmarks → places
confirmed
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008042806 Minefield/3.0pre

do we have a regression range?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted
I see it only in the left pane.
Since Bug 419731 you can expand "Recent Tags" in the left pane (it has a +).
When I click on "Recent Tags" and middle click on the tag in the right pane it loads normally.

Comment 3

10 years ago
It is the patch which is not smart. However, this may fix this problem.
chrome\browser\content\browser\places\utils.js

  openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) {
    var urlsToOpen = PlacesUtils.getURLsForContainerNode(aNode);
+   if (urlsToOpen.length < 1)return;
    if (!this._confirmOpenInTabs(urlsToOpen.length))
      return;

    this._openTabset(urlsToOpen, aEvent);
  },
(In reply to comment #3)
> It is the patch which is not smart. However, this may fix this problem.
> chrome\browser\content\browser\places\utils.js
> 
>   openContainerNodeInTabs: function PU_openContainerInTabs(aNode, aEvent) {
>     var urlsToOpen = PlacesUtils.getURLsForContainerNode(aNode);
> +   if (urlsToOpen.length < 1)return;
>     if (!this._confirmOpenInTabs(urlsToOpen.length))
>       return;
> 
>     this._openTabset(urlsToOpen, aEvent);
>   },

well, i already put that some time ago, but then we went with a different fix (trying to open correct contents rather than hiding the problem).
Hwv thank you for pointing that :)

Assignee: nobody → mak77
Blocks: 419731
Keywords: regression
Status: NEW → ASSIGNED
Summary: Middle clicking on a tag in the Library cause the current tab to become blank → Middle clicking on a tag in the Library left pane does not open in tabs
Keywords: qawanted
Created attachment 318398 [details] [diff] [review]
patch

- get contents manually when we inherit excludeItems from the root
- avoid mixing up var and let
- nullify and set the viewer in hasChildURIs like we do in getURLsForContainerNode before opening a container
Attachment #318398 - Flags: review?(mano)
Created attachment 318400 [details] [diff] [review]
patch

don't need to check if nodeIsContainer since a early return
Attachment #318398 - Attachment is obsolete: true
Attachment #318400 - Flags: review?(mano)
Attachment #318398 - Flags: review?(mano)
Comment on attachment 318400 [details] [diff] [review]
patch

r=mano
Attachment #318400 - Flags: review?(mano) → review+
Marco, what about the performance if the user has hundreds of bookmarks set with the same tag? Middle-click will open a huge amount of tabs within the browser which will probably result in a totally hang of Firefox for a longer period.

Also seen with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042922 Minefield/3.0pre
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk

Comment 9

10 years ago
We have browser.tabs.warnOnOpen;true to warn on opening multiple tabs
(In reply to comment #9)
> We have browser.tabs.warnOnOpen;true to warn on opening multiple tabs

At least there is no warning when doing the same with a folder located on the Bookmarks Toolbar. All contained bookmarks are opened in their own tab. That's why it should be handled with caution.

browser.tabs.warnOnOpen;true
browser.tabs.maxOpenBeforeWarn;5 (or less for testing. default value is 15)
i have the warning on folders in the toolbar.

reminder for me: before asking approval add a check for folder||tagQuery since not all containers can be treated like folders
Created attachment 318563 [details] [diff] [review]
patch v2

sorry Mano if i ask you another review, but i've seen that previous patch was not complete, this adds support for saved searches in the left pane, i'm considering 3 cases:
- folder||tag && excludeItems
- query && excludeItems
- other containers

the only thing that i cannot open in tabs are queries in the right pane, but i think that could be due to bug 430659
Attachment #318400 - Attachment is obsolete: true
Attachment #318563 - Flags: review?(mano)
You could fix these containers by un-setting expandQueries.
Created attachment 318573 [details] [diff] [review]
patch

this appear to be enough to open everything, i've created a saved search and opened it in tabs correctly.

Should we also take a fix to openTabset to avoid opening an empty tab if there is nothing in the urls array?
Attachment #318563 - Attachment is obsolete: true
Attachment #318573 - Flags: review?(mano)
Attachment #318563 - Flags: review?(mano)

Comment 16

10 years ago
(In reply to comment #15)
> Created an attachment (id=318573) [details]
> patch
> 
> this appear to be enough to open everything, i've created a saved search and
> opened it in tabs correctly.
> 
> Should we also take a fix to openTabset to avoid opening an empty tab if there
> is nothing in the urls array?
> 
browser.tabs.loadFolderAndReplace should be considered to open Tabs.

Comment 17

10 years ago
(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=318573) [details] [details]
> > patch
> > 
> > this appear to be enough to open everything, i've created a saved search and
> > opened it in tabs correctly.
> > 
> > Should we also take a fix to openTabset to avoid opening an empty tab if there
> > is nothing in the urls array?
> > 
> browser.tabs.loadFolderAndReplace should be considered to open Tabs.
> 
Please forget comment #16.

But, It should be considered bug 175124 comment 29; "middle click on folder appends tabset"
Comment on attachment 318573 [details] [diff] [review]
patch

most likely needs an unbitrot
Attachment #318573 - Flags: review?(mano)
Whiteboard: [needs new patch]
Target Milestone: --- → Firefox 3.1
Created attachment 362709 [details] [diff] [review]
patch v3.0

i refactored a bit the two functions splitting them in 2 parts, the first part opens a result and returns the root, the second part executes the task (return at first uri node or push into uris array).

works for all items in the left pane, tag containers and other generic queries.
Attachment #318573 - Attachment is obsolete: true
Attachment #362709 - Flags: review?(dietrich)
Whiteboard: [needs new patch]
Marco, do we use the same function as for bookmarks to open all uris in new tabs? If not, do we also use browser.tabs.maxOpenBeforeWarn here to warn the user?
(In reply to comment #20)
> Marco, do we use the same function as for bookmarks to open all uris in new
> tabs? If not, do we also use browser.tabs.maxOpenBeforeWarn here to warn the
> user?

sure, this only builds a list of urls, then those are passed to the common open function
Comment on attachment 362709 [details] [diff] [review]
patch v3.0

>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js
>--- a/toolkit/components/places/src/utils.js
>+++ b/toolkit/components/places/src/utils.js
>@@ -914,93 +914,135 @@ var PlacesUtils = {
>     var livemarks = annosvc.getItemsWithAnnotation(LMANNO_FEEDURI, {});
>     for (var i = 0; i < livemarks.length; i++) {
>       if (annosvc.getItemAnnotation(livemarks[i], LMANNO_FEEDURI) == feedSpec)
>         return livemarks[i];
>     }
>     return -1;
>   },
> 
>-  // Returns true if a container has uris in its first level
>-  // Has better performances than checking getURLsForContainerNode(node).length
>+  /**
>+   * Returns true if a container has uri nodes in its first level.
>+   * Has better performance than (getURLsForContainerNode(node).length > 0).
>+   * @param aNode
>+   *        The container node to search through.
>+   * @returns true if the node contains uri nodes, false otherwise.
>+   */
>   hasChildURIs: function PU_hasChildURIs(aNode) {
>     if (!this.nodeIsContainer(aNode))
>       return false;
> 
>-    // in the Library left pane we use excludeItems
>-    if (this.nodeIsFolder(aNode) && asQuery(aNode).queryOptions.excludeItems) {
>-      var itemId = PlacesUtils.getConcreteItemId(aNode);
>-      var contents = this.getFolderContents(itemId, false, false).root;
>-      for (var i = 0; i < contents.childCount; ++i) {
>-        var child = contents.getChild(i);
>-        if (this.nodeIsURI(child))
>-          return true;
>+    var root = null;
>+    var wasOpen = false;
>+    var oldViewer = null;
>+
>+    // excludeItems is inherited by child containers in an excludeItems view.
>+    var excludeItems = asQuery(aNode).queryOptions.excludeItems ||
>+                       asQuery(aNode.parentResult.root).queryOptions.excludeItems;
>+    // expandQueries is inherited by child containers in an expandQueries view.
>+    var expandQueries = asQuery(aNode).queryOptions.expandQueries &&
>+                        asQuery(aNode.parentResult.root).queryOptions.expandQueries;
>+
>+    if (this.nodeIsFolder(aNode) && excludeItems) {
>+      // Get folder contents manually.
>+      root = this.getFolderContents(this.getConcreteItemId(aNode),
>+                                    false, false).root;
>+    }
>+    else if (this.nodeIsQuery(aNode) && (excludeItems || !expandQueries)) {
>+      // Get query contents manually.
>+      var queries = {}, options = {};
>+      this.history.queryStringToQueries(aNode.uri, queries, {}, options);
>+      root = this.history.executeQueries(queries.value,
>+                                         queries.value.length,
>+                                         options.value).root;
>+      root.containerOpen = true;
>+    }
>+    else {
>+      wasOpen = aNode.containerOpen;
>+      root = aNode;
>+      if (!wasOpen) {
>+        oldViewer = root.parentResult.viewer;
>+        root.parentResult.viewer = null;
>+        root.containerOpen = true;
>       }
>-      return false;
>     }

all this bit is shared with getURLsForContainerNode, right? possible to put it in something like _getUsableRootForContainerNode()?

> 
>-    var wasOpen = aNode.containerOpen;
>-    if (!wasOpen)
>-      aNode.containerOpen = true;
>     var found = false;
>-    for (var i = 0; i < aNode.childCount && !found; i++) {
>-      var child = aNode.getChild(i);
>+    for (var i = 0; i < root.childCount && !found; i++) {
>+      var child = root.getChild(i);
>       if (this.nodeIsURI(child))
>         found = true;

could optimize and break here, yeah?

>     }
>+
>     if (!wasOpen)
>       aNode.containerOpen = false;
>+    if (oldViewer)
>+      root.parentResult.viewer = oldViewer;
>+
>     return found;
>   },
> 
>-  getURLsForContainerNode: function PU_getURLsForContainerNode(aNode) {
>-    let urls = [];
>-    if (this.nodeIsFolder(aNode) && asQuery(aNode).queryOptions.excludeItems) {
>-      // grab manually
>-      var itemId = this.getConcreteItemId(aNode);
>-      let contents = this.getFolderContents(itemId, false, false).root;
>-      for (let i = 0; i < contents.childCount; ++i) {
>-        let child = contents.getChild(i);
>-        if (this.nodeIsURI(child))
>-          urls.push({uri: child.uri, isBookmark: this.nodeIsBookmark(child)});
>+  /**
>+   * Returns an array containing all the uris in the first level of the
>+   * passed in container.
>+   * If you only need to know if the node contains uris, use hasChildURIs.
>+   * @param aNode
>+   *        The container node to search through
>+   * @returns array of uris in the first level of the container.
>+   */
>+  getURLsForContainerNode: function PU_getURLsForContainerNod(aNode) {

s/Nod/Node/

r=me, with a test added.
Attachment #362709 - Flags: review?(dietrich) → review+
(In reply to comment #22)

> > 
> >-    var wasOpen = aNode.containerOpen;
> >-    if (!wasOpen)
> >-      aNode.containerOpen = true;
> >     var found = false;
> >-    for (var i = 0; i < aNode.childCount && !found; i++) {
> >-      var child = aNode.getChild(i);
> >+    for (var i = 0; i < root.childCount && !found; i++) {
> >+      var child = root.getChild(i);
> >       if (this.nodeIsURI(child))
> >         found = true;
> 
> could optimize and break here, yeah?

there's no need, see the !found check in the loop condition
Created attachment 364185 [details] [diff] [review]
patch v4.0

another refactoring, with a unit test
Attachment #362709 - Attachment is obsolete: true
Attachment #364185 - Flags: review?(dietrich)
Flags: in-testsuite?
Attachment #364185 - Flags: review?(dietrich) → review+
Comment on attachment 364185 [details] [diff] [review]
patch v4.0


>+  /**
>+   * Returns a nsNavHistoryContainerResultNode with forced excludeItems and
>+   * expandQueries.
>+   * @param   aNode
>+   *          The node to convert
>+   * @param   [optional] excludeItems
>+   *          True to hide all items (individual bookmarks). This is used on
>+   *          the left places pane so you just get a folder hierarchy.
>+   * @param   [optional] expandQueries
>+   *          True to make query items expand as new containers. For managing,
>+   *          you want this to be false, for menus and such, you want this to
>+   *          be true.
>+   * @returns A nsINavHistoryContainerResultNode containing the unfiltered
>+   *          contents of the container.
>+   * @note    The returned container node could be open or closed, we don't
>+   *          guarantee its status.
>+   */
>+  getUnfilteredContainerNode:
>+  function PU_getUnfilteredContainerNode(aNode, aExcludeItems, aExpandQueries) {

"unfiltered" doesn't sound right, considering you can pass parameters that filter the result.

r=me otherwise
Created attachment 365170 [details] [diff] [review]
patch v4.1

addressed comment
Attachment #364185 - Attachment is obsolete: true
Blocks: 481512
http://hg.mozilla.org/mozilla-central/rev/82d502128473
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Verified fixed on trunk with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre Ubiquity/0.1.5 ID:20090310030639

Would the latest patch be ready to ask approval for 1.9.1?
Status: RESOLVED → VERIFIED
Flags: wanted-firefox3.1?
Attachment #365170 - Flags: approval1.9.1?
Comment on attachment 365170 [details] [diff] [review]
patch v4.1

sure, asking approval 1.9.1, low risk, comes with a test, a nice to have polish.
Flags: in-testsuite? → in-testsuite+
Duplicate of this bug: 478747
Comment on attachment 365170 [details] [diff] [review]
patch v4.1

a191=beltzner
Attachment #365170 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b28e83ae2f83
Flags: wanted-firefox3.5?
Keywords: fixed1.9.1
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre)
Gecko/20090414 Shiretoko/3.5b4pre
Keywords: fixed1.9.1 → verified1.9.1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.