426649 - Reordering folders in the left pane of Places Library is inconsistent/broken

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Max Shirshin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

Reordering subfolders of the same hierarchy level in the left pane of Library (Bookmarks/Organize Bookmarks) using drag-n-drop sometimes produces inconsistent results or doesn't work at all.



Reproducible: Sometimes

Steps to Reproduce:
1. Create a new profile.
2. Open "Bookmarks/Organize Bookmarks" (Library) window.
3. Create several new (empty) subfolders under the Bookmarks Menu folder so that they are siblings of the same level.
4. On the left pane of the Library window, try to rearrange the newly created folders several times using drag-n-drop. Drag folders so that only their order is changed (i.e. they still must remain siblings and direct subfolders of the Bookmarks Menu).
Actual Results:  
Sometimes a folder being drag-and-dropped doesn't move to where it should be according to a drag-n-drop indicator, but instead shifts forward or backward by 1 position; sometimes, it doesn't move at all (the latter case is reproducible, i.e. when the same item is dragged onto the same position again, it doesn't move again).

Usually it takes me 10-20 attempts of reordering folders to reproduce this bug, while having a total of 4 subfolders in the Bookmarks Menu.

Expected Results:  
When folders are rearranged by drag-n-drop, they should always move to the place shown by the drag-n-drop indicator.

I can reproduce the bug only using the LEFT pane of the Library to rearrange items; the right pane, as well as reordering folders directly in the Bookmarks menu, works for me.

Comment 1

[Max Shirshin]

Since the issue with folders not moving where they should can be reproduced even after the browser restart, I decided to attach my whole profile archived as a ZIP archive.

To reproduce the bug, do the following:
- start Firefox 3 beta 5 with the provided profile (maybe it is enough to use bookmarks from it)
- open Bookmarks/Organize Bookmarks
- in the Bookmarks Menu folder, find the "kkkkkkk" subfolder
- drag-n-drop it by 1 position up, exactly between folders "2" and "3" that can be found on the same level

Expected results: "kkkkkkk" folder should move between "2" and "3" folders
Actual results: folder "kkkkkkk" doesn't move to that position and remains where it is

Comment 2

[Max Shirshin]

Attachment: My profile (zipped) containing a bookmarks folder structure where the bug can be partially reproduced

see my previous comment for details and testcase description

Comment 3

[Max Shirshin]

The same bug exists in the latest nightly build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040305 Minefield/3.0pre

Comment 4

[Damian Shaw [Quan]]

Can confirm.

My STR:

1) Create new profile

2) Go to places library

3) In the bookmark menu of the places library, create 4 new folders, "New Folder", "New Folder2", "New Folder3" and "New Folder4"

4) In the left pane attempt to drag "New Folder" below either "New Folder2" or "New Folder3"

5) Nothing happens

Dragging up seemed to work for me and dragging below the bottom folder seemed to work for me.

Comment 5

[Marco Bonardo [:mak]]

so problem feels like being wrong new index calculation here. We have excludeItems in the Library left pane folder, you're moving down to a certain index, but it does not take in count hidden children. We should make index calculation aware of excludeItems.

Comment 6

[Marco Bonardo [:mak]]

mh, no, we are taking in count the excludeItems and inserting at the end if that's true. Still we are doing that only in tree.xml so we should sync the code in treeView.js to do the same... This way when moving a folder it will be put at the end.
We don't know what is the actual position of the element so we can't know the eact position the user want to put that, still we know that user wants to move it before or after another folder, and that is what we should try to do.
Using GetItemIndex would work here, but could be quite expensive since for the duration of the drag we continue to ask indexes to the db.

Comment 7

[Marco Bonardo [:mak]]

mh, my actual idea is adding new kind of orientation drops, we actually have DROP_BEFORE, DROP_AFTER, DROP_ON. i think we could add DROP_AFTER_ID and DROP_BEFORE_ID, for those as index we could pass the itemId, then at the end we call getItemIndex and drop based on returned position.

Comment 8

[Max Shirshin]

In Firefox 2, the functionality of drag-n-dropping folders in the left pane is not available at all. Maybe if it cannot be fixed reliably in sync with FF3 final release, it should be disabled at all, at least for the release branch?

Comment 9

[Hideo Oshima]

I can confirm with recent trunk/Linux.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041820 Minefield/3.0pre

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch

this is specifically for excludeItems trees, so should not create any problem on other views, being served directly by the getter when we ask for value we get a real index and not a bogus one so that should be safe too.
In future we could use this method everywhere, actually IIRC to calculate every index we call getIndexOfNode that walks the full container to return a valid index, and that is called at every drop movement, with this method instead we have one more db call but is executed only when we use the index value.

I've added a check to hasChildren because that behaviour is bogus in an excludeItems tree, i often finished up inserting a folder as first child of another instead of near it, only because it was marked as open but had excluded children (so no children really), i can drop on if i want to put the item inside.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 316424 [details] [diff] [review]
patch

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

>  */
> function InsertionPoint(aItemId, aIndex, aOrientation, aIsTag) {
>   this.itemId = aItemId;
>-  this.index = aIndex;
>-  this.orientation = aOrientation;
>+  this._index = aIndex;
>+  this._orientation = aOrientation;
>   this.isTag = aIsTag;
> }
>-InsertionPoint.prototype.toString = function IP_toString() {
>-  return "[object InsertionPoint(folder:" + this.itemId + ",index:" + this.index + ",orientation:" + this.orientation + ",isTag:" + this.isTag + ")]";
>-};
>+
>+InsertionPoint.prototype = {
>+  set index(val) {
>+    this._index = val;
>+  },
>+

Setters should return the value set, i.e. make that |return this._index = val|

>+  get index() {
>+    // For DROP_BEFORE_ID and DROP_AFTER_ID the insertionPoint.index is really
>+    // the itemId near which we should drop.

...the index of the item near which...

>+    switch (this._orientation) {
>+      case PlacesUIUtils.DROP_BEFORE_ID:
>+        return PlacesUtils.bookmarks.getItemIndex(this._index);
>+      case PlacesUIUtils.DROP_AFTER_ID:
>+        return PlacesUtils.bookmarks.getItemIndex(this._index) + 1;
>+      default:
>+        return this._index;
>+    }
>+  },

If we do keep this (see later), remove the default case and put it outside of the switch block.

>+
>+  set orientation(val) {
>+    this._orientation = val;
>+  },

See above.

>+
>+  get orientation() {
>+    // convert special insertionPoint orientation values for excludeItems trees.
>+    switch (this._orientation) {
>+      case PlacesUIUtils.DROP_BEFORE_ID:
>+        return Ci.nsITreeView.DROP_BEFORE;
>+      case PlacesUIUtils.DROP_AFTER_ID:
>+        return Ci.nsITreeView.DROP_AFTER;
>+      default:
>+        return this._orientation;
>+    }
>+  },

hrm, I really don't like this (this.orientation is not what you set this.orientation to) . Can we set DROP_BEFORE_ID/DROP_AFTER_ID as new property instead?

>+  
>+  toString: function IP_toString() {
>+    return "[object InsertionPoint(folder:" + this.itemId + ",index:" + this.index + ",orientation:" + this.orientation + ",isTag:" + this.isTag + ")]";
>+  }

Let's remove this debug stuff.

Comment 12

[Marco Bonardo [:mak]]

> >+
> >+  get orientation() {
> >+    // convert special insertionPoint orientation values for excludeItems trees.
> >+    switch (this._orientation) {
> >+      case PlacesUIUtils.DROP_BEFORE_ID:
> >+        return Ci.nsITreeView.DROP_BEFORE;
> >+      case PlacesUIUtils.DROP_AFTER_ID:
> >+        return Ci.nsITreeView.DROP_AFTER;
> >+      default:
> >+        return this._orientation;
> >+    }
> >+  },
> 
> hrm, I really don't like this (this.orientation is not what you set
> this.orientation to) . Can we set DROP_BEFORE_ID/DROP_AFTER_ID as new property
> instead?

i've done like this because in other code we could expect that this.orientation has a valid value (from nsITreeView). Alternatively as you said we could add a 5th bool property to insertionPoint called convertIndex or dropOnItemId (or something similar). I simply found cleaner having those orientation grouped with others since they have the same meaning.
If you think that this could bring to future bug i'll go for the new property, but i don't see why i should read the orientation from the insertionPoint and expect to get the DROP_BEFORE_ID/DROP_AFTER_ID if the index will be replaced on get (i only need to know if it's after or before)

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch

Mano: this should follow your suggestion, added dropNearItemId optional param, orientation is standard, index calculated on request if needed. Other comments fixed

Comment 14

[Asaf Romano (gone)]

Comment on attachment 316583 [details] [diff] [review]
patch

r=mano.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 316583 [details] [diff] [review]
patch

a=mconnor on behalf of 1.9 drivers

Comment 16

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.232; previous revision: 1.231
done
Checking in browser/components/places/content/tree.xml;
/cvsroot/mozilla/browser/components/places/content/tree.xml,v  <--  tree.xml
new revision: 1.115; previous revision: 1.114
done
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.67; previous revision: 1.66
done

Comment 17

[Frederic Bezies]

Erh... Sorry to spam this bug, but could this patch produce this kind of error when trying to drag and drop an icon on personal toolbar :

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/controller.js :: PCDH_canMoveContainer :: line 1387"  data: no]

Comment 18

[Marco Bonardo [:mak]]

there's already a bug on that (bug 430948), is not caused by this check-in

Comment 19

[Frederic Bezies]

Thanks for the tip.

Sorry I don't find it before :(

Comment 20

[Tracy Walker [:tracy]]

verified with: 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042904 Minefield/3.0pre

Comment 22

[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