If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

"Bookmarks Menu" folder can be dropped into itself

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: dao, Assigned: Michael Schonfeld)

Tracking

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Assignee)

Comment 3

10 years ago
Created attachment 306020 [details] [diff] [review]
v1
Assignee: nobody → dev
Status: NEW → ASSIGNED
Attachment #306020 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Attachment #306020 - Flags: review?(mano)
(Assignee)

Comment 4

10 years ago
Postponing review until resolution of bug 384370, due to changes in the wrapNode function.
Target Milestone: --- → Firefox 3
Depends on: 384370
(Assignee)

Comment 5

10 years ago
Created attachment 309442 [details] [diff] [review]
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.
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-
(Assignee)

Comment 7

10 years ago
Created attachment 309457 [details] [diff] [review]
v2.1
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+
(Assignee)

Comment 9

10 years ago
Created attachment 309460 [details] [diff] [review]
For checkin
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.
Created attachment 309488 [details] [diff] [review]
for checkin

Also, do the type-check first.
Attachment #309460 - Attachment is obsolete: true
Created attachment 309489 [details] [diff] [review]
for checkin

Wrong file :-/
Attachment #309488 - Attachment is obsolete: true
Attachment #309489 - Attachment is patch: true
Attachment #309489 - Attachment mime type: application/octet-stream → text/plain
Created attachment 309491 [details] [diff] [review]
for checkin for real!

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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 423169
Depends on: 423163
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.