If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Middle-click on empty folder in Bookmarks Toolbar or Library creates empty new tab

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
--
minor
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Rick, Assigned: dao)

Tracking

({polish, verified1.9.1})

3.0 Branch
Firefox 3.6a1
polish, verified1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy][polish-interactive][polish-p4])

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

This is very minor, but for thoroughness, I'll file it.
I also provided the (short) code to fix it below.

Points:
1. For the toolbar, this also occurs for any other kind of click that should produce a new tab, depending on how one's prefs are set up: Ctrl-left-click, Shift-left-click, Ctrl-shift-left-click.
2. For the library, only middle-clicks apply since Ctrl/Shift left-clicks are used for selection.
3. This includes folders that contain only other folders.
4. There is already code in the sidebar to prevent this from happening, so the toolbar and library should be consistent.

In fact, here is the self-tested code (in extension form).  I just did what the bookmarks sidebar does and put in a "hasChildURIs()" check before "openContainerNodeInTabs()" is called for each case.  Alternatively, you could put the check in "openContainerNodeInTabs()" itself (utils.js) which would take care of all the cases at once, though I haven't tried this.

1. For the Bookmarks Toolbar, change browser.js:

eval("BookmarksEventHandler.onClick ="+BookmarksEventHandler.onClick.toString().replace(
   /var target = aEvent.originalTarget;/,
   <><![CDATA[
    var target = aEvent.originalTarget;

    //RC Do not open blank new tab if container has nothing to open
    if (PlacesUtils.nodeIsContainer(target.node) && !PlacesUtils.hasChildURIs(target.node))
      return;
   ]]></>
));


2. For the Library, change places.js:

eval("PlacesOrganizer.onTreeClick ="+PlacesOrganizer.onTreeClick.toString().replace(
   /else if \(PlacesUtils.nodeIsContainer\(selectedNode\)\) {/,
   <><![CDATA[
      else if (PlacesUtils.nodeIsContainer(selectedNode)) {

        //RC Do not open blank new tab if container has nothing to open
        if (!PlacesUtils.hasChildURIs(selectedNode))
          return;
   ]]></>
));


Reproducible: Always

Steps to Reproduce:
1. Create empty folder (or contains only other folders), in BM toolbar or Library
2. Middle-click on it

Actual Results:  
Blank tab created.

Expected Results:  
Nothing should happen.

Comment 1

9 years ago
I can confirm this bug in last night's build (20080805). Confirm it affects the Bookmarks Menu and also in the Organize Bookmarks.
I am using Windows XP SP2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080504 Minefield/3.1a2pre

Bug 445301 and bug 260611 could be related (opening tabs in background)

Comment 2

9 years ago
In addition I noticed a small inconsistency.

When middle clicking to open tabs they all open in new tabs (leaving your current tab undisturbed) - but if you right click and select 'Open all in tabs' then your current tab is re-used.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

9 years ago
Whiteboard: [polish-easy][polish-interactive]
(Assignee)

Updated

9 years ago
Assignee: nobody → dao
Status: NEW → ASSIGNED
(Assignee)

Comment 3

9 years ago
Created attachment 357475 [details] [diff] [review]
patch

Note that the fix for _openTabset would be sufficient to fix this particular bug, but I figure it's better to make tabbrowser's loadTabs less dumb as well.
Attachment #357475 - Flags: review?(dietrich)
(Assignee)

Updated

9 years ago
Keywords: polish

Updated

9 years ago
Attachment #357475 - Flags: review?(gavin.sharp)
Attachment #357475 - Flags: review?(dietrich)
Attachment #357475 - Flags: review+
Comment on attachment 357475 [details] [diff] [review]
patch

you need also a review from a browser peer, i'm forwarding to gavin
Attachment #357475 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 3.0 Branch
(Assignee)

Comment 5

9 years ago
http://hg.mozilla.org/mozilla-central/rev/2da6386c98c0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
(Assignee)

Updated

9 years ago
Attachment #357475 - Flags: approval1.9.1?
Attachment #357475 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 357475 [details] [diff] [review]
patch

a191=beltzner
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 7

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d7e5f87ba8e7
Keywords: checkin-needed → fixed1.9.1
This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.
Whiteboard: [polish-easy][polish-interactive] → [polish-easy][polish-interactive][polish-p4]
verified FIXED on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090714 Shiretoko/3.5.1pre (.NET CLR 3.5.30729) ID:20090714052038

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090714 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090714054201
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.