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.

2007051804 works
2007051821 broke

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-18+04%3A00%3A00&maxdate=2007-05-18+21%3A00%3A00&cvsroot=%2Fcvsroot

Probably broke from:
"Turing on Bookmarks on Places."

See also bug 338401 comment 17, about "Bookmark Manager".

Cancelling "suryakrao: blocking1.8.0.14 = ?" as this bug is 1.9 trunk only.
suryakrao, explain if you disagree.

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

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112401 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112401 Minefield/3.0b2pre

In the Places Organizer, I am not able to drag and drop folders or bookmarks from one location to another.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Can see it under Linux too.

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

Dupe of bug 405625 or vice versa?

how can a unconfirmed bug become a firefox3 blocker? :)

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

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

Could it be nominated for beta2 ? As it could be seen as a regression from beta1 which have a working drag and drop in places organizer.

Just asking, sorry for the spam.

I can confirm this bug, and imo its a showstopper. Not being able to drag and drop makes places redundant.

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

This is a bad user experience and a serious regression. If we have a simpole fix for this, can we please get this into M10?

Cut and paste still works, so I'm not sure that I'd consider it a serious regression for beta 2; it's not ideal, obviously, but I'm not sure that I'd hold beta 2 for the issue.

Mano: is this a trivial fix?

> is this a trivial fix?

investigating, will report back ASAP.

