418592 - "Bookmarks Menu" folder can be dropped into itself

Status: VERIFIED FIXED

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

Description

[Dão Gottwald [:dao]]

STR:

1. Open the Library
2. Click "All Bookmarks" in the left tree.
3. Drag the "Bookmarks Menu" folder in the content tree and drop it into itself or into "Bookmarks Menu" in the left tree.

Expected:
Dropping is not permitted.

Actual results:
The folder disappears from "All Bookmarks" on the left side. It's still shown on the right side, temporarily. While it's there, you can still drag it back to "All Bookmarks". Otherwise "Bookmarks Menu" also appears in the Bookmarks menu, from where you can drag it.

Comment 1

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

Blocking for another potential to hoark one's Places database ...

Comment 2

[Dietrich Ayala (:dietrich)]

mconnor told a terrific story about how the Mac menu indexer followed this to infinity and beyond.

Comment 3

[Michael Schonfeld]

Attachment: v1

Comment 4

[Michael Schonfeld]

Postponing review until resolution of bug 384370, due to changes in the wrapNode function.

Comment 5

[Michael Schonfeld]

Attachment: v2

On top of not allowing a folder to be dropped on it self, the patch will also disable the option of dropping a folder on any of its descendants.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 309442 [details] [diff] [review]
v2


>+    // Get the information of the dragged item
>+    var xferable = this._initTransferable(session);
>+    session.getData(xferable, 0);
>+    var data = { }, flavor = { };
>+    xferable.getAnyTransferData(flavor, data, { });
>+    data.value.QueryInterface(Ci.nsISupportsString);
>+    var dragged = PlacesUtils.unwrapNodes(data.value.data, flavor.value)[0];

Each dragged node should be checked (and then you should disallow dropping if _any_ of the items cannot be dropped).

>+    // The following loop disallows the dropping of a folder on itself or
>+    // on any of its descendants
>+    if (dragged.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER ||
>+        /^place/.test(dragged.uri)) {

nit: "^place:".

Comment 7

[Michael Schonfeld]

Attachment: v2.1

Comment 8

[Asaf Romano (gone)]

Comment on attachment 309457 [details] [diff] [review]
v2.1

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

>-  canDrop: function PCDH_canDrop() {
>+  canDrop: function PCDH_canDrop(ip) {
>     var session = this.getSession();
>-    if (session) {
>-      var types = PlacesUIUtils.GENERIC_VIEW_DROP_TYPES;
>-      for (var i = 0; i < types.length; ++i) {
>-        if (session.isDataFlavorSupported(types[i]))
>-          return true;
>+    var xferable = this._initTransferable(session);
>+    var dropCount = session.numDropItems;
>+    if (!session)
>+      return false;
>+

The order here is wrong, numDropItems should be used only once |session| is known to be non null.

r=mano otherwise.

Comment 9

[Michael Schonfeld]

Attachment: For checkin

Comment 10

[Asaf Romano (gone)]

> +    for (var i = 0; i < types.length; ++i) {

I'm changing this to for(i =0 ... on checkin, |i| is declared in the first loop within this function.

Comment 11

[Asaf Romano (gone)]

Attachment: for checkin

Also, do the type-check first.

Comment 12

[Asaf Romano (gone)]

Attachment: for checkin

Wrong file :-/

Comment 13

[Asaf Romano (gone)]

Attachment: for checkin for real!

I forgot to change the last return statement.

Comment 14

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-places.js 1.119
mozilla/browser/components/places/content/controller.js 1.222
mozilla/browser/components/places/content/menu.xml 1.122
mozilla/browser/components/places/content/toolbar.xml 1.145
mozilla/browser/components/places/content/treeView.js 1.59
mozilla/toolkit/components/places/src/utils.js 1.4

Comment 15

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre

Windows displays the circle / symbol. Mac provides no discouraging feedback btu doesn't permit the drop.

Comment 16

[Aakash Desai [:aakashd]]

Test case 5947 has been updated to take this bug into account.

Comment 17

[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