firefox 3 beta 3: middle click on bookmark folder does not "open all in tabs"

Is this bug about not being able to D&D a URL into a sub-folder (on the bookmarks toolbar) (folder doesn't "open" on hover")?

Does "Places" not have an owner?

(In reply to comment #1)
>
Indeed. Main folders don't expand so you can't open sub-folders as well.

This affects dragging a link to the "Bookmarks" menu as well here.
Tried with a clean profile and clean firefox folder.

When I release the button (mouse up) on the "Bookmarks" menu, it will be added to the bottom of the list and the menu will open for a very brief period and close.

Could this be a result of bug #203573 ?
Which addresses the UI freeze of firefox during dragging.

This can not be bug 203573, as there was no patch submitted there (yet) !

(In reply to comment #5)
> This can not be bug 203573, as there was no patch submitted there (yet) !
>

Because there isn't a fix for that bug this bug can't be a result of that bug? That makes no sense.
From that bug I quote: "the workaround fires events after the end of the drag
session only.". That is exactly what I am seeing when dragging links to the bookmarks menu.

Bug 203573 was filed years before places was born and this function only regressed after 10 May 2005, like described in comment 0.

Re-checked, and the regression range is correct. Has someone an idea what bug else could have caused this except for bug 337305 - which still seems the most likely culprit to me?

if bug 326273 fits into regression range it seems like a likely candidate, it caused other d&d related regressions too

What I see is that the pre-Threadmanager changes drags didn't block the updates of the content of the firefox window (during page-load etc). After the patch was checked in: the content and all other UI elements get blocked until the drag is ended.

(In reply to comment #10)
>
What patch exactly in the range of comment 0 do you suspect Ger?

Looks like a thread manager regression. This WFM if I drag the link out of another Fx instance.

Neil, can you take a look at this?

My guess is that the timer isn't being fired during a drag.

Seeing this in Thunderbird as well.
Dragging an email over an collapsed folder will not open the folder.

The timer callbacks only get called if the disabled code in nsBaseAppShell::NativeEventCallback is reenabled, but that causes a significant hit to performance. I really can't say what code should be instead though.

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

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

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

I'm seeing this behavior since Places has been enabled. The following message appears in the Error Console (Console2):

Error: Cannot modify properties of a WrappedNative = NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN
Source file: chrome://downbar/content/downbaroverlay.js
Line: 192

Here's my current build ID, although I've been seeing it since place was enabled a few days ago on the trunk.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070528 Minefield/3.0a5pre ID:2007052818 [cairo]

Sorry for the bug spam, but I think the error message in <A HREF="https://bugzilla.mozilla.org/show_bug.cgi?id=337761#c20">my previous post</A> is caused by the download statusbar extension, and not the bookmarks problem. However, I'm still having a problem with folders not expanding on the bookmarks toolbar or the bookmarks menu itself.

Neil, reassining to you per the granparadiso meeting. If you can't get to this let me/Damon know and we'll play developer-roulette again ;-)

I've already investigated this comment 16. I really won't be able to understand the thread code in any short timeframe.

(In reply to comment #15)
> Seeing this in Thunderbird as well.
> Dragging an email over an collapsed folder will not open the folder.

That's been filed as bug 338401; I'm assuming it'll get fixed when this does.

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

Strange, WFM now.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072504 Minefield/3.0a7pre

I think this is a duplicate of Bug 299185.

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

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

(In reply to comment #14)
> My guess is that the timer isn't being fired during a drag.
>

Yeah, it looks like it isn't being fired until the click is released, instead; at least, that is how the UI is acting in practice. (and sorry if I am stating the obvious, just thought I'd add this for the sake of completion).

In the Bookmarks Organizer, go to Bookmarks Toolbar Folder, then Places, then Recently Used Tags. In the right-hand pane (if you have created any tags) there will be a list of tags. Right click on one of those.

When I do that, the following error dialog is generated. The context menu shows up after the dialog is dismissed.

---
ASSERT: Node QI Failed
Stack Trace:
0:QI_node([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)],nsINavHistoryQueryResultNode)
1:asQuery([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
2:PU_getURLsForContainerNode([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
3:PC_buildContextMenu([object XULElement])
4:buildContextMenu([object XULElement])
5:onpopupshowing([object MouseEvent])
---

i think this might be because the tag containers are a generic container object type, but have a result type of a folder:

http://mxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#4220

if i change the result type to RESULT_TYPE_HOST, for example, the assert goes away and the context menu is properly built.

i'd rather not add a new specialized result type for tag containers. instead we should add a generic container type, not specific to any particular content type.

(i'd like to move towards a more generic grouping and sorting strategy as well. but that's for another bug ;))

the assert won't happen (once we ship, as we won't don't alert / assert), but the context menu will still be messed up.

morphing the title.

thanks for catching this, deb.

Created an attachment (id=285201)
screen shot of the "bad" context menu on windows

note that you will get the same problem if you right click on these menu items on windows.

I believe context menus are disabled for menu items on mac

Created an attachment (id=285203)
wip

getting "too much recursion" from the placesFlavors getter in utils.js

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

I'm seeing this in today's build as I was trying to right-click to open the tag contents in all tabs (which you can do for other folder appearing things in bookmarks).

If the assert isn't valuable, should it be removed?

Right clicking now causes the whole menu under "Places" in the toolbar to dismiss. Is this expected?

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9a9pre) Gecko/2007110204 Minefield/3.0a9pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9a9pre) Gecko/2007110405 Minefield/3.0a9pre

In the bookmarks sidebar, middle-clicking a bookmarks folder doesn't open the bookmarks in tabs (it doesn't open anything).

In Firefox 2.0.0.*, it opens them.

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

This works for me, at least as of the latest nightly build. So I can confirm something must have changed because it used to hit me badly when I did anything with tag containers.

It was expected that it will start working after landing of bug 385245. This has happened and was now confirmed. In any case, "Recent Tags" are now built completely differently, so if there are still some problems, a new bug should be filled.

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13pre) Gecko/20080226 BonEcho/2.0.0.13pre ID:2008022603

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022806 Minefield/3.0b4pre

(From update of attachment 298333)
Marking obsolete, as it was never checked in.

There are still some bugs with dragging folders, at least on linux. I'm having all kinds of problems that are hard to nail down.

Same with control-click:

Steps:
- Use the bookmark organizer to make a folder under the bookmarks toolbar folder and populate it with a couple of individual bookmarks.
- Control click on a folder in the bookmarks toolbar (under the address bar)

Actual (FF3):
- The folder opens as if an unmodified left click had been made

Expected (and FF2 behaviour):
- Each bookmark in the folder is opened as a tab.

The above is also valid if "control-click" is replaced with "middle-click".

(In reply to comment #73)
> I didn't realize that bug resolutions were allowed to be made based on
> results from hourly builds.

Actually, a bug is usually resolved fixed as soon as the patch is (cvs) checked in.
(It may be reopened later if something goes wrong.)

*****

Comment 74 and comment 75 (and comment 79):
*FF regression: bug 419740.
*SM regression: bug 420341.

*****

V.Fixed, per bug 420341 comment 1.

(From update of attachment 305975)

>Index: browser/components/places/content/sidebarUtils.js
>===================================================================

>- var modifKey = aEvent.shiftKey || aEvent.ctrlKey || aEvent.altKey ||
>- aEvent.metaKey || (aEvent.button != 0);
>- if (!modifKey && tbo.view.isContainer(row.value)) {
>+
>+ var modifKey = aEvent.ctrlKey || Event.metaKey;
>+ var openInTabs = aEvent.button == 1 || (aEvent.button == 0 && modifKey);

this should rather be #ifdef'ed.... Cmd+click on mac and Ctrl+click on windows. Shift is supposed to open in tabs in a new window, I think. Check the toolbar.

(In reply to comment #27)

> Shift is supposed to open in tabs in a new window, I think. Check the toolbar.

the toolbar does open tabs in new window if you Shift-click the "open all in tabs" item, while shift-click on the folder is like a normal click (opens the folder).

the FX2 sidebar opens in new window when you shift-click on the folder, so i'm adding this check to get back the same behaviour as FX2

I think shift+middle-click for open-tabs-in-new-tabs makes sense in the toolbar and menu context as well.

Mano, this is the actual behaviour with an updated patch:

1. left-click: toggle container open (CORRECT)
2. ctrl+left-click: open container contents in tabs (append) (CORRECT)
3. shift+left-click: open container contents in tabs in new window (CORRECT)
4. ctrl+shift+left-click: open container contents in tabs (replace) (CORRECT?)

5. middle-click: open container contents in tabs (append) (CORRECT)
6. ctrl+middle-click: open container contents in tabs (append) (CORRECT)
7. shift+middle-click: open container contents in tabs (replace) (CORRECT?)
8. ctrl+shift+middle-click: open container contents in tabs (replace) (CORRECT?)

"CORRECT?" items are mostly due to the fact we are simply passing the work to openContainerNodeInTabs that ends up calling whereToOpenLink(aEvent, false, true); If we want to change that behaviour we must patch _openTabSet too to force the opening in a new window.

Clicking both keys makes unclear the user will, in FX2 that does not open anything, should be the same here?

(In reply to comment #29)
> I think shift+middle-click for open-tabs-in-new-tabs makes sense in the toolbar and menu context as well.

this will most probably have the same behaviour as previous points, with shift+middle-click, whereToOpenLink will _replace_ contents with new tabs.

Created an attachment (id=307676)
patch

implements comment #30

i want to add gutter selection in onClick to this patch, should solve Bug 421210

(From update of attachment 307676)
Mike, could you define what is the expected behaviour about comment #30?

I can still see this bug with Mozilla Firefox Beta 3 (German) on Windows XP SP2.

In order to reproduce it I created a new profile and it still happens. Steps to reproduce:

1. Create new profile and start Firefox Beta 3.

2. I bookmarked the Beta start page http://www.mozilla.com/en-US/firefox/3.0b3/firstrun/ with the tag "mozilla".

3. I left-clicked on the folder "Intelligent Bookmarks" > "Recently used tags" and than right-clicked on the tag "mozilla".

4. Three error massasages/popups came:

ASSERT: Node QI Failed
Stack Trace:
0:QI_node([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)],nsINavHistoryQueryResultNode)
1:asQuery([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
2:([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
3:PC__hasRemovableSelection(true)
4:PC_isCommandEnabled(placesCmd_moveBookmarks)
5:isCommandEnabled(placesCmd_moveBookmarks)
6:updatePlacesCommand(placesCmd_moveBookmarks)
7:goUpdatePlacesCommands()
8:oncommandupdate([object Event])
9:focus()
10:buildContextMenu([object XULElement])
11:onpopupshowing([object MouseEvent])

ASSERT: Node QI Failed
Stack Trace:
0:QI_node([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)],nsINavHistoryQueryResultNode)
1:asQuery([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
2:([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
3:PC__hasRemovableSelection(false)
4:PC_isCommandEnabled(cmd_cut)
5:isCommandEnabled(cmd_cut)
6:goUpdateCommand(cmd_cut)
7:goUpdateGlobalEditMenuItems()
8:updateEditUIVisibility([object MouseEvent])

ASSERT: Node QI Failed
Stack Trace:
0:QI_node([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)],nsINavHistoryQueryResultNode)
1:asQuery([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
2:([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)])
3:PC__hasRemovableSelection(false)
4:PC_isCommandEnabled(cmd_delete)
5:isCommandEnabled(cmd_delete)
6:goUpdateCommand(cmd_delete)
7:goUpdateGlobalEditMenuItems()
8:updateEditUIVisibility([object MouseEvent])

(In reply to comment #41)
> I can still see this bug with Mozilla Firefox Beta 3 (German) on Windows XP

It will be fixed with Firefox beta4.

Under linux, the problem is that the drag object is not transparent, so it's not possible to see where to drop it (3.0b4) (especially when dragging more than one item).

(From update of attachment 307676)
I believe that the behaviour outlined in comment 30 is correct, as I understand it. I think we've got pretty solid logic in whereToOpenLink at this point, so we should really just trust it to do the right thing...

Created an attachment (id=308873)
patch

this fixes:

- containers in sidebar open on middle-click or left-click + modifiers
- selection in sidebar can be done in gutter (space before the favicon)
- you can middle-click the "Open all in tabs" option in menupopups
- open all in tabs context menu option disabled state is calculated faster
- open all in tabs context menu option works correctly for folder shortcuts
- onclick handler in BookmarksEventHandler code cleanup (rem. useless code)
- middle-click or left-click with modifiers works on folders in toolbar and menus

after ev. review + checkin will look around to close related bugs

Created an attachment (id=309413)
unbitrot for PlacesUIUtils

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

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

(From update of attachment 309413)
>+ var modifKey = aEvent.metaKey || aEvent.shiftKey);

Syntax error, in both places. I'll fix this on checkin.

r=mano

I'm simplifying the check to:
#ifdef XP_MACOSX
    var modifKey = aEvent.metaKey || aEvent.shiftKey;
#else
    var modifKey = aEvent.ctrlKey || aEvent.shiftKey;
#endif
    if (aEvent.button == 2 || (aEvent.button == 0 && !modifKey))
      return;

Created an attachment (id=309495)
for checkin

mozilla/browser/base/content/browser-places.js 1.120
mozilla/browser/components/places/content/bookmarksPanel.xul 1.12
mozilla/browser/components/places/content/controller.js 1.223
mozilla/browser/components/places/content/history-panel.xul 1.16

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

after this fix when i open folder with many tab with middle-click the Confirm open dialog is showing before the menus is closing.

the dialog is behind the open menus

onemen.one: Please file a bug on that and cc me and Marco.

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

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre

https://litmus.mozilla.org/show_test.cgi?id=5275 added to Litmus to cover this scenario.

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

Test cases were added for 3.x test runs on litmus for regression testing.

For 3.0,
https://litmus.mozilla.org/show_test.cgi?id=7536

For 3.1,
https://litmus.mozilla.org/show_test.cgi?id=7537

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