See bug 1219804 comment 7

Comment from verdi: should be draggable, and operation should be a copy instead of move.

I'll check if this hasn't been fixed already by bug 1248267.

Michael, I think the current behavior is what you seem to want in comment 1. Can you check that in a recent Nightly build - see https://nightly.mozilla.org/?

(In reply to Justin Dolske [:Dolske] from comment #1) > Comment from verdi: should be draggable, and operation should be a copy > instead of move. (In reply to Mike de Boer [:mikedeboer] from comment #3) > Michael, I think the current behavior is what you seem to want in comment 1. > Can you check that in a recent Nightly build - see > https://nightly.mozilla.org/? I apologize. I misunderstood the implications of copy vs. move. I must have been worried about bookmarks disappearing from the recent bookmarks list. The recent bookmarks are draggable - yay! But they work differently than the recent bookmarks smart folder. Dragging from the recent bookmarks list copies the bookmark to another place AND makes a duplicate entry in the recent bookmarks list. I don't think we want either of those results.

(In reply to Verdi [:verdi] from comment #4) > I apologize. I misunderstood the implications of copy vs. move. I must have > been worried about bookmarks disappearing from the recent bookmarks list. > The recent bookmarks are draggable - yay! But they work differently than the > recent bookmarks smart folder. Dragging from the recent bookmarks list > copies the bookmark to another place AND makes a duplicate entry in the > recent bookmarks list. I don't think we want either of those results. I think that's a more general improvement: does it make sense to have duplicate entries in the 'Recently Bookmarked' list? I don't think so. Dão, is it possible to adjust the query to only return unique URLs?

(In reply to Verdi [:verdi] from comment #4) > I apologize. I misunderstood the implications of copy vs. move. I must have > been worried about bookmarks disappearing from the recent bookmarks list. > The recent bookmarks are draggable - yay! But they work differently than the > recent bookmarks smart folder. Dragging from the recent bookmarks list > copies the bookmark to another place AND makes a duplicate entry in the > recent bookmarks list. I don't think we want either of those results. Regardless of comment 5, can you specify what result you _do_ expect?

(In reply to Mike de Boer [:mikedeboer] from comment #5) > (In reply to Verdi [:verdi] from comment #4) > > I apologize. I misunderstood the implications of copy vs. move. I must have > > been worried about bookmarks disappearing from the recent bookmarks list. > > The recent bookmarks are draggable - yay! But they work differently than the > > recent bookmarks smart folder. Dragging from the recent bookmarks list > > copies the bookmark to another place AND makes a duplicate entry in the > > recent bookmarks list. I don't think we want either of those results. > > I think that's a more general improvement: does it make sense to have > duplicate entries in the 'Recently Bookmarked' list? I don't think so. > Dão, is it possible to adjust the query to only return unique URLs? Maybe? My SQL is rusty, but this another abstraction layer anyway, so I really have no clue. Forwarding to mak.

(In reply to Mike de Boer [:mikedeboer] from comment #6) > Regardless of comment 5, can you specify what result you _do_ expect? I would expect that if I dragged a bookmark from the recent bookmark list to, for example, the bookmarks menu it would be moved from the other bookmarks folder to the bookmarks menu folder and there would still only be its one original entry in the recent bookmarks list.

(In reply to Verdi [:verdi] from comment #8) > I would expect that if I dragged a bookmark from the recent bookmark list > to, for example, the bookmarks menu it would be moved from the other > bookmarks folder to the bookmarks menu folder and there would still only be > its one original entry in the recent bookmarks list. Thanks! Well, that's not the case currently, so there's work to be done here.

No, not Dungeons and Dragons.

Comment on attachment 8757005 [details] [diff] [review] Patch v1: move simulated places nodes after a successful DnD operation Review of attachment 8757005 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/controller.js @@ +1550,5 @@ > * > * @param aNode > * A nsINavHistoryResultNode node. > + * @param aDOMNode > + * A XUL DOM node. Optional. @param [optional] aDOMNode @@ +1555,4 @@ > * @return True if the node can be moved, false otherwise. > */ > canMoveNode: > + function PCDH_canMoveNode(aNode, aDOMNode) { while here, let's use a shorthand: canMoveNode(aNode, aDOMNode) { @@ +1565,5 @@ > + // Normally, parentless places nodes can not be moved. > + if (!aDOMNode) > + return false; > + // Except when they are simulated, thus special. > + return aDOMNode.hasAttribute("simulated-places-node"); I want a slightly stricter check and the conditions can be coalesced: // Normally parentless places nodes can not be moved, // but simulated bookmarked URI nodes are special. return aDOMNode && aDOMNode.hasAttribute("simulated-places-node") && PlacesUtils.nodeIsBookmark(aNode);

(In reply to Marco Bonardo [::mak] from comment #11) > // Normally parentless places nodes can not be moved, > // but simulated bookmarked URI nodes are special. > return aDOMNode && > aDOMNode.hasAttribute("simulated-places-node") && > PlacesUtils.nodeIsBookmark(aNode); ...but with this snippet only simulated nodes may ever be moved. The `&&` use is a bit confusing here...

I think you want me to use: ```js // Normally parentless places nodes can not be moved, // but simulated bookmarked URI nodes are special. return (!aDOMNode || aDOMNode.hasAttribute("simulated-places-node")) && PlacesUtils.nodeIsBookmark(aNode); ``` right?

(In reply to Mike de Boer [:mikedeboer] from comment #12) > (In reply to Marco Bonardo [::mak] from comment #11) > > // Normally parentless places nodes can not be moved, > > // but simulated bookmarked URI nodes are special. > > return aDOMNode && > > aDOMNode.hasAttribute("simulated-places-node") && > > PlacesUtils.nodeIsBookmark(aNode); > > ...but with this snippet only simulated nodes may ever be moved. The `&&` > use is a bit confusing here... This would still be under the if (!parentNode) check... right? I don't see what you mean.

https://hg.mozilla.org/integration/fx-team/rev/4dbe106594fa8197404081eb801cbd4b2a42d06b Bug 1248616 - move simulated places nodes after a successful drag 'n drop operation, which applies to Recently Bookmarked items. r=mak

Backed out for failing browser_423515.js: https://hg.mozilla.org/integration/fx-team/rev/5c8f09493cad Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=4dbe106594fa8197404081eb801cbd4b2a42d06b Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=9637625&repo=fx-team 09:01:07 INFO - 173 INFO TEST-START | browser/components/places/tests/browser/browser_423515.js 09:01:07 INFO - TEST-INFO | started process screentopng 09:01:09 INFO - TEST-INFO | screentopng: exit 0 09:01:09 INFO - 174 INFO checking window state 09:01:09 INFO - 175 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | checking PlacesUtils, running in chrome context? - 09:01:09 INFO - 176 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | checking PlacesUIUtils, running in chrome context? - 09:01:09 INFO - 177 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | checking PlacesControllerDragHelper, running in chrome context? - 09:01:09 INFO - 178 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | confirm test root is empty - 09:01:09 INFO - 179 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populate added data to the test root - 09:01:09 INFO - 180 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | can move regular folder node - 09:01:09 INFO - 181 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populated data to the test root - 09:01:09 INFO - 182 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | node is folder - 09:01:09 INFO - 183 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | folder id and folder node item id match - 09:01:09 INFO - 184 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | node is folder shortcut - 09:01:09 INFO - 185 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | shortcut id and shortcut node item id match - 09:01:09 INFO - 186 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | shortcut node id and concrete id match - 09:01:09 INFO - 187 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | can move folder shortcut node - 09:01:09 INFO - 188 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populated data to the test root - 09:01:09 INFO - 189 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | bookmark id and bookmark node item id match - 09:01:09 INFO - 190 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | query id and query node item id match - 09:01:09 INFO - 191 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | can move query node - 09:01:09 INFO - 192 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | populated data to the test root - 09:01:09 INFO - 193 INFO TEST-PASS | browser/components/places/tests/browser/browser_423515.js | Node found - 09:01:09 INFO - 194 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_423515.js | shouldn't be able to move special folder node - Got undefined, expected false

Well, that does seem like the only failure and the fix is ultra-simple. Pushed to try to prove it.

https://hg.mozilla.org/integration/fx-team/rev/a9d17539d46af1dc601c51277eee909874d0e9ad Bug 1248616 - move simulated places nodes after a successful drag 'n drop operation, which applies to Recently Bookmarked items. r=mak

I have reproduced this bug with Nightly 47.0a1 (2016-02-16)on Windows 8.1 64 bit! This bug's fix is verified on Latest Aurora 49.0a2. Build ID : 20160720004018 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 [bugday-20160720]

Reproduced this issue in firefox nightly 47.0a1 (2016-02-16) with ubuntu 16.04 (64 bit) Verified as this issue fixed with latest firefox aurora 49.0a2 (Build ID: 20160722004032) Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0 As it is also verified on windows (Comment 21), Marking it as verified!