509676 - Can't drag and drop a tag container to another standard point

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[vicky.sun]

Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9.1.1) Gecko/20090803 Firefox/3.5.1


Steps to Perform:

   1. Go to Bookmarks | Organize Bookmarks
   2. Add a tag to a bookmark or set of bookmarks within the library.
   3. Go to the newly created tag folder, via the left pane, and drag-and-drop it to the Bookmarks Toolbar folder. View the contents of the newly added instance.
   4. Repeat step 3 for the Bookmarks menu.

	
Expected Results:

After step 3 and 4, a new instance of step 2's tag folder should be added to the standard point. It's contents should include the tagged bookmark(s) and the original tag folder should not be removed from the general Tags folder.

Actual Results:
Nothing happened.
In error console, there is an error as below:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///usr/lib/firefox/components/nsPlacesTransactionsService.js :: placesMoveItemTransactions :: line 570"  data: no]

Comment 1

[Ginn Chen]

also reproducible with mozilla-central HEAD.

Comment 2

[Ginn Chen]

Also broken on Linux.

372       case PlacesUtils.TYPE_X_MOZ_PLACE:
373         if (data.id <= 0) // non-bookmark item
374           return this._getURIItemCopyTransaction(data, container, index);
375   
376         if (copy) {
377           // Copying a child of a live-bookmark by itself should result
378           // as a new normal bookmark item (bug 376731)
379           return this._getBookmarkItemCopyTransaction(data, container, index,
380                                                       ["livemark/bookmarkFeedURI"]);
381         }
382         else
383           return this.ptm.moveItem(data.id, container, index);

data.id is undefined.
copy is false on GNOME, because drop effect is "link" not "copy".

On Mac, data.id is also undefined, but drop effect is "copy".

Comment 3

[Ginn Chen]

This might be a regression of Bug 371823.

I guess, if (data.id <= 0) should be if (typeof data.id == 'undefined' || data.id <= 0)

Comment 4

[Drew Willcoxon :adw]

Probably related: bug 502192, bug 484489

Comment 7

[Marco Bonardo [:mak]]

can i get a better description of steps 3 and 4?
i should drag a tag container from library to the toolbar, and then also drag a tag container to bookmarks menu?

And this is reproduceable only in Linux and SunOS?

Comment 8

[vicky.sun]

(In reply to comment #7)
> i should drag a tag container from library to the toolbar, and then also drag a
> tag container to bookmarks menu?
No, the step3 and step4 are not precedence relationship. the purpose of the test case is to prove the tag folder can be copied to Bookmark toolbar or Bookmark menu
> 
> And this is reproduceable only in Linux and SunOS?
I haven't other OS at hand now. But I know Mac is ok.

Comment 9

[Ginn Chen]

(In reply to comment #7)
> can i get a better description of steps 3 and 4?
> i should drag a tag container from library to the toolbar, and then also drag a
> tag container to bookmarks menu?
> 
> And this is reproduceable only in Linux and SunOS?

Drag it to either toolbar or bookmark menu or bookmark folder would fail on Linux and SunOS.

Comment #2 and comment #3 explained why it fails, and why it doesn't on Mac OS X.

Comment 10

[Marco Bonardo [:mak]]

yeah, data.id is undefined on windows too, so that's not the problem (still a bug though), the copy does not happen due to the "link" dropEffect issue.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Our utils are so complex to follow, they are doing fancy loops and calls to accomplish quite easy tasks. They really need a cleanup, fixing a bug like this takes a lot of time just to follow all serialization calls.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 399378 [details] [diff] [review]
patch v1.0

forgot something

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.1

this one is correct

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 399384 [details] [diff] [review]
patch v1.1


>+          // Mark root folders.
>+          if (aJSNode.id == self.bookmarks.placesRoot)
>+            aJSNode.root = "placesRoot";
>+          else if (aJSNode.id == self.bookmarks.bookmarksMenuFolder)
>+            aJSNode.root = "bookmarksMenuFolder";
>+          else if (aJSNode.id == self.bookmarks.tagsFolder)
>+            aJSNode.root = "tagsFolder";
>+          else if (aJSNode.id == self.bookmarks.unfiledBookmarksFolder)
>+            aJSNode.root = "unfiledBookmarksFolder";
>+          else if (aJSNode.id == self.bookmarks.toolbarFolder)
>+            aJSNode.root = "toolbarFolder";
>+        }

we should probably make the roots into consts for all of utils.js to reduce all the xpconnect traversals. please file a followup for this, or fix here.

r=me otherwise.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v1.2

addressed comments, replaced some instance of PU.bookmarks.some_root, that is bad.

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/bcca5bf83070

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 400012 [details] [diff] [review]
patch v1.2

asking approval, hard to test automatically, but small change.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 400012 [details] [diff] [review]
patch v1.2

a192=beltzner, if we do this, needs to be for beta

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/79ae4d37ab67

Comment 21

[Gervase Markham [:gerv]]

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