part of the problem is here:

        canDrop: function VO_canDrop(index, orientation) {
          var result = this._self.getResult();
          var resultview = this._self.getResultView();
          var node = index != -1 ? resultview.nodeForTreeIndex(index) : result.root;

          if (orientation == NHRVO.DROP_ON) {
            // The user cannot drop an item into itself or a read-only container
            var droppingOnSelf =
                this._getSourceView() == this._self &&
                this._self.view.selection.isSelected(index);
            if (droppingOnSelf || PlacesUtils.nodeIsReadOnly(node)
              return false;
          }

I think that we need to make this:

return droppingOnSelf || PlacesUtils.nodeIsReadOnly(node);

If we are dropping on, and not on ourselves and the node is not readonly, we should allow it.

This allows us to dnd from the right hand pane into "drop able" targets in the left. (Note, tags should not allow us to drag onto it, that will be a follow up bug.)

I still need to get dnd to work within the right hand pane.

So, I think that fixing this depends on https://bugzilla.mozilla.org/show_bug.cgi?id=337761#c16 being fixed first.

Same thing happens when middle-clicking on a tag, to open all windows associated with this tag.

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

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

See also bug 407499 which I marked as dupe. The same assertion occurs when choosing "Open All in Tabs" from the Bookmarks Toolbar.

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

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

Bug 407893 is almost a duplicate. I had made an additional observation under that bug which is as follows:
"Similarly, if I expand a folder in the bookmark toolbar and drag an item (from
within that folder) to a different position in the same folder, the item is
dropped 4 items below where the mouse pointer is."

I investigated this a little bit. In Mozilla Firefox 3 Beta 1\chrome\browser.jar\content\browser\places\controller.js in the function _getDropPoint: function TBV_DO_getDropPoint(event):
I find that on dropping a dragged item at a particular entry, I get the following values:
event.target.boxObject.y = 209 (= a)
event.target.boxObject.height = 20 (= b)
event.clientY = 250 (= c)
I was expecting that a <= c <= a + b, which is not the case.
also, i found that event.target refers to the correct item. I did so by looking at event.target.label. The dropped item is getting added a couple of items below the mouse pointer because event.clientY is not on event.target at all. Thus comparing event.clientY with nodeY in the function is incorrect. event.target and its properties should be used to identify the target.
But, if we want to find whether mouse is in upper half or lower half, event.clientY is needed which, IMHO, appears to return incorrect value.

Mrinal, this issue seems to be bug 405087.

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

Created an attachment (id=293492)
serve middle-click on bookmark folders

this is for firefox 2 parity

serve middle-click on bookmarks folders +
trailing spaces fix +
a fix to openContainerNodeInTabs since if there are no urls to open, it opens an (untitled) tab

Mano: should i fix the last problem in _openTabset with an "if (aURLS.length <= 0) return;" instead of in openContainerNodeInTabs (that uses _openTabset)?

Reed: please, don't check-in until i put checkin-needed in keywords, i need to address review comments, thank you :)

(From update of attachment 293492)
Why just folders? In Places, Open-In-Tabs is supported for all urls-containers (except day containers, known bug)

i was looking at FF2 parity, FF2 does not open history containers

also, open in tabs does not get all urls recursively, it opens only the first sublevel, so in history sidebar it would serve only hosts folders and date folders, but not grouped by date & hosts.

changing to open all containers has problems with day containers (as you said, it tries to open a bunch of not visible visits), but also on host containers, and also gives me an assertion in CountVisibleRowsForItems...

Created an attachment (id=293513)
open urls for all containers

this is for all containers.

notes:
on middle-click the node container opens, than urls are loaded, this is because i need to get only childs with child.viewIndex >= 0 (or open in tabs will try to open all hidden duplicates, so for a folder with 1 visible item i end up with a request to open 64 tabs!)

still don't know if the check on urlsToOpen.length should be done in _openTabset

after opening, itemInserted for dynamic containers is calling _countVisibleRowsForItem for a non visible item, and that is causing an assertion, so i'm excluding dynamic containers from _countVisibleRowsForItem

Someone marked my bug as a duplicate of this one, so I thought I would add that it is impossible for a bookmark to be dragged into a sub-subdirectory, when this was possible in 2.0.11 (I'm running 3b1).

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

Darnit! I hate logging duplicate bugs. How come a search for the string "Node QI Failed" doesn't return this bug?

Does not work on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904
Firefox/3.0b1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120
Firefox/3.0b2

Probably because that string isn't in the summary.

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

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

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

See comment 12, I don't know the thread manager code at all, sorry.

This problem also occurs on the Macintosh version:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2

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

when right clicking - "Open All In Tabs" from history sidebar (when view by site) we open multiple copies of each item

the duplicate copies appear to be from visits.

see attached screen shot.

the problem might be in getURLsForContainerNode().

perhaps we're collapsing duplicates in the UI (tree view), but not using the same logic when determining urls in the container.

Created an attachment (id=294688)
screen shot

could be a regression from bug #399729: "Reduce places viewa performance overhead", but I have not debugged or confirmed any of this (or my theory in comment #1).

found this while testing a fix for bug #409301, but firefox 3 b 2 has the same bug.

this is related to Bug 293513, there is a similar problem there, it's because PlacesUtils.getURLsForContainerNode gets also non visible urls

sorry, i was talking about Bug 402558

Created an attachment (id=294705)
patch

I think we should consider re-disabling dynamic containers for 1.9, the implementation is not all that usable at this point.

does that work when a folder is closed and you right click it and call open all in tabs?

i'm asking because in Bug 402558 i had to open the folder before calling openSelectionInTabs

This is a ridiculous failure of UI functionality and should not be tolerated for a release, much less a release candidate or even a beta.

Using either cut&paste or the Move... dialog (in the Organize menu inside the Places Organizer window) is _not_ an acceptable workaround!

I don't apologize for the tone. It's the kind of usability flaw that drives the most basic, average user ... absolutely nuts. They are _not_ going to intuit the cut&paste or Organize->Move methods!

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2007122815 Minefield/3.0b3pre

I can say this problem still exists.

(In reply to comment #18)
> This is a ridiculous failure of UI functionality and should not be tolerated
> for a release much less a release candidate

Agreed. The bug is already marked blocking-firefox3+, though, so you're preaching to the choir! It's generally not useful to advocate in Bugzilla comments unless you have new information or insight to share, or are interested in contributing a fix.

> or even a beta.

I disagree, and obviously the release team doesn't agree. I respect their judgement here. You might want to consider that this isn't the _only_ bug left to fix before Firefox 3, and that while it may be personally quite frustrating for you, shipping a beta or two with this bug certainly isn't the end of the world given the workarounds. You are using pre-release software, after all - perhaps you should adjust your expectations.

Is this related? Happened while dragging a tab to the Bookmarks Menu.

    ASSERT: Insertion point for an menupopup view during a drag must be -1!
    Stack Trace:
    0:BMDH_onDrop([object MouseEvent],[object Object],[xpconnect wrapped (nsISupports, nsIDragService, nsIDragSession)])
    1:([object MouseEvent],[object Object])
    2:ondragdrop([object MouseEvent])
    3:invokeDragSessionWithImage([object XULElement],[xpconnect wrapped nsISupportsArray],null,7,null,0,0,[object MouseEvent])
    4:([object MouseEvent],[object XULElement])
    5:ondraggesture([object MouseEvent])

Steps to reproduce:
1. Drag a tab with valid URL to the Bookmarks Menu
2. --

Expected Behaviour:
1. Automagically open Bookmarks menu upon (to give more options where to place the bookmark)

Actual result:
1. Bookmarks menu won't open, the choice of Bookmarks folders is not given as it was in FF 1.5 or FF 2

(In reply to comment #7)
> does that work when a folder is closed and you right click it and call open all
> in tabs?
>
> i'm asking because in Bug 402558 i had to open the folder before calling
> openSelectionInTabs
>

OAIT menuitem is disabled when a container is closed.

open only visible items will be fixed in Bug 409998

Created an attachment (id=295230)
open container urls on middle-click

- removed fix for non visible items (Bug 409998)
- moved empty url array check in _openTabset
- middle click open urls ONLY if container is open (since context menu Open All in Tabs is disabled for closed containers)

note: i don't get asserts anymore on dynamic containers, don't know what other patch could have fixed that though...

I can confirm this bug for: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5.1; en-US; rv:1.9b3pre) Gecko/2008010404 Minefield/3.0b3pre

(In reply to comment #11)
> part of the problem is here:
>
>
> canDrop: function VO_canDrop(index, orientation) {
> var result = this._self.getResult();
> var resultview = this._self.getResultView();
> var node = index != -1 ? resultview.nodeForTreeIndex(index) :
> result.root;
>
> if (orientation == NHRVO.DROP_ON) {
> // The user cannot drop an item into itself or a read-only
> container
> var droppingOnSelf =
> this._getSourceView() == this._self &&
> this._self.view.selection.isSelected(index);
> if (droppingOnSelf || PlacesUtils.nodeIsReadOnly(node)
> return false;
> }
>
> I think that we need to make this:
>
> return droppingOnSelf || PlacesUtils.nodeIsReadOnly(node);
>
> If we are dropping on, and not on ourselves and the node is not readonly, we
> should allow it.
>
> This allows us to dnd from the right hand pane into "drop able" targets in the
> left. (Note, tags should not allow us to drag onto it, that will be a follow
> up bug.)
>
> I still need to get dnd to work within the right hand pane.
>

Hi, I'm just now getting into the bugzilla community, so I apologize if my questions seem "newbish" - I assure you I'd love to figure fix this bug, and I do have plenty of time for research. I have two questions thus far:

What right-hand pane? (again, sorry for newbness)
Is this truly related to https://bugzilla.mozilla.org/show_bug.cgi?id=337761#c16 ?

(In reply to comment #11)
> I think that we need to make this:
>
> return droppingOnSelf || PlacesUtils.nodeIsReadOnly(node);

No. This code is fine. The erroneous code is here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/places/content/tree.xml&rev=1.88&mark=772&&#772

PlacesControllerDragHelper.canDrop gets a view with id=51. As SQLite Manager tells me, it's the real root (parent of history, tags, all bookmarks). That's why the function always return false because this view is readonly. We have to pass the correct view to PlacesControllerDragHelper.canDrop.

mozilla/browser/components/places/content/utils.js 1.94

Since when "Open All
in Tabs is disabled for closed containers", that seems like a bug/regression.

see https://bugzilla.mozilla.org/show_bug.cgi?id=409998#c8

No reasoning there either.

the problem is that to open only visible items we rely on viewIndex, so the container must be opened before calling open in tabs...

a better solution could be change getURLsForContainerNode to return an array of unique urls, and remove check on viewIndex

This should be done only the query/site container in question is closed, when it's opened, any visible url nodes should be "mapped" to tabs.

(From update of attachment 295230)
i need to re-check a couple of things, clearing requests

the problem in the right pane of the organizer is due to this code in treeView.js

  isContainer: function PTV_isContainer(aRow) {
    this._ensureValidRow(aRow);
    if (this._flatList)
      return false; // but see getCellProperties

for a flatList containers are not marked as containers (but a container atom is manually added to them), so when D&D code calls isContainer it gets false and does not drop in

in getCellProperties:

    // To disable the tree gestures for containers (e.g. double-click to open)
    // we don't mark container nodes as such in flat list mode. The container
    // appearance is desired though, so we add the container atom manually.
    if (this._flatList && PlacesUtils.nodeIsContainer(node))
      aProperties.AppendElement(this._getAtomFor("container"));

getInsertionPoint and other D&D functions use view.isContainer() and view.isContainerOpen() that are returning false

Mano: why they should not be reported as containers there? i've tried to comment out the if code and everything is working fine (double click too)

i was tryiong to create a new util to see if a folder has url childs (without having to use getURLsForContainerNode(node).length == 0 that is slow), but for the right pane when i call node.containerOpen = true to use childCount i get a "cannot change properties for a Native Wrapped object".

I think that is because folders in the right pane are not containers (see https://bugzilla.mozilla.org/show_bug.cgi?id=405198#c24)

Because treebodyframe may try to open them, basically. d&d code can call PlacesUtils.nodeIsContainer for the node at that row.

Still, I would bet you cannot implement anything useful here as long as the tread manager bug isn't fixed.

Created an attachment (id=296319)
wip

mano: this is working for everything but not for dynamic containers in the left pane, mainly because
this.getFolderContents(aNode.itemId, false, false).root;
returns an empty container for left pane bookmark menu, toolbar and unfiled, because they are place:folder=x... so having no children the option open all in tabs is disabled.
how can i get correct content? changing expandQueries option does not work (the interface says that for simple folder queries it has no effect since they should be expanded by default)

is not the thread manager bug killing only D&D between different views/menus? i think that D&D in folder bug in the same view (Library right pane) is not affected by that, it's a flatlist bug (so, disabling flatlist makes D&D working like a charm in right pane, also with thread manager bug).
what i'm testing is draggin a bookmark menu item in a bookmark menu folder only in the right pane.

Created an attachment (id=296327)
make D&D work inside flatList

this works for dragging into folders in Library right pane.

It's not enough to fix this bug, but it's a little step...

(From update of attachment 296327)
damn, never enough testing even after quad testing...

(From update of attachment 296319)
partly fixed in Bug 411803

Created an attachment (id=296524)
fixes modifiers too

- add a new util to check if a container has child urls (perf)
- fix right click context to use that
- fix sidebar to open in tabs on middle-click, CTRL+click, META+click

still does not count correctly for Library left pane folders (need some hint, could be spun-off to a new bug)

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

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

Created an attachment (id=296956)
testcase

The problem is that a popup doesn't open on a dragenter (or other drag event) event.

Now, the places code also uses timers in combination with drag events, so it also suffers from bug 203573, atm. But because of the first mentioned issue, you can't work around this, by not using a timer.

Related bugs for the first mentioned issue: bug 389615, bug 343729.

It seems to me something like in bug 395397 should be done for windows.

Jim, maybe you know how to fix this?
It seems to me that (at least) windows widget code needs to be fixed to allow things like timers and layout still to keep working while a drag is performed inside Mozilla.

Adding bug 389931 as a blocker (Enn said that's the closest to fixing the regressions from the thread manager change).

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

Created an attachment (id=298333)
drag and drop event patch v.1

Ok, here’s a patch that addresses this. The main thread is caught up in nsDragService’s DoDragDrop, and doesn’t return until the drop operation finishes. During that call thread events don’t get processed up in nsAppShell. Windows calls back to Fx3 into nsNativeDragTarget, where we process drag events and dispatch them as UI events into the current window as mouse messages. These are then processed by the view, which ultimately calls down into the places code observer where the timer is set. That timer event though is based on an nsRunnable event, so it doesn’t get processed until after DoDragDrop returns. Hence the Bookmark stuff failing but things like tab arrows are successfully drawn within the layout code.

What I did was simply add a call to nsThreadUtil’s NS_ProcessPendingEvents in nsNativeDragTarget so when the timer event fires, it gets processed during the drag operation. That seems to have fixed things and doesn’t appear to be hurting drag operation performance.

Just noticed, was this a problem on all platforms or just windows?

If the user stops moving the mouse, events won't be processed, right?

I'm amazed that Windows doesn't run our window event loops during drag. Does it not process messages in *any* window?

Sorry not the native event handler, the event processing that takes place in nsBaseAppShell::Run, which calls NS_ProcessNextEvent(thread).

(From update of attachment 298333)
What is the point of adding a space in <nsDragService.cpp> ?

>If the user stops moving the mouse, events won't be processed, right?

Yes, actually it does. Javascript timers in general freeze during drag and drop operations currently. For example you can stop any setTimeout from firing by dragging something out of the browser window and holding it on the dekstop.

I wonder if Windows delivers window messages anywhere where we can grab them and run our event loop normally.

(In reply to comment #27)
> I think this is a duplicate of Bug 299185.

While the symptom seems to be similar/same,
that bug predates the regression(s) caused by bug 326273 (on 20060510),
hence it's not a duplicate of this bug.

(In reply to comment #47)
> Just noticed, was this a problem on all platforms or just windows?

On comment 12, Asaf changed this bug from Windows to All;
yet, in my memory, this regression was Windows specific...

>I wonder if Windows delivers window messages anywhere where we can grab them
>and run our event loop normally.

Ok, I'll do some more poking around and see what I can come up with.

Created an attachment (id=298533)
test settimeout html

A useful testcase with a little bit of animation on a div.

(From update of attachment 298333)
Excellent! I believe this will fix quite a few issues. r=me, just don't add that extraneous space into nsDragService.cpp.

--Ere

> I wonder if Windows delivers window messages anywhere where we can grab them
> and run our event loop normally.

It does, we have for example a hidden event window that can receive events during the operation. All other win32 window event procedures also continue to receive events. One experiment I tried was to set a timer in the event window during drag operations to keep event processing occurring which worked. Setting and clearing this from nsDragService was a pain and js timers were still inaccurate though due to the resolution of the timer.

Overall this patch still seems like the simplest solution. The only drawback is that event processing will stop if the mouse leaves all moz created windows and remains stationary. Events will get processed if the mouse sits over one of our windows and is stationary as windows continues to send event callbacks in this case.

Any other suggestions on alternative approaches are welcome. :) I'd be happy to test them out. My experience with our event system / windowing is pretty new so I'd welcome any feedback / ideas.

> just don't add that extraneous space into nsDragService.cpp.

Sorry, should have caught that. Will clean it up and repost.

Hang on. If the hidden event window ("nsAppShell:EventWindow") is still able to receive events, why don't Gecko events work? When we post a Gecko event we should be calling nsAppShell::ScheduleNativeEventCallback, which post a message to that window which will give us a chance to process the Gecko events.

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

There was some code to make sure this was happening, but it created a big perf regression. In my testing I was able to see this, this call was made repeatedly during startup. After startup things seemed to calm down. Without this or some other solution SetNativeCallback is never called during the drag operation.

http://lxr.mozilla.org/mozilla/source/widget/src/xpwidgets/nsBaseAppShell.cpp#89

I think we need to try to make that work without a performance regression instead of hacking in a call during drags. I.e., the real fix to this bug is to fix 338225. If that is fixed, we don't need to do anything here and any fix specific to the drag code will become cruft.

please add this as relnote item for beta 3 releasenotes.

Created an attachment (id=300873)
enable left pane as a drop target

this is for testing only, as already pointed in comment #23 candrop is checking if root is a readonly folder, that should not happen since even if root is readonly, its children could be writable!
So this removes that check, then we should ensure that getdroppoint makes a good job in don't allowing putting items into wrong places (it should really be updated to take in count excludeitems).
Dropping into items should work now that we have the new folderItemId used in insertionPoint.

NOTICE that applying this patch will expose a full number of new bugs in drag & drop (like dropping in the empty area below tree, dragging tags, dragging queries, dragging roots, dragging in all Bookmarks and so on), but without this finding them will be even more difficult.

this is for drag inside left pane, and to/from left pane, does not solve any bug in the right pane flatlist.

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

A rightclick on the bookmark bar while the tag is selected also triggers the assert.

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre ID:2008020304

-> Verified

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

Created an attachment (id=302110)
enable D&D into the Library

this is still a test patch, to investigate on.

Mano: since the main problem in the right pane is isContainer during a drag operation, we could make it detect if we have a valid drag session, and in that case return the correct item type. This way treebodyframe will usually see that as a non-container, while during a drag its behaviour is correct.

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

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

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

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

Doh. I accidentally started a drag action on an item inside a folder on the Bookmarks Toolbar. The bookmark gets moved to the toolbar itself, and it's now impossible to move it elsewhere. :(

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

(In reply to comment #37)
> Doh. I accidentally started a drag action on an item inside a folder on the
> Bookmarks Toolbar. The bookmark gets moved to the toolbar itself, and it's now
> impossible to move it elsewhere. :(

Justin, I filed that in bug 416655 some days ago.

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

This doesn't appear to have been fixed correctly. I can confirm for Firefox 3 Beta 3 that when you drag & drop a bookmark in the same folder it ends up in the main bookmark menu :/.

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

Target Milestone need to be updated? This clearly missed beta 3.

This actual folder opening is fixed by the patch in bug 389931, which fixes the underlying issue on all platforms.

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

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

Bug 389931 now has a patch attached. With that patch, what is the status of this bug? Does that fix the issue or do any of the patches to this bug fix this issue if installed in conjunction with that patch?

after a patch for thread manager bug we'll need a patch here to make things work as expected. Tomorrow i'll test Mats's patch

Great! If you can come up with a patch that needs testing I can make Linux and Windows builds which include such a patch available on my server so that people who can't do their own builds can test the fix.

I have made Linux and Windows builds, that include both the latest patch from here and the patch for bug 389931, available for testing on my server at:

http://www.wg9s.com/mozilla/firefox/

thanks bill i will test the windows build for you when i get home. I will post results here. Eager to see this bug fixed.

Downloaded and tested this build and I am seeing very high CPU usage, 60-80% all the time, even doing nothing.
Same in New Profile and Safe-mode

Good news is that the d&d issues seem mostly fixed. I only noticed one strange thing during quick testing.
1. From the Menu bar click Bookmarks
2. Drag a folder across a separator
3. The Menu unexpectedly closes leaving you holding the folder and no place to drop it. Lands in a random spot, or does not move. No errors in the Console2

Dragging a folder across a separator in the Library does not have any issues.

I wish there was an edit:
System specs here:
1.4 ghz Athlon Thunderbird
1.0 gig Ram
Vista HP - fully up to date

Tried the custom build out, breaks D&D even more for me and uses large amounts of CPU up.

When I drag and drop, it doesn't move the bookmark anywhere in the bookmark menu now. Instead it just takes me to the page I'm trying to drag and drop.

Firefox continuously uses 40 - 50% of my CPU.

2.0 GHz Athlon 64 X2
2 GiB Ram
Windows XP x64

P.S tried this build normally and in safe mode.

I built firefox with only this patch and no problem with my archlinux 64 bits. Looks like Windows XP x64 is far more busted than XP 32 bits ;)

Does this sepcial build include the latest patch? The CPU usage should be fixed then.

It's true that in this build many d&d bugs occur. But I think it's more important that some core issues that exist for over a year now are finally fixed. The newer bugs should be just followups as d&n is more broken than fixed anyway.

I'm sure this is new, as I can remember doing this before and not getting this error. But I might be mistaken....

On Firefox 3 Beta 3 I get an error when I go to and click on:

Smart Bookmarks > Recent Tags > (random tag) > Open All in Tabs

Error comes up:

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:PU_openContainerInTabs([xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode)],[object XULCommandEvent])
4:oncommand([object XULCommandEvent])

It's different to the error when I go to and right click on:

Smart Bookmarks > Recent Tags > (random tag)

Which produces:

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

And again slightly different to when I go to and right click on:

Smart Bookmarks > Recent Tags > (random tag) > (random item)

Which produces:

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])

Works in :

Vista 64 SP1
E6600
4GB Ram

Problems

High cpu usage, stuck around 40-50%

Drag and Drop:

Works fine, fix the cpu problem and it will at least work in Vista. The people who are having issues with it working are probably suffering lag issues due to high cpu usage on slower processors.

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

(In reply to comment #50)
> Problems
>
> High cpu usage, stuck around 40-50%

If the high CPU usage was using my test build,g 389931 was not the latest version. The earlier patch is known to cause high CPU usage issues. I have new builds running now. Windows should be available around 5:30 Eastern, Linux about an hour later.

I'll make sure to test the build and do a clean install with it given it seemed to break for me more than anyone else. 17:30 eastern the same as 23:30 GMT?

22:30 I think. In 1/2 hour from now.

Dietrich, how many percent of all work to do is your WIP? Or is it bitrotted meanwhile?

When using a test build that has possible fixes for numerous Drag & Drop issues, I have found that the asserts no longer occur:

http://www.wg9s.com/mozilla/firefox/

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021613 Minefield/3.0b4pre Firefox/3.0 ID:2008021613

CPU usage is normal with the latest build, and things look pretty good.
I have also noted that this build seems to fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=400109

So not sure which patch fixed it...

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021613 Minefield/3.0b4pre Firefox/3.0 ID:2008021613

Did Marco asked you all to test it with the last patch here? For what it's worth, it has no dependancies on the other bug and it's actually a very partial workaround to that issue as far as i know.

Marco in comment 32 stated it was for testing, and Bill provided the build with the patches. Just following their lead and doing 'Testing'.

Sorry if any results reports were premature.

See comment 41.

A better solution is on the pipe, which also implement spring-load-folders (and doesn't depend on the platform bug).

Created an attachment (id=303809)
wise wise

The behavior here is very similar to mac's finder. If you just drop on a container, the item goes into it; if you rather wait a sec, the container becomes the tree contents.

(From update of attachment 303809)
sanity-r=me, but post-facto review from dietrich is greatly desired

Created an attachment (id=303822)
as checked in

mozilla/browser/base/content/browser-places.js 1.95
mozilla/browser/components/places/content/controller.js 1.203
mozilla/browser/components/places/content/places.js 1.128
mozilla/browser/components/places/content/places.xul 1.106
mozilla/browser/components/places/content/toolbar.xml 1.121
mozilla/browser/components/places/content/tree.xml 1.94
mozilla/browser/components/places/content/treeView.js 1.38

(In reply to comment #60)
> The behavior here is very similar to mac's finder. If you just drop on a
> container, the item goes into it; if you rather wait a sec, the container
> becomes the tree contents.
>

I can't seem to get this to work.
1. Drag a bookmark from the right pane to a folder on the left
2. Folder never expands no matter how long I hold it

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021701 Minefield/3.0b4pre Firefox/3.0 ID:2008021701

Testing the hourly build 20080217_0130_firefox-3.0b4pre.en-US.win32 which the CVS checkin seems to indicate that this is in.

Still sort of broken, I can drag and drop in the main bookmark menu fine (could do that before hand). Drag and drop is broken from inside folder of the bookmark menu, but less so, I don't get it doing weird things like I do on beta 3. When I drag and drop on this build inside a folder on the bookmark menu, nothing happens, but it seemed when I restarted Firefox the drag and drop took affect.

Somewhat similar to the way when you choose "Sort By Name" from bug 417471

(In reply to comment #63)
> (In reply to comment #60)
> > The behavior here is very similar to mac's finder. If you just drop on a
> > container, the item goes into it; if you rather wait a sec, the container
> > becomes the tree contents.
> >
>
> I can't seem to get this to work.
> 1. Drag a bookmark from the right pane to a folder on the left
> 2. Folder never expands no matter how long I hold it
>
> Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021701
> Minefield/3.0b4pre Firefox/3.0 ID:2008021701
>

No, that's just for the dragging within the right pane. You can drop on folders in the left pane too now, they don't spring load though.

(In reply to comment #64)

This bug is about the organizer window.

The Places Organizer is now empty for me, and I get the following in the Error Console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsINavHistoryService.executeQueries]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://browser/content/places/tree.xml :: load :: line 97" data: no]

Was it working for you before this was checked in? the queries were not changed here.

Mmh works fine for me with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008021704 Minefield/3.0b4pre ID:2008021704 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008021704 Minefield/3.0b4pre ID:2008021704.

I cannot see any of the issues described above. Jonathan please create a fresh profile and run the tests again. Perhaps something is broken with your existing profile?

Should I make the bug I explained in comment #64 a separate bug or is it covered by the remit of this bug or some other bug?

I notice the bug explained in comment #63 has it's own bug, bug 418088

I can do bugzilla queries as much as you can, this bug covers the regression described in comment 0.

Thanks for pointing me to bug 418088.

(In reply to comment #65)
> (In reply to comment #63)
> > (In reply to comment #60)
> > > The behavior here is very similar to mac's finder. If you just drop on a
> > > container, the item goes into it; if you rather wait a sec, the container
> > > becomes the tree contents.
> > >
> >
> > I can't seem to get this to work.
> > 1. Drag a bookmark from the right pane to a folder on the left
> > 2. Folder never expands no matter how long I hold it
> >
> > Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008021701
> > Minefield/3.0b4pre Firefox/3.0 ID:2008021701
> >
>
> No, that's just for the dragging within the right pane. You can drop on folders
> in the left pane too now, they don't spring load though.
>

Just tested, trying to drag & drop a bookmark into a folder in the right-pane, and the folder still not open (spring-load). File a new bug ?

I reverted my tree to just before the checkin using MOZ_CO_DATE and rebuilt. That fixed it, but since going back to TIP, I can't reproduce. Weird. :-/

(In reply to comment #72)

Yeah, probably, with clear STR pleast.

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

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

(From update of attachment 303822)
>Index: browser/base/content/browser-places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser-places.js,v
>retrieving revision 1.94
>diff -u -p -8 -r1.94 browser-places.js
>--- browser/base/content/browser-places.js 13 Feb 2008 15:43:17 -0000 1.94
>+++ browser/base/content/browser-places.js 17 Feb 2008 06:13:46 -0000
>@@ -769,20 +769,19 @@ var BookmarksMenuDropHandler = {
> * @param event
> * A dragover event
> * @param session
> * The active DragSession
> * @returns true if the user can drop onto the Bookmarks Menu item, false
> * otherwise.
> */
> canDrop: function BMDH_canDrop(event, session) {

should remove these params and fix the caller.

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.202
>diff -u -p -8 -r1.202 controller.js
>--- browser/components/places/content/controller.js 14 Feb 2008 11:15:25 -0000 1.202
>+++ browser/components/places/content/controller.js 17 Feb 2008 06:13:51 -0000
>@@ -1406,17 +1406,22 @@ PlacesMenuDNDObserver.prototype = {
> this._view._selection = event.target.node;
> this._view._cachedInsertionPoint = undefined;
> if (event.ctrlKey)
> dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
> xferData.data = this._view.controller.getTransferData(dragAction.action);
> },
>
> canDrop: function TBV_DO_canDrop(event, session) {

ditto

>-function PlacesTreeView(aShowRoot, aFlatList) {
>+function PlacesTreeView(aShowRoot, aFlatList, aOnOpenFlatContainer) {
> if (aShowRoot && aFlatList)
> throw("Flat-list mode is not supported when show-root is set");
>
> this._tree = null;
> this._result = null;
> this._collapseDuplicates = true;
> this._showSessions = false;
> this._selection = null;
> this._visibleElements = [];
> this._showRoot = aShowRoot;
> this._flatList = aFlatList;
>+ this._openContainerCallback = aOnOpenFlatContainer;

nit: _openFlatContainerCallback would've been clearer.

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

Verified with:

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022004 Minefield/3.0b4pre ID:2008022004

definitely already in-litmus. However, I plan to flesh out the test cases to be specific individual paths instead of a broad encompassing dnd test.

Firefox hangs now for good when I try to click away the errors.
I tried the 23 Feb build of Jim's link but it doesn't start up :(.

(In reply to comment #27)
> Firefox hangs now for good when I try to click away the errors.
>

confirmed on latest Mac nightly.

I cannot repoduce this with today's nightly on Windows.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022308 Minefield/3.0b4pre Firefox/3.0 ID:2008022308

Created an attachment (id=305261)
Error dialog when right-clicking on Smart Bookmarks, Recent Tags

Screen shot of error dialog

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

this could dependant on Bug 385245 since that's changing how results are returned, and we could check for Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY

Why has this dropped to P2? I don't see how Firefox can realistically ship with out fixing this, as soon as Journalists, Opera fanboys and Microsofts fanboys find out about this Firefox will become practically a laughing stock.

And the bug seems to have got progressively worse, when I used to right click I just got 1 error message, last night on the latest trunk build I got 7 distinct error messages, then when I right clicked again I got about 14 distinct error messages.

Damian, this is still a blocker

It looks like my patch for bug 385245 is fixing this, at least I cannot confirm the behavior described here. Everything I have tried works fine.

(From update of attachment 296524)
this patch needs a refresh

I don't see the problem anymore. This bug is fixed, by bug 389931.

And thanks, of course, to everyone who contributed.

Which build is it working for you? I still see this in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022604 Minefield/3.0b4pre ID:2008022604

The patch that fixed this bug went in after the 2-26 nightly was built. Either download an hourly tinderbox build or wait until tomorrow's nightly.

For future record, note that comment #69 came at nearly 11AM PST and that your buildid says that it was pulled sometime during the 4AM hour PST (that's what the last 04 in the buildid is), so it was pulled 7 hours before the relevant patch went in.

Thanks for the clarification. It will be fixed in tomorrow's nightly, then. I didn't realize that bug resolutions were allowed to be made based on results from hourly builds. I had always assumed that hourly builds could be too chaotic to definitively report a resolution on a bug. I apologize for making that assumption, and I am glad this is finally fixed. Thanks everyone!

Created an attachment (id=305975)
patch

unbitrot, used new functions in utils

I'm still a bit confused. Does this bug only cover the Bookmarks Toolbar? Doing the same for the Bookmarks Menu shows that it doesn't work. The menu doesn't get opened when dragging a bookmark over it. This is reproducable at least under OS X and Windows. Ria, please help. ;) If it only applies to the toolbar folder I can file a new bug if there isn't already one.