Closed Bug 416580 Opened 15 years ago Closed 7 years ago

Don't allow pasting "special" Library folders anywhere.

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: adelfino, Unassigned)

References

Details

(Keywords: uiwanted)

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020908 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020908 Minefield/3.0b4pre

These context menu options are now allowed in Library, and can duplicate "special" folders:

Copying "History".
Copying "Tags", viewing "Tags" properties.
Copying "All Bookmarks", viewing "All Bookmarks" properties.
Copying "Bookmarks Toolbar", viewing "Bookmarks Toolbar" properties.
Copying "Bookmarks Menu", viewing "Bookmarks Menu" properties.
Copying "Unfiled Bookmarks", viewing "Unfiled Bookmarks" properties.

Reproducible: Always
Version: unspecified → Trunk
Summary: Several Library options than should be disallowed → Several Library options should be disallowed
Summary: Several Library options should be disallowed → Several Library context menu options should be disallowed
Flags: blocking-firefox3?
(In reply to comment #1)
> Dupe of Bug 415667 ?
> 

No, this bug deals with duplicating "special folders": "History". "Tags", "All Bookmarks", "Bookmarks Toolbar", "Bookmarks Menu", "Unfiled Bookmarks"; or showing options which are potentially destructive (like changing their name).
Summary: Several Library context menu options should be disallowed → Disable copying and other commands on "special" places folders
Summary: Disable copying and other commands on "special" places folders → Disable copying and other commands on "special" Library folders
OS: Linux → All
Hardware: PC → All
no, copy should not be disabled, until copy is "make a copy of the query in a new item" and not make a copy of the content. suppose that i want put a copy of unfiled folder into the toolbar, or tags into bookmarks meno and so on. what should be disabled are move and delete that is mostly Bug 406089
(In reply to comment #3)
> no, copy should not be disabled, until copy is "make a copy of the query in a
> new item" and not make a copy of the content. suppose that i want put a copy of
> unfiled folder into the toolbar, or tags into bookmarks meno and so on. what
> should be disabled are move and delete that is mostly Bug 406089
> 

You can consider this bug a (partial) spin-off of:

https://bugzilla.mozilla.org/show_bug.cgi?id=406089#c5
Blocking for determination on potential DB hoarking, here.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Priority: P3 → P4
Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Nominating for blocking‑firefox3.1 now that we have some time.
Flags: blocking-firefox3.1?
There should be no risk of database problems, as copying these results in an un-special copy.

However, there's a significant risk of confusing the user, who may expect ex-special folders to act specially after a copy. I'd really like to see copy on these folders disabled due to this.
This will not block 3.1, as it's not a P1 feature nor a program error without a workaround. However, I'd really like to see this fixed. Adding uiwanted to get some feedback from UX (see comment #8).
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Keywords: uiwanted
Target Milestone: --- → Firefox 3.1
Priority: P4 → P2
Assignee: nobody → ddahl
Target Milestone: Firefox 3.1 → ---
Depends on: 485442
Attached patch patch 0.1Splinter Review
This patch keeps you from pasting top level roots anywhere in places.
Attachment #371750 - Flags: review?(mak77)
Comment on attachment 371750 [details] [diff] [review]
patch 0.1

looks like you are disabling pasting something we already have in the clipboard.
The bug is about disabling copy/cut/delete commands (shortcuts/context menus) on those items, being able to copy and then unable to paste would be a bad experience for users.
So you should work more on command enabled part of the controller rather than checking later if you have copied something bad.
Attachment #371750 - Flags: review?(mak77) → review-
There is always the case where someone wants to copy the "All Bookmarks" root and paste it into a text document outside of Fx, or into a web app. Disabling copy would be quite frustrating to users who want to use the copied data outside of places.
what's the use case of copying All Bookmarks container and pasting into another app?
I have done that before, perhaps to use the urls in a spreadsheet or something. It does seem like a valid edge case. "Poor-man's backup":)
(In reply to comment #13)
> what's the use case of copying All Bookmarks container and pasting into another
> app?

To echo David's comments, I've found it useful before to be able to copy special library folders to paste into other apps (Emacs really).  It's not the copying that's the problem but the pasting back into Places.
ok... well really personally i think also pasting is not (should not be) an issue, we should simply create a shortcut to the special folder, and let user manage that (that is bug 435851), but i also see that could be confusing for non technical users.
Options i see here are:
1. disable copy command, this won't allow to paste contents of a special folder in an external app
2. disable paste, this will confuse a user that can copy a folder, but then cannot paste it (i'm sure some user will file this bug)
3. Allow copy, but fix it so that it creates special copies that will auto-update (bug 435851). Good for technical users that want a special link to unsorted or tags in the toolbar or similar features, could confuse other users if they wrongly create a cyclic root (a root containing a link to itself, potentially we could not allow pasting in this case if what we are pasting is one of the ancestors of the insertion point).

I'm all for 3, but is clearly my personal idea. Dietrich?
#2 is the best resolution to this bug IMO.

#3 should be filed as a separate issue, needs more discussion.
Comment on attachment 371750 [details] [diff] [review]
patch 0.1

ok, fine for me, then i'll re-evaluate the patch.
Attachment #371750 - Flags: review- → review?(mak77)
Comment on attachment 371750 [details] [diff] [review]
patch 0.1

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -345,55 +345,106 @@ PlacesController.prototype = {
>       if (nodes[i] == root)
>         return true;      
>     }
> 
>     return false;
>   },
> 
>   /**
>-   * Looks at the data on the clipboard to see if it is paste-able. 
>+   * Checks if the current clipboard data can be inserted whether it is
>+   * dropped or pasted
>+   */

not completely true, we are passing unwrappedNodes to this method, so it is not directly
checking clipboard data.
So it should be something like _arePlacesNodesPastable, then it should run through nodes and see if any of the nodes is one of the leftpanequeries.

>+
>+  _isDataInsertable: function PC__isDataInsertable(unwrappedNodes){
>+
>+    var ip = this._view.insertionPoint;
>+    var node = unwrappedNodes[0];

unwrappedNodes can be an array of nodes, isn't it? So you should check all of its elements.

>+
>+    if (node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER ||
>+          /^place:/.test(node.uri)) {
>+        var parentId = ip.itemId;
>+        while (parentId != PlacesUtils.placesRootId) {
>+          if (node.concreteId == parentId || node.id == parentId){
>+            return false;
>+          }
>+          for each (let val in PlacesUIUtils.leftPaneQueries){
>+            if (node.id == val || node.concreteId == val){
>+              return false;
>+            }
>+          }
>+          parentId = PlacesUtils.bookmarks.getFolderIdForItem(parentId);

what's the purpose of this complex check? it will degrade performances since has to cycle till the root, causing a bunch of queries.
I think you should only check if one of the copied nodes is one of the leftPaneQueries, is there something other you are trying to avoid?
Looks like you are trying to avoid pasting inside a parent, unluckily with our current api doing that would be a pain causing too much overhead. Unless we have a bs.isDecendantOf(folderId) API method (with a nested tree table), i would not take this hit.
You could at a maximum walk up the dom tree from the node you are pasting into and check parents, should be a less heavy hog, but could be less reliable.

>+
>+  /**
>+   * Looks at the data on the clipboard to see if it is paste-able.
>    * Paste-able data is:
>    *   - in a format that the view can receive
>    * @returns true if: - clipboard data is of a TYPE_X_MOZ_PLACE_* flavor,
>                        - clipboard data is of type TEXT_UNICODE and
>                          is a valid URI.
>    */
>   _isClipboardDataPasteable: function PC__isClipboardDataPasteable() {
>     // if the clipboard contains TYPE_X_MOZ_PLACE_* data, it is definitely
>     // pasteable, with no need to unwrap all the nodes.

this would end up being no more valid, now we would have to unwrap all nodes to check if they are roots.

> 
>     var flavors = PlacesControllerDragHelper.placesFlavors;
>     var clipboard = PlacesUIUtils.clipboard;
>     var hasPlacesData =
>-      clipboard.hasDataMatchingFlavors(flavors, flavors.length,
>-                                       Ci.nsIClipboard.kGlobalClipboard);
>-    if (hasPlacesData)
>-      return this._view.insertionPoint != null;
>+    clipboard.hasDataMatchingFlavors(flavors, flavors.length,
>+                                     Ci.nsIClipboard.kGlobalClipboard);

nit: bad indentation

>+    var xferable = Cc["@mozilla.org/widget/transferable;1"].
>+                   createInstance(Ci.nsITransferable);
>+    if (hasPlacesData){
>+      try {
>+        xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER);
>+        xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
>+        clipboard.getData(xferable, Ci.nsIClipboard.kGlobalClipboard);
>+        var data = { }, type = { };
>+        xferable.getAnyTransferData(type, data, { });
>+        data = data.value.QueryInterface(Ci.nsISupportsString).data;
>+        var unwrappedNodes = PlacesUtils.unwrapNodes(data, type.value);
>+
>+        if(!this._isDataInsertable(unwrappedNodes)){
>+          return false;
>+        }
>+
>+        return this._view.insertionPoint != null;
>+      }
>+      catch(e){
>+        dump(e);
>+        return false;
>+      }
>+    }
>+

you should try to get a single transferable and unwrapNodes only one time if possible, to reduce work load and avoid doing same things 2 times. simply add your flavors below.

> 
>     // if the clipboard doesn't have TYPE_X_MOZ_PLACE_* data, we also allow
>     // pasting of valid "text/unicode" and "text/x-moz-url" data
>-    var xferable = Cc["@mozilla.org/widget/transferable;1"].
>+    let xferable = Cc["@mozilla.org/widget/transferable;1"].

don't mix var and let please.

> 
>       // unwrapNodes() will throw if the data blob is malformed.
>-      var unwrappedNodes = PlacesUtils.unwrapNodes(data, type.value);
>+      var unwrappedNodes = PlacesUtils.unwrapNodes(data, typevalue);

ugh! this is wrong

>   /**
>+   * Check for special 'No Copy' node that should not be copied
>+   */
>+  isNoCopyNode: function PC_isNoCopyNode(aNode) {
>+    if (aNode.indentLevel < 1){
>+      return true;
>+    }
>+    else {
>+      return false;
>+    }
>+  },
>+

what's this? looks unused.

>@@ -1207,16 +1272,20 @@ PlacesController.prototype = {
>       var xferable = makeXferable(types);
>       clipboard.getData(xferable, Ci.nsIClipboard.kGlobalClipboard);
> 
>       var data = { }, type = { };
>       try {
>         xferable.getAnyTransferData(type, data, { });
>         data = data.value.QueryInterface(Ci.nsISupportsString).data;
>         var items = PlacesUtils.unwrapNodes(data, type.value);
>+        // do not allow pasting of parent nodes into child nodes
>+        if(!self._isDataInsertable(items)){
>+          return [];
>+        }

since we should not even allow to paste, the correct thing to do is to disable cmd_paste, so all the checking code should be into _IsClipBoardDataPasteable. If we have reached paste() with bogus data that means we have a bug somewhere that allow to paste when we don't want to. So we should fix that bug and not workaround paste().


>diff --git a/browser/components/places/tests/browser/browser_416580_no_copy_special_folders.js b/browser/components/places/tests/browser/browser_416580_no_copy_special_folders.js

>+      var tests = {
>+
>+        sanity: function tests_sanity(){
>+          ok(PU, "PlacesUtils in scope");
>+          ok(PUIU, "PlacesUIUtils in scope");
>+          ok(PO, "Places organizer in scope");
>+        },
>+
>+        testCannotPaste: function tests_cannotPaste(){
>+
>+          try {
>+
>+            // make sure you cannot paste special folders into a child node
>+            PO._places.controller.copy();

what are we copying? please be more explicit, select a node, check the selected node is correct, copy it, check that clipboard is ready to paste and contains your node, then paste.

>+            // get the unsorted bookmarks
>+            var node = PO._places.view.nodeForTreeIndex(bs.unfiledBookmarksFolder);
>+            PO.selectLeftPaneQuery("UnfiledBookmarks");

to get the node you can use .selectedNode after the selection, and check it is correct.

>+            PO._places.controller.paste();
>+
>+            is(node.hasChildren,false,"Node does not have children");

notice the left pane node is an excludeItems node, so this could always return false.
Attachment #371750 - Flags: review?(mak77) → review-
Summary: Disable copying and other commands on "special" Library folders → Don't allow pasting "special" Library folders anywhere.
Duplicate of this bug: 432368
Flags: wanted-firefox3.5+ → wanted-firefox3.6+
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
Assignee: ddahl → nobody
Priority: P2 → --
this should be no more neded, there should be no problem in copying queries.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.