Closed Bug 418671 Opened 16 years ago Closed 16 years ago

Clean up places views drag and drop code

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.1b1

People

(Reporter: mak, Assigned: mak)

References

(
URL
)

Details

Attachments

(2 files, 8 obsolete files)

patch 1.4
94.21 KB, patch
asaf
: review+
Details | Diff | Splinter Review
fix failing mochitest
739 bytes, patch
Details | Diff | Splinter Review 
spun-off from Bug 389290:

>+      <!-- This returns the FavourSet accepted by this popup -->
>+      <method name="getSupportedFlavours">
>+        <body><![CDATA[
>+          var flavourSet = new FlavourSet();
>+          var acceptedDropFlavours = PlacesUtils.GENERIC_VIEW_DROP_TYPES;
>+          acceptedDropFlavours.forEach(flavourSet.appendFlavour, flavourSet);
>+          return flavourSet;

Mano: Either here or in a follow up, we should cache these once in placesutils.
Attached patch wip (obsolete) — — Splinter Review 
Mano, i don't want a real review on this, instead i'd like to know if you agree with the cleanup i'd like to do to the views D&D code.

Ideas are mainly:
- move common D&D code to PlacesControllerDragHelper
- change _GetDropPoint, we actually are walking the popup to find the drop point, my idea is to use the dropNearItemId property of insertionPoint and the event target, so we don't have to walk the popup. Actually when dropping over a non-places node i append at the end, i could detect if we are before startMarker and append at beginning but that is probably not so useful.
- fix drop into readonly containers using disallowInsertion util
- some cleanup

i've started moving common code and cleaning up menu.xml changing getDropPoint, this has some targetting problem on the toolbar menus due to missing event.layerY support (regression bug 453238) apart that it is working fine.

Can i go on with this? Some idea/changes from your point of view?
Attachment #336595 - Flags: review?(mano)
Summary: for views we could cache FlavourSet in PlacesUtils → Clean up places views drag and drop code
Assignee: nobody → mak77
Attached patch wip 0.2 (obsolete) — — Splinter Review 
Mano: as before, i only need an opinion so don't take a long time reviewing the full thing

i've changed to using event.dataTransfer, the main advantage is that we can set allowedEffect and, differently from dragAction it works as expected and a copy op forced from startDrag is retained till the end.

So this patch started as a cleanup but also potentially fixes some behaviour regarding forced copy operations, popups closing or not closing after a drag, drag&drop reordering, menu containers drop background on Vista, wrong markers, drag from readonly containers (recent regression bug 453165) and so on...
Attachment #336595 - Attachment is obsolete: true
Attachment #336706 - Flags: review?(mano)
Attachment #336595 - Flags: review?(mano)
Attached patch wip 0.3 (obsolete) — — Splinter Review 
fixed some regression in the Library
regarding https://bugzilla.mozilla.org/show_bug.cgi?id=453165#c11 this is what i got so far:

opening library; drag folders/items from library to bookmarks toolbar; deleting/restoring (undo)/renaming items within library:

Warning: reference to undefined property menupopup.childNodes[i].folderId
Source file: chrome://browser/content/places/editBookmarkOverlay.js
Line: 667
+
Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]
Source file: chrome://browser/content/places/editBookmarkOverlay.js
Line: 124

drag item (not folder) from bookmark menu (folder: bookmark toolbar) to bookmark toolbar (outside library):

Warning: assignment to undeclared variable eventY
Source file: chrome://browser/content/places/menu.xml
Line: 234
 ----------
Warning: assignment to undeclared variable nodeY
Source file: chrome://browser/content/places/menu.xml
Line: 235
 ----------
Warning: assignment to undeclared variable nodeHeight
Source file: chrome://browser/content/places/menu.xml
Line: 236

all above mentioned operations do work despite of the error console output. more testing to follow ...
Attached patch wip 0.4 (obsolete) — — Splinter Review 
- fixed closing of a popup when you start dragging from it
- fixed drop to desktop (regression from previous wip)
- fixed undeclared variables (comment 4)

(In reply to comment #4)
> regarding https://bugzilla.mozilla.org/show_bug.cgi?id=453165#c11 this is what
> i got so far:
> 
> opening library; drag folders/items from library to bookmarks toolbar;
> deleting/restoring (undo)/renaming items within library:
> 
> Warning: reference to undefined property menupopup.childNodes[i].folderId
> Source file: chrome://browser/content/places/editBookmarkOverlay.js
> Line: 667
> +
> Error: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE)
> [nsINavBookmarksService.getFolderIdForItem]
> Source file: chrome://browser/content/places/editBookmarkOverlay.js
> Line: 124

i can't reproduce this, so if you have better STR it would be good
Attachment #336706 - Attachment is obsolete: true
Attachment #336743 - Attachment is obsolete: true
Attachment #336706 - Flags: review?(mano)
did you set the pref "javascript.options.showInConsole" to 'true'?
(In reply to comment #6)
> did you set the pref "javascript.options.showInConsole" to 'true'?
yes, i have it always enabled, hwv i don't see that kind of error in editBookmarkOverlay.js. i'm doing a full rebuild though since i had some "fsync" patch in my queue, so i'll retry with a clean build.
Attached patch wip 0.5 (obsolete) — — Splinter Review 
- bookmarks menu closes if you drop on the "bookmarks" menuitem in the menubar
- fixed drop indicators when you can't drop (eg dropping a folder into itself)
- separators are no more considered uris when dragging (they were converted to ------------------, so if dropping over locationbar you were going to www.------------.com)
- fixed some regression from previous wip
- drag from queries (history) to desktop (link action)

Sorry, i still cannot reproduce first part of comment 4... I think we can generate a new build starting from this patch and test on it.
Will cc Henrik for that
Attachment #337425 - Attachment is obsolete: true
Target Milestone: --- → Firefox 3.1
well, i discovered i can generate trybuilds using Mercurial, damn good thing, will do that by myself (However thank you henrik). starting building in a couple minutes.
i'm still getting that error (always 3x at once) mentioned in comment 4 (first part):

so, with javascript.options.strict = true (you're really sure you set this proper?) error console reports:

Warning: reference to undefined property menupopup.childNodes[i].folderId
Source file: chrome://browser/content/places/editBookmarkOverlay.js
Line: 667

* every time i open up the library
* every time i click on a parent folder (all/tags/history/menu/toolbar/unsorted) in left pane in library
* every time i click in the right pane on a children item of a given parent folder


Error: transferData.first is null
Source file: chrome://global/content/nsDragAndDrop.js
Line: 458

* every time dropping a separator to loacation bar (there's no forbidden sign, though neither ther is without this patch, there's '--------------------' added to locationbar after the drop -> worth filing a new bug since i don't see any sense dropping non-text/non bookmarks to location bar?)


Error: 'Illegal value' when calling method: [nsITreeView::getParentIndex] = NS_ERROR_ILLEGAL_VALUE

* every time dragging & dropping a bookmark item from bookmarks toolbar (outside library) to library's right pane


Error: 'Illegal value' when calling method: [nsITreeView::isContainer] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::getRowProperties] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::isSeparator] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::isContainer] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::getCellProperties] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::getLevel] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::isContainer] = NS_ERROR_ILLEGAL_VALUE
 ----------
Error: 'Illegal value' when calling method: [nsITreeView::getImageSrc] = NS_ERROR_ILLEGAL_VALUE

* every time dragging & dropping a folder (e.g. 'Mozilla Firefox') from bookmarks menu to toolbar (both non library), then in library window choose organize/undo

more to follow :)
do you find the open on dragover better between menus? are they closing better?
(In reply to comment #11)
> i'm still getting that error (always 3x at once) mentioned in comment 4 (first
> part):
> 
> so, with javascript.options.strict = true (you're really sure you set this
> proper?) error console reports:
> 
> Warning: reference to undefined property menupopup.childNodes[i].folderId
> Source file: chrome://browser/content/places/editBookmarkOverlay.js
> Line: 667

Please fill a new bug and CC me, i can reproduce also in current version with strict mode on, so it's unrelated to this patch.


> Error: transferData.first is null
> Source file: chrome://global/content/nsDragAndDrop.js
> Line: 458
> 
> * every time dropping a separator to loacation bar (there's no forbidden sign,
> though neither ther is without this patch, there's '--------------------' added
> to locationbar after the drop -> worth filing a new bug since i don't see any
> sense dropping non-text/non bookmarks to location bar?)

There's already a bug on that, however i'll try to take care of this


> Error: 'Illegal value' when calling method: [nsITreeView::getParentIndex] =
> NS_ERROR_ILLEGAL_VALUE
> 
> * every time dragging & dropping a bookmark item from bookmarks toolbar
> (outside library) to library's right pane

will take care of this


> Error: 'Illegal value' when calling method: [nsITreeView::isContainer] =
> NS_ERROR_ILLEGAL_VALUE
>  ----------
> ... (and so on)

known bug, already filled, will not take care of this since it's not related to this patch and happens also in current version.
(In reply to comment #11)
> Error: transferData.first is null
> Source file: chrome://global/content/nsDragAndDrop.js
> Line: 458
> 

actually this ended up being a drag & drop toolkit bug on textboxes, Neil Deakin has probably found the cause, and will fill a bug on that.
that's bug 454595, so will not take care more of that, separators will not be draggable to textboxes (so locationbar and searchbar) when that bug will be fixed.
(In reply to comment #13)
> (In reply to comment #11)
> > Warning: reference to undefined property menupopup.childNodes[i].folderId
> > Source file: chrome://browser/content/places/editBookmarkOverlay.js
> > Line: 667
> 
> Please fill a new bug and CC me, i can reproduce also in current version with
> strict mode on, so it's unrelated to this patch.

filled Bug 454656
(In reply to comment #13)
> > Error: 'Illegal value' when calling method: [nsITreeView::getParentIndex] =
> > NS_ERROR_ILLEGAL_VALUE
> > 
> > * every time dragging & dropping a bookmark item from bookmarks toolbar
> > (outside library) to library's right pane

finally i can tell this is also visible in latest nightly, so not related to this patch, filled bug 454698.







> 
> will take care of this
> 
> 
> > Error: 'Illegal value' when calling method: [nsITreeView::isContainer] =
> > NS_ERROR_ILLEGAL_VALUE
> >  ----------
> > ... (and so on)
> 
> known bug, already filled, will not take care of this since it's not related to
> this patch and happens also in current version.
Attached patch wip 0.6 (obsolete) — — Splinter Review 
To sum up, these are actual changes:

1. move common D&D code to PlacesControllerDragHelper
2. change _GetDropPoint, we actually are walking the popup to find
   the drop point, use dropNearItemId instead. Actually when dropping over a
   non-places node i append at the end, i could detect if we are before
   startMarker and append at beginning but that is probably not needed.
3. fix drop into readonly containers using disallowInsertion
4. try to use event.dataTransfer, the main advantage is that we can set
   allowedEffect and, differently from dragAction it persists, and it makes code
   smaller
5. fixed closing of a popup when you start dragging from it
6. fixed Vista hover style (until we get a correct native fix)
7. close bookmarks menu if you drop on the menuitem in the menubar
8. fixed drop indicators when you can't drop (eg dropping a folder into itself)
9. separators are no more considered uris when dragging
10. drag from queries (history) to desktop (link action supported)
11. cleaned up markers (we still have to use builder="end" but i got rid of
    "start" builder and markers appear more consistent)
12. draggin up multiple items in Library was dropping them reverse
13. enable shift+drag for toolbar folders on Mac

Well there are other parts that could be cleaned up but i don't want to go further
than this, everything is working as far as i can tell and i'd like to avoid
regressions, if possible.

These errors that are not related to this patch:
1. Error: 'Illegal value' when calling method: [nsITreeView::getParentIndex]
   (bug 454698)
2. Error: 'Illegal value' when calling method: [nsITreeView:: . . . ]
   (bug 433370)
3. Warning: reference to undefined property menupopup.childNodes[i].folderId
   (Bug 454656)

Note: in this version i had to comment out some changes to controller onDrop and canDrop because dataTransfer is null in the session when dragging from the OS, will ask Neil for this.

new TryServer builds coming, will ask for testing in Mozillazine Forums
Attachment #337528 - Attachment is obsolete: true
Keywords: qawanted
Whiteboard: [needs testing]
Attached patch patch v1 (obsolete) — — Splinter Review 
- fixed some regression (menu, chevron) found on mozillazine forums
- fixed dragging toolbar folders in Linux with shift+drag (the same fix could be applied to other platforms, could make the behaviour better don't allowing the popupshowing if shift is pressed, i'll ask Beltzner if we can go consistent between platforms, supporting only shift+drag in all of them and removing alt drag from windows)
- made the tree dropFeedback | allowedEffect more consistent

This is the first patch i can consider "complete" i've fixed all i wanted to fix in this cleanup, more can be done in future but i prefer not moving forward now.

I've done a ton of manual testing, and i'm starting generating a new trybuild for continuing anti-regressions tests on mozillazine, i think that we can start with a real review though, patch is in good shape imho and Mano will probably need some time to review the full thing.
Attachment #338100 - Attachment is obsolete: true
Attachment #338608 - Flags: review?(mano)
Status: NEW → ASSIGNED
Whiteboard: [needs testing] → [needs help testing][needs review mano]
Mano: see comment 18 + comment 21 for actual changes, so you don't have to read the full bug
Marco, not everyone is willed to follow the forum posts. So it would be nice if you could also post the try server build link here. Thanks.
reminder: XP_LINUX does not work usually so use MOZ_WIDGET_GTK2
Attached patch patch v1.1 (obsolete) — — Splinter Review 
fixed a couple minor issues reported on mozillazine's forums
Attachment #338608 - Attachment is obsolete: true
Attachment #338753 - Flags: review?(mano)
Attachment #338608 - Flags: review?(mano)
Comment on attachment 338753 [details] [diff] [review]
patch v1.1

First pass:

diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -706,17 +706,16 @@ var BookmarksEventHandler = {
         target._endMarker = -1;
       }
       return;
     }
 
     if (!target._endOptSeparator) {
       // create a separator before options
       target._endOptSeparator = document.createElement("menuseparator");
-      target._endOptSeparator.setAttribute("builder", "end");

Why are you removing that? I thought you're just removing "start"

@@ -784,17 +783,18 @@ var BookmarksEventHandler = {
  */
 var BookmarksMenuDropHandler = {
   /**
    * Need to tell the session to update the state of the cursor as we drag
    * over the Bookmarks Menu to show the "can drop" state vs. the "no drop"
    * state.
    */
   onDragOver: function BMDH_onDragOver(event, flavor, session) {
-    session.canDrop = this.canDrop(event, session);
+    if (!this.canDrop(event, session))
+      event.dataTransfer.effectAllowed = "none";

Shouldn't we still set canDrop as well?


@@ -807,33 +807,50 @@ var BookmarksMenuDropHandler = {

+    // Close the menu
+    if (event.target.lastChild.localName == "menupopup")
+      event.target.lastChild.hidePopup();

I don't know if this is desired, ask ux@. I cannot test that.

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
@@ -1010,72 +1010,67 @@ PlacesController.prototype = {
 
   /**
-   * Get a TransferDataSet containing the content of the selection that can be
-   * dropped elsewhere. 
-   * @param   dragAction
-   *          The action to happen when dragging, i.e. copy
-   * @returns A TransferDataSet object that can be dragged and dropped 
-   *          elsewhere.
+   * Fills a DataTransfer with the content of the selection that can be
+   * dropped elsewhere.

a DataTransfer object.

-  getTransferData: function PC_getTransferData(dragAction) {
-    var copy = dragAction == Ci.nsIDragService.DRAGDROP_ACTION_COPY;
+  setDataTransfer: function PC_setDataTransfer(aEvent) {
+    var dt = aEvent.dataTransfer;
+    var doCopy = dt.effectAllowed == "copyLink";
+    var doMove = dt.effectAllowed == "move";
+

This is confusing, given that you can only do one of them. Keep one variable with constants.

@@ -1243,16 +1238,28 @@ PlacesController.prototype = {
 

-   * Determines if a container node can be moved.
+   * Determines if a node can be moved.
    * 
    * @param   aNode
-   *          A bookmark folder node.
-   * @param   [optional] aInsertionPoint
-   *          The insertion point of the drop target.
-   * @returns True if the container can be moved.
+   *          A nsINavHistoryResultNode node.
+   * @returns True if the node can be moved, false otherwise.
    */
-  canMoveContainerNode:
-  function PCDH_canMoveContainerNode(aNode, aInsertionPoint) {
+  canMoveNode:
+  function PCDH_canMoveNode(aNode) {
     // can't move query root
     if (!aNode.parent)
       return false;
 
-    var targetId = aInsertionPoint ? aInsertionPoint.itemId : -1;
     var parentId = PlacesUtils.getConcreteItemId(aNode.parent);
     var concreteId = PlacesUtils.getConcreteItemId(aNode);
 
-    // can't move tag containers 
-    if (PlacesUtils.nodeIsTagQuery(aNode))
+    // can't move child of tag containers

children

+    if (PlacesUtils.nodeIsTagQuery(aNode.parent))
       return false;
 
-    // check is child of a read-only container 
+    // can't move child of read-only containers

ditto.

 /**
    * Handles the drop of one or more items onto a view.
    * @param   insertionPoint
    *          The insertion point where the items should be dropped
    */
   onDrop: function PCDH_onDrop(insertionPoint) {
+    var dt = this.currentDataTransfer;
+    var doCopy = dt.dropEffect == "copy";
+
     var session = this.getSession();
-    // XXX dragAction is not valid, so we also set copy below by checking
-    // whether the dropped item is moveable, before creating the transaction
-    var copy = session.dragAction & Ci.nsIDragService.DRAGDROP_ACTION_COPY;
+    var doCopy = session.dragAction & Ci.nsIDragService.DRAGDROP_ACTION_COPY;
+

doCopy is declared and set twice.

+      var dragginUp = insertionPoint.itemId == unwrapped.parent &&
+                      index < PlacesUtils.bookmarks.getItemIndex(unwrapped.id);
+      if (index != -1 && dragginUp)
+        index += movedCount++;

nit: index+=
 

diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml
--- a/browser/components/places/content/menu.xml
+++ b/browser/components/places/content/menu.xml
@@ -83,18 +83,21 @@
       <method name="onDragOver">
         <parameter name="aEvent"/>
         <parameter name="aFlavour"/>
         <parameter name="aDragSession"/>
         <body><![CDATA[
           PlacesControllerDragHelper.currentDropTarget = aEvent.target;
           // check if we have a valid dropPoint
           var dropPoint = this._getDropPoint(aEvent);
-          if (!dropPoint)
+          if (!dropPoint || !dropPoint.ip ||
+              !PlacesControllerDragHelper.canDrop(dropPoint.ip)) {
+            aEvent.dataTransfer.effectAllowed = "none";     

Isn't this set somewhere in controller.js, I would rather create a helper in placesuiutils than duplicate this logic across multiple files.

(ant nit: trailing spaces)     


@@ -112,57 +115,59 @@
             dropPoint.folderNode.setAttribute("dragover-into", "true");
           }
           else {
             // We are not dragging over a folder
             // Clear out old _overFolder information
             this._overFolder.clear();
           }
 
+          // Autoscroll the popup strip if we drag over the scroll buttons
+          var anonid = aEvent.originalTarget.getAttribute('anonid');
+          var scrollDir = anonid == "scrollbutton-up" ? -1 :
+                          anonid == "scrollbutton-down" ? 1 : 0;
+          if (scrollDir != 0) {
+              this._scrollBox.scrollByIndex(scrollDir);

Check with Neil if there's a cleaner way to do so.


diff --git a/browser/components/places/content/toolbar.xml b/browser/components/places/content/toolbar.xml
--- a/browser/components/places/content/toolbar.xml
+++ b/browser/components/places/content/toolbar.xml



+#ifdef MOZ_WIDGET_GTK2
+      <handler event="mousedown"><![CDATA[
+        // Allow drag and drop of folders in Linux.
+        // We must prevent popupshowing event from firing when shift is pressed.
+        var target = event.originalTarget;
+        if (event.button == 1 && event.shiftKey &&
+            target.localName == "toolbarbutton" && target.type == "menu")
+          this._draggingContainer = true;
+      ]]></handler>
+#endif

I think it might make sense not to #ifdef this handler, so extensions won't just override it.

diff --git a/browser/components/places/content/tree.xml b/browser/components/places/content/tree.xml
--- a/browser/components/places/content/tree.xml
+++ b/browser/components/places/content/tree.xml
@@ -429,21 +430,20 @@
         <getter><![CDATA[
           // invalidated on selection and focus changes
           if (this._cachedInsertionPoint !== undefined)
             return this._cachedInsertionPoint;
 
           // there is no insertion point for history queries
           // so bail out now and save a lot of work when updating commands
           var resultNode = this.getResultNode();
-          if (PlacesUtils.nodeIsQuery(resultNode)) {
-            var options = asQuery(resultNode).queryOptions;
-            if (options.queryType == options.QUERY_TYPE_HISTORY)
-              return this._cachedInsertionPoint = null;
-          }
+          if (PlacesUtils.nodeIsQuery(resultNode) &&
+              asQuery(resultNode).queryOptions.queryType ==
+                Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY)
+              return null;
 
why do you not set this._cachedInsertionPoint?


diff --git a/browser/components/places/content/treeView.js b/browser/components/places/content/treeView.js
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@@ -991,82 +991,61 @@ PlacesTreeView.prototype = {
     return this._result.sortingMode !=
            Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_NONE;
   },
 
   canDrop: function PTV_canDrop(aRow, aOrientation) {
     if (!this._result)
       throw Cr.NS_ERROR_UNEXPECTED;
 
-    var node = aRow != -1 ? this.nodeForTreeIndex(aRow) : this._result.root;
+    // drop position into a sorted treeview would be wrong
+    if (this.isSorted())
+      return false;
 
Didn't we use to drop at the end if the view is sorted?

   drop: function PTV_drop(aRow, aOrientation) {
     // We are responsible for translating the |index| and |orientation| 
     // parameters into a container id and index within the container, 
     // since this information is specific to the tree view.
     var ip = this._getInsertionPoint(aRow, aOrientation);
     if (!ip)
-      throw Cr.NS_ERROR_NOT_AVAILABLE;
+      throw Cr.NS_OK;

Why? That's not something you should throw.

diff --git a/browser/components/places/content/utils.js b/browser/components/places/content/utils.js
--- a/browser/components/places/content/utils.js
+++ b/browser/components/places/content/utils.js
@@ -1030,60 +1033,53 @@ var PlacesUIUtils = {
     }
     element.node = aNode;
     element.node.viewIndex = 0;
 
     return element;
   },
 
   cleanPlacesPopup: function PU_cleanPlacesPopup(aPopup) {
-    // Find static menuitems at the start and at the end of the menupopup,
-    // marked by builder="start" and builder="end" attributes, and set
-    // markers to keep track of their indices.
+    // Remove places popup children and update markers to keep track of
+    // their indices.

+      else {
+        // This is static content...
+        if (!placesNodeFound)
+          // ...at the start of the popup
+          aPopup._startMarker++;

Add a comment here: "initialized in menu.xml, in the base binding"

diff --git a/browser/themes/winstripe/browser/browser-aero.css b/browser/themes/winstripe/browser/browser-aero.css
--- a/browser/themes/winstripe/browser/browser-aero.css
+++ b/browser/themes/winstripe/browser/browser-aero.css
+
+/* Bookmark drag and drop styles */
+.bookmark-item[dragover-into="true"]:-moz-system-metric(windows-default-theme) > .menu-iconic-text {
+  background: transparent !important;
+  color: MenuText !important;
+}

why !important?
Attachment #338753 - Flags: review?(mano) → review-
(In reply to comment #27)
> (From update of attachment 338753 [details] [diff] [review])
> --- a/browser/base/content/browser-places.js
> +++ b/browser/base/content/browser-places.js
> @@ -706,17 +706,16 @@ var BookmarksEventHandler = {
>          target._endMarker = -1;
>        }
>        return;
>      }
> 
>      if (!target._endOptSeparator) {
>        // create a separator before options
>        target._endOptSeparator = document.createElement("menuseparator");
> -      target._endOptSeparator.setAttribute("builder", "end");
> 
> Why are you removing that? I thought you're just removing "start"

Yes, but in this case we were both setting the builder "end" and the endMarker, since the old code was doing wrong endMarker syncing... the new code is able to maintain sync and rebuild works correctly without the need to add the builder.
The builder should be used only in those cases where we define a menu in a binding content and the menu is initially empty, in this case i could not detect where places content ends since there's no content and i cannot set the endMarker.

> @@ -784,17 +783,18 @@ var BookmarksEventHandler = {
>   */
>  var BookmarksMenuDropHandler = {
>    /**
>     * Need to tell the session to update the state of the cursor as we drag
>     * over the Bookmarks Menu to show the "can drop" state vs. the "no drop"
>     * state.
>     */
>    onDragOver: function BMDH_onDragOver(event, flavor, session) {
> -    session.canDrop = this.canDrop(event, session);
> +    if (!this.canDrop(event, session))
> +      event.dataTransfer.effectAllowed = "none";
> 
> Shouldn't we still set canDrop as well?

From what Neil told me, using the new dataTransfer should allow us to drop completely dragSession use. Indeed setting a "none" action allowed does not allow the user to drop and he gets correctly the "drop-not-allowed" icon.

> @@ -807,33 +807,50 @@ var BookmarksMenuDropHandler = {
> 
> +    // Close the menu
> +    if (event.target.lastChild.localName == "menupopup")
> +      event.target.lastChild.hidePopup();
> 
> I don't know if this is desired, ask ux@. I cannot test that.

Yes, i'll ask for it, i'm not sure about it too

> -  getTransferData: function PC_getTransferData(dragAction) {
> -    var copy = dragAction == Ci.nsIDragService.DRAGDROP_ACTION_COPY;
> +  setDataTransfer: function PC_setDataTransfer(aEvent) {
> +    var dt = aEvent.dataTransfer;
> +    var doCopy = dt.effectAllowed == "copyLink";
> +    var doMove = dt.effectAllowed == "move";
> +
> 
> This is confusing, given that you can only do one of them. Keep one variable
> with constants.

mh, i'm not completely sure about this, since when we start dragging, if between our nodes we have some read-only one, we force a copy op... so probably a only "move" allowedEffect would never exists and we can get rid of the filtering code. When we should force a move only operation not allowing the user to copy? When we fill the dataTransfer we still don't know if the user will do a copy or move if both are allowed, so don't makes sense looking for move here. It would be useful if we had some unique object that can only be moved to avoid creating dupes, but that's not our case actually.
I'm removing the filtering code for now, we can discuss of it over IRC though

> diff --git a/browser/components/places/content/menu.xml
> b/browser/components/places/content/menu.xml
> --- a/browser/components/places/content/menu.xml
> +++ b/browser/components/places/content/menu.xml
> @@ -83,18 +83,21 @@
>        <method name="onDragOver">
>          <parameter name="aEvent"/>
>          <parameter name="aFlavour"/>
>          <parameter name="aDragSession"/>
>          <body><![CDATA[
>            PlacesControllerDragHelper.currentDropTarget = aEvent.target;
>            // check if we have a valid dropPoint
>            var dropPoint = this._getDropPoint(aEvent);
> -          if (!dropPoint)
> +          if (!dropPoint || !dropPoint.ip ||
> +              !PlacesControllerDragHelper.canDrop(dropPoint.ip)) {
> +            aEvent.dataTransfer.effectAllowed = "none";     
> 
> Isn't this set somewhere in controller.js, I would rather create a helper in
> placesuiutils than duplicate this logic across multiple files.

Sorry i don't understand the comment, what logic is duplicated? the only place where i'm setting to "none" is onDragOver, and menu, toolbar and tree use slightly different checks before setting it

> +          // Autoscroll the popup strip if we drag over the scroll buttons
> +          var anonid = aEvent.originalTarget.getAttribute('anonid');
> +          var scrollDir = anonid == "scrollbutton-up" ? -1 :
> +                          anonid == "scrollbutton-down" ? 1 : 0;
> +          if (scrollDir != 0) {
> +              this._scrollBox.scrollByIndex(scrollDir);
> 
> Check with Neil if there's a cleaner way to do so.

Will ask, even if i doubt since there's no native drag support in menus

> +#ifdef MOZ_WIDGET_GTK2
> +      <handler event="mousedown"><![CDATA[
> +        // Allow drag and drop of folders in Linux.
> +        // We must prevent popupshowing event from firing when shift is
> pressed.
> +        var target = event.originalTarget;
> +        if (event.button == 1 && event.shiftKey &&
> +            target.localName == "toolbarbutton" && target.type == "menu")
> +          this._draggingContainer = true;
> +      ]]></handler>
> +#endif
> 
> I think it might make sense not to #ifdef this handler, so extensions won't
> just override it.

do you mean ifdef the content rather than the handler, or setting this._draggingContainer for all the platforms but use it only in Linux?
About this, you told me we can't do the same in Windows because if i start drag a folder it should remain open to allow me to drag into it (something that actually does not work though)... but then i thought we don't allow dragging a folder inside itself! So again why not disallowing the popup opening on all the three platforms when holding shift?
As a Windows user i often cannot drag a folder because popup opening sometimes does not allow me to do that. This happens sometimes on Windows, while always on Linux, but it's still annoying since sometimes i have to try again until it works.


>    canDrop: function PTV_canDrop(aRow, aOrientation) {
>      if (!this._result)
>        throw Cr.NS_ERROR_UNEXPECTED;
> 
> -    var node = aRow != -1 ? this.nodeForTreeIndex(aRow) : this._result.root;
> +    // drop position into a sorted treeview would be wrong
> +    if (this.isSorted())
> +      return false;
> 
> Didn't we use to drop at the end if the view is sorted?

i'm not sure it makes sense, maybe in a followup we should try dropping "after_node", the main problem is that the dropFeedback will show they are dropping at a certain position, but the drop will be at the end. I still think we should try to explain users the difference between a sorted view and an unsorted one when they are doing D&D.

>    drop: function PTV_drop(aRow, aOrientation) {
>      // We are responsible for translating the |index| and |orientation| 
>      // parameters into a container id and index within the container, 
>      // since this information is specific to the tree view.
>      var ip = this._getInsertionPoint(aRow, aOrientation);
>      if (!ip)
> -      throw Cr.NS_ERROR_NOT_AVAILABLE;
> +      throw Cr.NS_OK;
> 
> Why? That's not something you should throw.

was a test to see if i could get rid of some errors i was seeing. though throwing error_not_available appear as wrong as ok, what about a return false?
notice we are throwing NS_OK also in tree.xml:
            // Disallow dragging the root node of a tree
            var parent = node.parent;
            if (!parent)
              throw Cr.NS_OK;
also here i think we should simply return false

> diff --git a/browser/themes/winstripe/browser/browser-aero.css
> b/browser/themes/winstripe/browser/browser-aero.css
> --- a/browser/themes/winstripe/browser/browser-aero.css
> +++ b/browser/themes/winstripe/browser/browser-aero.css
> +
> +/* Bookmark drag and drop styles */
> +.bookmark-item[dragover-into="true"]:-moz-system-metric(windows-default-theme)
> > .menu-iconic-text {
> +  background: transparent !important;
> +  color: MenuText !important;
> +}
> 
> why !important?

Because it's needed, we are doing the same in browser.css, if you don't override with !important the style isn't applied. Hwv i want to ask ux team if i can take the change or if they have better fix for the next version.


I fixed locally all of the comments, but the aboves. Waiting for an IRC session to discuss them.
(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 338753 [details] [diff] [review] [details])
> > --- a/browser/base/content/browser-places.js
> > +++ b/browser/base/content/browser-places.js
> > @@ -706,17 +706,16 @@ var BookmarksEventHandler = {
> >          target._endMarker = -1;
> >        }
> >        return;
> >      }
> > 
> >      if (!target._endOptSeparator) {
> >        // create a separator before options
> >        target._endOptSeparator = document.createElement("menuseparator");
> > -      target._endOptSeparator.setAttribute("builder", "end");
> > 
> > Why are you removing that? I thought you're just removing "start"
> 
> Yes, but in this case we were both setting the builder "end" and the endMarker,
> since the old code was doing wrong endMarker syncing... the new code is able to
> maintain sync and rebuild works correctly without the need to add the builder.
> The builder should be used only in those cases where we define a menu in a
> binding content and the menu is initially empty, in this case i could not
> detect where places content ends since there's no content and i cannot set the
> endMarker.

>
ok, got it.

> > @@ -784,17 +783,18 @@ var BookmarksEventHandler = {
> >   */
> >  var BookmarksMenuDropHandler = {
> >    /**
> >     * Need to tell the session to update the state of the cursor as we drag
> >     * over the Bookmarks Menu to show the "can drop" state vs. the "no drop"
> >     * state.
> >     */
> >    onDragOver: function BMDH_onDragOver(event, flavor, session) {
> > -    session.canDrop = this.canDrop(event, session);
> > +    if (!this.canDrop(event, session))
> > +      event.dataTransfer.effectAllowed = "none";
> > 
> > Shouldn't we still set canDrop as well?
> 
> From what Neil told me, using the new dataTransfer should allow us to drop
> completely dragSession use. Indeed setting a "none" action allowed does not
> allow the user to drop and he gets correctly the "drop-not-allowed" icon.
>

I understand that now. 

> > @@ -807,33 +807,50 @@ var BookmarksMenuDropHandler = {
> > 
> > +    // Close the menu
> > +    if (event.target.lastChild.localName == "menupopup")
> > +      event.target.lastChild.hidePopup();
> > 
> > I don't know if this is desired, ask ux@. I cannot test that.
> 
> Yes, i'll ask for it, i'm not sure about it too
>

 
> > -  getTransferData: function PC_getTransferData(dragAction) {
> > -    var copy = dragAction == Ci.nsIDragService.DRAGDROP_ACTION_COPY;
> > +  setDataTransfer: function PC_setDataTransfer(aEvent) {
> > +    var dt = aEvent.dataTransfer;
> > +    var doCopy = dt.effectAllowed == "copyLink";
> > +    var doMove = dt.effectAllowed == "move";
> > +
> > 
> > This is confusing, given that you can only do one of them. Keep one variable
> > with constants.
> 
> mh, i'm not completely sure about this, since when we start dragging, if
> between our nodes we have some read-only one, we force a copy op... so probably
> a only "move" allowedEffect would never exists and we can get rid of the
> filtering code. When we should force a move only operation not allowing the
> user to copy? When we fill the dataTransfer we still don't know if the user
> will do a copy or move if both are allowed, so don't makes sense looking for
> move here. It would be useful if we had some unique object that can only be
> moved to avoid creating dupes, but that's not our case actually.
> I'm removing the filtering code for now, we can discuss of it over IRC though

OK, Please file a bug to figure that out. Basically, we should always get the lowest common denominator. And theoretically, if one node can only be moved and one node can only be copied, you cannot drag at all.

> > diff --git a/browser/components/places/content/menu.xml
> > b/browser/components/places/content/menu.xml
> > --- a/browser/components/places/content/menu.xml
> > +++ b/browser/components/places/content/menu.xml
> > @@ -83,18 +83,21 @@
> >        <method name="onDragOver">
> >          <parameter name="aEvent"/>
> >          <parameter name="aFlavour"/>
> >          <parameter name="aDragSession"/>
> >          <body><![CDATA[
> >            PlacesControllerDragHelper.currentDropTarget = aEvent.target;
> >            // check if we have a valid dropPoint
> >            var dropPoint = this._getDropPoint(aEvent);
> > -          if (!dropPoint)
> > +          if (!dropPoint || !dropPoint.ip ||
> > +              !PlacesControllerDragHelper.canDrop(dropPoint.ip)) {
> > +            aEvent.dataTransfer.effectAllowed = "none";     
> > 
> > Isn't this set somewhere in controller.js, I would rather create a helper in
> > placesuiutils than duplicate this logic across multiple files.
> 
> Sorry i don't understand the comment, what logic is duplicated? the only place
> where i'm setting to "none" is onDragOver, and menu, toolbar and tree use
> slightly different checks before setting it

Setting effectAllow, but ignore that for now.

> > +          // Autoscroll the popup strip if we drag over the scroll buttons
> > +          var anonid = aEvent.originalTarget.getAttribute('anonid');
> > +          var scrollDir = anonid == "scrollbutton-up" ? -1 :
> > +                          anonid == "scrollbutton-down" ? 1 : 0;
> > +          if (scrollDir != 0) {
> > +              this._scrollBox.scrollByIndex(scrollDir);
> > 
> > Check with Neil if there's a cleaner way to do so.
> 
> Will ask, even if i doubt since there's no native drag support in menus

It's not the dragging, it's the way you scroll, _scrollBox is not part of the menu pseudo API.

 
> > +#ifdef MOZ_WIDGET_GTK2
> > +      <handler event="mousedown"><![CDATA[
> > +        // Allow drag and drop of folders in Linux.
> > +        // We must prevent popupshowing event from firing when shift is
> > pressed.
> > +        var target = event.originalTarget;
> > +        if (event.button == 1 && event.shiftKey &&
> > +            target.localName == "toolbarbutton" && target.type == "menu")
> > +          this._draggingContainer = true;
> > +      ]]></handler>
> > +#endif
> > 
> > I think it might make sense not to #ifdef this handler, so extensions won't
> > just override it.
> 
> do you mean ifdef the content rather than the handler, or setting
> this._draggingContainer for all the platforms but use it only in Linux?

the latter, for now.
> 
> >    canDrop: function PTV_canDrop(aRow, aOrientation) {
> >      if (!this._result)
> >        throw Cr.NS_ERROR_UNEXPECTED;
> > 
> > -    var node = aRow != -1 ? this.nodeForTreeIndex(aRow) : this._result.root;
> > +    // drop position into a sorted treeview would be wrong
> > +    if (this.isSorted())
> > +      return false;
> > 
> > Didn't we use to drop at the end if the view is sorted?
> 
> i'm not sure it makes sense, maybe in a followup we should try dropping
> "after_node", the main problem is that the dropFeedback will show they are
> dropping at a certain position, but the drop will be at the end. I still think
> we should try to explain users the difference between a sorted view and an
> unsorted one when they are doing D&D.
>

I can do that in iTunes, but it doesn't show the drag feeedback if the view is sorted. Can we do that?
 
> >    drop: function PTV_drop(aRow, aOrientation) {
> >      // We are responsible for translating the |index| and |orientation| 
> >      // parameters into a container id and index within the container, 
> >      // since this information is specific to the tree view.
> >      var ip = this._getInsertionPoint(aRow, aOrientation);
> >      if (!ip)
> > -      throw Cr.NS_ERROR_NOT_AVAILABLE;
> > +      throw Cr.NS_OK;
> > 
> > Why? That's not something you should throw.
> 
> was a test to see if i could get rid of some errors i was seeing. though
> throwing error_not_available appear as wrong as ok, what about a return false?
> notice we are throwing NS_OK also in tree.xml:
>             // Disallow dragging the root node of a tree
>             var parent = node.parent;
>             if (!parent)
>               throw Cr.NS_OK;
> also here i think we should simply return false
>

You mean just |return|, isn't drop a void function?
(In reply to comment #29)
> > > @@ -807,33 +807,50 @@ var BookmarksMenuDropHandler = {
> > > 
> > > +    // Close the menu
> > > +    if (event.target.lastChild.localName == "menupopup")
> > > +      event.target.lastChild.hidePopup();
> > > 
> > > I don't know if this is desired, ask ux@. I cannot test that.
> > 
> > Yes, i'll ask for it, i'm not sure about it too
> >

Removed for now, we already have a bug filled on that, will handle there after having an answer from ux team (hwv i agree with you we should let the menu open)


> > > -  getTransferData: function PC_getTransferData(dragAction) {
> > > -    var copy = dragAction == Ci.nsIDragService.DRAGDROP_ACTION_COPY;
> > > +  setDataTransfer: function PC_setDataTransfer(aEvent) {
> > > +    var dt = aEvent.dataTransfer;
> > > +    var doCopy = dt.effectAllowed == "copyLink";
> > > +    var doMove = dt.effectAllowed == "move";
> > > +
> > > 
> > > This is confusing, given that you can only do one of them. Keep one variable
> > > with constants.
> > 
> > mh, i'm not completely sure about this, since when we start dragging, if
> > between our nodes we have some read-only one, we force a copy op... so probably
> > a only "move" allowedEffect would never exists and we can get rid of the
> > filtering code. When we should force a move only operation not allowing the
> > user to copy? When we fill the dataTransfer we still don't know if the user
> > will do a copy or move if both are allowed, so don't makes sense looking for
> > move here. It would be useful if we had some unique object that can only be
> > moved to avoid creating dupes, but that's not our case actually.
> > I'm removing the filtering code for now, we can discuss of it over IRC though
> 
> OK, Please file a bug to figure that out. Basically, we should always get the
> lowest common denominator. And theoretically, if one node can only be moved and
> one node can only be copied, you cannot drag at all.

will fill a followup as soon as this is checked-in


> > > +          // Autoscroll the popup strip if we drag over the scroll buttons
> > > +          var anonid = aEvent.originalTarget.getAttribute('anonid');
> > > +          var scrollDir = anonid == "scrollbutton-up" ? -1 :
> > > +                          anonid == "scrollbutton-down" ? 1 : 0;
> > > +          if (scrollDir != 0) {
> > > +              this._scrollBox.scrollByIndex(scrollDir);
> > > 
> > > Check with Neil if there's a cleaner way to do so.
> > 
> > Will ask, even if i doubt since there's no native drag support in menus
> 
> It's not the dragging, it's the way you scroll, _scrollBox is not part of the
> menu pseudo API.

Neil told me we could probably add drag events to nsAutoRepeatBoxFrame::HandleEvent, so i'll fill a followup, the actual code is not beatiful but it's working until we will do that.


> > > +#ifdef MOZ_WIDGET_GTK2
> > > +      <handler event="mousedown"><![CDATA[
> > > +        // Allow drag and drop of folders in Linux.
> > > +        // We must prevent popupshowing event from firing when shift is
> > > pressed.
> > > +        var target = event.originalTarget;
> > > +        if (event.button == 1 && event.shiftKey &&
> > > +            target.localName == "toolbarbutton" && target.type == "menu")
> > > +          this._draggingContainer = true;
> > > +      ]]></handler>
> > > +#endif
> > > 
> > > I think it might make sense not to #ifdef this handler, so extensions won't
> > > just override it.
> > 
> > do you mean ifdef the content rather than the handler, or setting
> > this._draggingContainer for all the platforms but use it only in Linux?
> 
> the latter, for now.

so i'm setting and unsetting _draggingContainer for all platforms, but using only in Linux, will fill a followup to investigate activation of that on all platforms, i still think it would greatly help on Windows too.

> > >    canDrop: function PTV_canDrop(aRow, aOrientation) {
> > >      if (!this._result)
> > >        throw Cr.NS_ERROR_UNEXPECTED;
> > > 
> > > -    var node = aRow != -1 ? this.nodeForTreeIndex(aRow) : this._result.root;
> > > +    // drop position into a sorted treeview would be wrong
> > > +    if (this.isSorted())
> > > +      return false;
> > > 
> > > Didn't we use to drop at the end if the view is sorted?
> > 
> > i'm not sure it makes sense, maybe in a followup we should try dropping
> > "after_node", the main problem is that the dropFeedback will show they are
> > dropping at a certain position, but the drop will be at the end. I still think
> > we should try to explain users the difference between a sorted view and an
> > unsorted one when they are doing D&D.
> >
> 
> I can do that in iTunes, but it doesn't show the drag feeedback if the view is
> sorted. Can we do that?

the Drag feedback is set and invalidated in nsTreeBody, we could try hacking it in some way, but if the drag is allowed it will be showed... leaving drop disabled for now on sorted views, will fill a follow-up to investigate possible solutions or eventually re-enable old behaviour.
Attached patch patch 1.4 — — Splinter Review 
in this version of the patch, apart fixing remaining comments i've removed the move hover style hack (setting background to menu-iconic-text to use the "_moz-menuactive" attribute onDragOver... This makes menu.xml styling correct on both XP and Vista.
Attachment #338753 - Attachment is obsolete: true
Attachment #339358 - Flags: review?(mano)
Comment on attachment 339358 [details] [diff] [review]
patch 1.4

r=mano with follow ups filed
Attachment #339358 - Flags: review?(mano) → review+
filled: 
Bug 455957 - dragging multiple items should check if we are forcing both copy and move
Bug 455958 - clean up menupopups scrolling onDragOver
Bug 455959 - investigate using _draggingContainer on all platforms when dragging folders on bookmarks toolbar
Bug 455960 - define drop behaviour on sorted TreeViews
Blocks: 446221
Blocks: 444874
Blocks: 384095
Blocks: 389914
Blocks: 441470
Blocks: 429495
Blocks: 435845
Blocks: 431699
Blocks: 431870
Blocks: 395176
Blocks: 395173
Keywords: qawanted
Whiteboard: [needs help testing][needs review mano]
http://hg.mozilla.org/mozilla-central/rev/45e42cafab3f
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This change appears to have caused orange:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_423515.js | Exception thrown - TypeError: PlacesControllerDragHelper.canMoveContainerNode is not a function

I'm planning on doing a backout, so contact me on IRC if you have a trivial fix you can push.
contacted on IRC, the test update wasout of the patch, the test needs to be updated
Benjamin Smedberg has done a s/canMoveContainerNode/canMoveNode on browser_423515.js, that was needed but not enough, i pushed this as a post-facto to solve the failing mochitest, it's restoring previous code
Attachment #339522 - Flags: review?(mano)
Blocks: 420449
Blocks: 456805
Depends on: 457498
Target Milestone: Firefox 3.1 → Firefox 3.1b1
Blocks: 466340
Attachment #339522 - Flags: review?(mano)
Depends on: 470773
Blocks: 422612
Now that bug 455590 is fixed, we should be able to clean this up a bit more, as the dataTransfer is now passed directly to the tree view methods canDrop and drop, so it doesn't need to be cached.
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
Depends on: 741065
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: