Closed Bug 418592 Opened 13 years ago Closed 13 years ago

"Bookmarks Menu" folder can be dropped into itself

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: dao, Assigned: dev)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Flags: blocking-firefox3?
Blocking for another potential to hoark one's Places database ...
Priority: -- → P2
Flags: blocking-firefox3? → blocking-firefox3+
mconnor told a terrific story about how the Mac menu indexer followed this to infinity and beyond.
OS: Windows XP → Windows 95
Hardware: PC → All
OS: Windows 95 → All
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → dev
Status: NEW → ASSIGNED
Attachment #306020 - Flags: review?(mano)
Attachment #306020 - Flags: review?(mano)
Postponing review until resolution of bug 384370, due to changes in the wrapNode function.
Target Milestone: --- → Firefox 3
Depends on: 384370
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #306020 - Attachment is obsolete: true
Attachment #309442 - Flags: review?(mano)
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:".
Attachment #309442 - Flags: review?(mano) → review-
Attached patch v2.1 (obsolete) — Splinter Review
Attachment #309442 - Attachment is obsolete: true
Attachment #309457 - Flags: review?(mano)
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.
Attachment #309457 - Flags: review?(mano) → review+
Attached patch For checkin (obsolete) — Splinter Review
Attachment #309457 - Attachment is obsolete: true
> +    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.
Attached patch for checkin (obsolete) — Splinter Review
Also, do the type-check first.
Attachment #309460 - Attachment is obsolete: true
Attached patch for checkin (obsolete) — Splinter Review
Wrong file :-/
Attachment #309488 - Attachment is obsolete: true
Attachment #309489 - Attachment is patch: true
Attachment #309489 - Attachment mime type: application/octet-stream → text/plain
I forgot to change the last return statement.
Attachment #309489 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 423169
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Flags: in-litmus? → in-litmus+
Test case 5947 has been updated to take this bug into account.
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.