Closed Bug 405198 Opened 17 years ago Closed 16 years ago

Cannot drag and drop folders / items in Places Organizer

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta4

People

(Reporter: ronin.achilles, Assigned: asaf)

References

Details

(Keywords: dogfood)

Attachments

(1 file, 4 obsolete files)

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.
Flags: blocking-firefox3?
Blocks: 393514
Can see it under Linux too.
Assignee: nobody → mano
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Dupe of bug 405625 or vice versa?
how can a unconfirmed bug become a firefox3 blocker? :)

Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Version: unspecified → Trunk
Blocks: 405625
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.
Target Milestone: --- → Firefox 3 M11
I can confirm this bug, and imo its a showstopper. Not being able to drag and drop makes places redundant.
Keywords: relnote
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.
No longer blocks: 407449
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.
See comment 12, I don't know the thread manager code at all, sorry.
Assignee: mano → nobody
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
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!
(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
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.
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)
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.
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.
Attached patch make D&D work inside flatList (obsolete) — Splinter Review
this works for dragging into folders in Library right pane.

It's not enough to fix this bug, but it's a little step...
Attachment #296327 - Flags: review?(mano)
Comment on attachment 296327 [details] [diff] [review]
make D&D work inside flatList

damn, never enough testing even after quad testing...
Attachment #296327 - Attachment is obsolete: true
Attachment #296327 - Flags: review?(mano)
Adding bug 389931 as a blocker (Enn said that's the closest to fixing the regressions from the thread manager change).
Depends on: 389931
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
please add this as relnote item for beta 3 releasenotes.
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.
Attached patch enable D&D into the Library (obsolete) — Splinter Review
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.
Attachment #300873 - Attachment is obsolete: true
Assignee: nobody → mak77
Hardware: PC → All
Whiteboard: [has wip patch]
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. :(
Keywords: dogfood
(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.
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 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.
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.                
(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.
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).
Assignee: mak77 → mano
Priority: P2 → P1
Whiteboard: [has wip patch]
Attached patch wise wise (obsolete) — Splinter Review
Attachment #302110 - Attachment is obsolete: true
Attachment #303809 - Flags: review?(dietrich)
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.
Comment on attachment 303809 [details] [diff] [review]
wise wise

sanity-r=me, but post-facto review from dietrich is greatly desired
Attachment #303809 - Flags: review+
Attached patch as checked inSplinter Review
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
Attachment #303809 - Attachment is obsolete: true
Attachment #303822 - Flags: review?(dietrich)
Attachment #303809 - Flags: review?(dietrich)
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(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.
Depends on: 418120
Comment on attachment 303822 [details] [diff] [review]
as checked in

>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.
Attachment #303822 - Flags: review?(dietrich) → review+
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
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.
Flags: in-litmus? → in-litmus+
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.
No longer blocks: 405625
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).
Depends on: 505919
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.

Attachment

General

Created:
Updated:
Size: