389290 - Bookmarks Menu - dropmarker missing

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P3)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072310 Minefield/3.0a7pre ID:2007072310

repro:
open FF
open bookmarks menu
drag a bookmark from one to another place in the menu

result:
no dropmarker (horizontal line)

regressionrange:
works in 20070714_0800_firefox-3.0a7pre.en-US.win32
fails in 20070714_0836_firefox-3.0a7pre.en-US.win32

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=1184425200&maxdate=1184427359&cvsroot=%2Fcvsroot

-> Bug 337771

Comment 1

[Peter van der Woude [:Peter6]]

Bug 388209 (which regressed before this one) isn't visable because of this one so can't say if that one is still a problem or not.

Comment 2

[Simon Bünzli]

Attachment: border-/margin-hack

So, with native styling we can no longer separately style the borders we used to use as a cheap drop-marker replacement. This leaves several alternatives:

1. Make native themeing understand our poor man's drop-markers.
2. Move to real drop-markers inside the menupopup binding.
3. Use the next best thing to the borders we've used so far and hope that we can get away without having to resort to 1. or 2. this time.

This patch goes for option 3. It ain't really pretty and I don't see much room for further polish, but it might still be good enough for now. If it isn't, somebody with more cycles left will have to figure out a better solution...

Comment 3

[James Ross]

There is another option, which I feel is way more elegant:
  Make native themed widget border overriding optional.

Currently, a number other methods (e.g. GetWidgetPadding) return PRBool to indicate if they have specified an override or not. GetWidgetBorder is not one, but it is not hard to update the code so it is - and it provides more consistency in the nsITheme API to boot.

Unfortunately, your patch for other bits has added native 'borders' to menu items (entirely unneeded, they're only padding it). Make the native code specify padding, if anything, and have the drop markers in CSS.

Comment 4

[Simon Bünzli]

Now that explains the API change in your patch to bug 337771 which never made sense to me... oh, well. I'm all for more consistent APIs, but in this case your more elegant proposal doesn't really solve the underlying issue: that borders (part of a box) aren't really drop-markers (elements on their own) at all.

IMO the most elegant - because correct - solution would be to extend popup.xml with #popup-droptarget and use either a stack or a zero-width box with margin-magic to allow themes to display as drop-marker whatever they please (option 2 above).

I guess we'd still take a patch as you propose if there's no patch before beta 1 to implement a proper drop-marker, though.

Comment 5

[Simon Bünzli]

Attachment: popup.xml#popup-droptarget

Alright, option #2 would have to look about like this:
(i) we extend popup.xml#popup to act as a drop-handler which moves an invisible drop indicator and (as a bonus) even scrolls the popup in the overflow case;
(ii) Places' menu.xml#places-menupopup extends the new binding and gives it a few hints about when not to show the drop indicator (when the bookmark is dropped into a subfolder);
(iii) Winstripe gives the drop indicator an appearance (it remains zero-width for the other themes) which for lack of a better alternative currently looks like a chevron.

TBD:
1. Settle on a more proper look for the drop indicator, either an arrow (RTL-aware) or a dash as used in the bookmarks sidebar
2. Test the patch non-Windows platforms (can't do that myself)
3. Determine whether non-Windows platforms should take full advantage of the new binding as well (needed for platform parity)
4. Take care of SeaMonkey in a follow up bug: adapt bookmarksToolbar.css to use the new binding for the bookmarks menu and bookmarksMenu.js to configure it as Places' menu.xml does.

Comment 6

[Simon Bünzli]

Attachment: popup.xml#popup-droptarget, v2

Fixed a few issues with the drop-marker not being drawn at the correct place and with it sliding towards the correct position instead of directly appearing there.

Comment 7

[Simon Bünzli]

Attachment: popup.xml#popup-droptarget, v3

Adapted some tricks from SeaMonkey's tabbrowser.xml to simplify the code. Furthermore the drop indicator is no longer drawn for non-bookmark items and it uses a horizontal bar instead of the chevron so that it should also be RTL-save.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 275321 [details] [diff] [review]
popup.xml#popup-droptarget, v3

regressions don't need ui-rs :)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

The patch seems to have bitrotten slightly, though I still managed to apply it. I noticed you're not removing a bunch of the dragover styles in browser.css, and the attribute setting in menu.xml. Is that because it's still being used on the other platforms? If we go this route I think we should get rid of those entirely and just use this binding cross-platform.

CCing Neil and Mano for their thoughts.

Comment 10

[Simon Bünzli]

I won't be able to make this patch cross-platform. It's mostly there, though, and shouldn't need much further care. What's left TBD:
* Consider making the interaction with the binding more API like: mDropFlavours and _hideDropIndicator should probably be accessible as non-private properties.
* Copy the changes to Winstripe's popup.css over to the other themes (and adapt them if required).
* Remove all code related to dragover-top and dragover-bottom (note that dragover-into will still be required).
* File a SeaMonkey bug for adapting their bookmarksToolbar.css to use
the new binding for the bookmarks menu and bookmarksMenu.js to configure it as
Places does.

Comment 11

[Simon Bünzli]

Attachment: unbitrotted, cross-platform (hopefully)

This patch needs quite some testing on Mac and Linux and feedback on how to appropriately style the drop-marker (currently it's a blue horizontal bar - quite similar to the drop-marker in the Bookmarks/History sidebar). Please give it a go!

(In reply to comment #9)
> I noticed you're not removing a bunch of the dragover styles in browser.css,
> and the attribute setting in menu.xml.

The additional dragover styles in browser.css are now gone (they seem to have been left-overs from a long time ago). Setting/removing the attributes might still be wanted by themers though, so I won't touch these without feedback from the experts.

BTW: There's a off-by-one (or two) error in the Places code which makes testing this patch slightly tricky; dragging a bookmark over the Bookmarks top menu item still throws; and subfolders don't automatically open. Not sure whether these issues aren't being fixed because of the missing drop-marker itself...

Comment 12

[Marco Bonardo [:mak]]

> BTW: There's a off-by-one (or two) error in the Places code which makes testing
> this patch slightly tricky;

bookmark menu/toolbar offset is Bug 405087

Comment 13

[Marco Bonardo [:mak]]

if dragging in submenus the dropmarker between items does not appear (while it works on folders)

Comment 14

[Simon Bünzli]

Comment on attachment 292097 [details] [diff] [review]
unbitrotted, cross-platform (hopefully)

This patch has bitrotted again. I've currently got no cycles left for taking care of that, though.

Comment 15

[Marco Bonardo [:mak]]

Attachment: a slightly different approach

this tries to use the current styles (stealing some idea from simon patch), without changing much code, but sounds a little hackish (see border usage on .menu-iconic-text). But it works fine on all menus and toolbar, ideas?

notice that now the offset on the bookmark menu should be gone (there is still an offset in toolbar chevron popup though)

Comment 16

[Marco Bonardo [:mak]]

Attachment: (screenshot) how this appear on windows

Comment 17

[Mike Connor [:mconnor]]

Not going to continue to block on this.  If you disagree with this decision, please renominate with reasons why we can't ship with this in final

Comment 18

[mmortal03]

(In reply to comment #17)
> Not going to continue to block on this.  If you disagree with this decision,
> please renominate with reasons why we can't ship with this in final
> 

Because it was fully functional in 2.0 and because it is a basic UI design requirement.  It looks really bad to regress something like this between final version implementations.  It is almost a requirement when moving bookmarks around that you can see where they are going to be dropped.  Otherwise, the UI feels very disconcerting.  You can still do it, but it is very unprofessional, and takes away some speed and accuracy from the user.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

This has a patch that just needs updating, I think we should block on it.

Comment 20

[Daniel.S]

This is alo in IE (at least v7) and could confuse people that switch from IE.

I personally think it's difficult to work in the menu without this.

Comment 22

[Andreas Goetz]

+1 for blocking- the current behaviour is below par with any comparable product. Worse than not being able to drop into subfolders...

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

I was going to say that the drag-ghosting makes things passable, but that coupled with the hard to see off-by-whatever errors makes this a little more important to fix.

Comment 24

[Marco Bonardo [:mak]]

taking!
i'm unbitrotting Simon's patch (sorry if i did not do that before but i had to learn some xbl magic first).

talked with Mano about this, and i'm moving the binding out of toolkit as a places only binding, since it's quite late to add a new binding in toolkit.

Comment 25

[Marco Bonardo [:mak]]

Attachment: patch

this is mostly Simon Bunzli's patch, i've moved the binding to places menu.xml, cleaned up a little and applied it to single places menupopups. This lives out of toolkit, so probably we should move this bug into places and create a new one to ask a toolkit popup-droptarget (or viceversa). This fixes also Bug 405969 and Bug 388209 for places menus (bookmark menu and toolbar folders menus).
Thanks to Mano for his hints!

Comment 27

[Asaf Romano (gone)]

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

>Index: browser/components/places/content/menu.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/menu.xml,v
>retrieving revision 1.99
>diff -u -8 -p -r1.99 menu.xml
>--- browser/components/places/content/menu.xml	6 Feb 2008 21:05:23 -0000	1.99
>+++ browser/components/places/content/menu.xml	14 Feb 2008 09:07:17 -0000
>@@ -40,17 +40,17 @@
>
credits? :)

> <bindings id="placesMenuBindings"
>           xmlns="http://www.mozilla.org/xbl"
>           xmlns:xbl="http://www.mozilla.org/xbl"
>           xmlns:html="http://www.w3.org/1999/xhtml"
>           xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
>   <binding id="places-menupopup"
>-           extends="chrome://global/content/bindings/popup.xml#popup">
>+           extends="chrome://browser/content/places/menu.xml#popup-droptarget">

please revert the bindings order.

>     <implementation>
>       <destructor><![CDATA[
>         this._resultNode = null;
>         if (this._result) {
>           this._result.viewer = null;
>           this._result = null;
>         }
>       ]]></destructor>
>@@ -605,9 +605,110 @@
>         // |popupNode == menupopup| happens when the area between
>         // menuseparators is clicked.
>         this._selection = popupNode.node || popupNode.parentNode._resultNode;
>         this._cachedInsertionPoint = undefined;
>       ]]></handler>
>     </handlers>
>   </binding>
> 
>+  <!-- requires nsDragAndDrop.js -->

you don't need that comment here, it's required for any places front-end usage.

>+  <binding id="popup-droptarget"
>+           extends="chrome://global/content/bindings/popup.xml#popup">
>+    <content>
>+      <xul:hbox flex="1">
>+        <xul:vbox class="menupopup-drop-indicator-bar" hidden="true">
>+          <xul:image class="menupopup-drop-indicator" mousethrough="always"/>
>+        </xul:vbox>
>+        <xul:arrowscrollbox class="popup-internal-box" flex="1" orient="vertical">
>+          <children/>
>+        </xul:arrowscrollbox>
>+      </xul:hbox>
>+    </content>
>+
>+    <implementation>
>+
>+      <field name="acceptedDropFlavours">PlacesUtils.GENERIC_VIEW_DROP_TYPES</field>
>+
>+      <method name="onDragOver">
>+        <parameter name="aEvent"/>
>+        <parameter name="aFlavour"/>
>+        <parameter name="aDragSession"/>
>+        <body><![CDATA[
>+          var indicatorBar = document.getAnonymousElementByAttribute(this, "class",
>+                                                                     "menupopup-drop-indicator-bar");

This ought to be cached, onDragOver is called very often during drag operations...

>+          if (!aDragSession.canDrop || this.hideDropIndicator(aEvent)) {
>+            indicatorBar.hidden = true;
>+            return;
>+          }
>+
>+          var scrollBox = document.getAnonymousElementByAttribute(this, "class",
>+                                                                  "popup-internal-box");

ditto.




>+          // 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)
>+            scrollBox.scrollByIndex(scrollDir);
>+
>+          var newMarginTop = 0;
>+          if (scrollDir == 0) {
>+            var node = this.firstChild;
>+            while (node && aEvent.screenY > node.boxObject.screenY + node.boxObject.height / 2)
>+              node = node.nextSibling;
>+            newMarginTop = node ? node.boxObject.screenY - scrollBoxObject.screenY :
>+                                  scrollBoxObject.height;
>+          }
>+          else if (scrollDir == 1) {
>+            newMarginTop = scrollBoxObject.height;
>+          }
>+

nit: remove the braces.


>+      <method name="onDragExit">
>+        <parameter name="aEvent"/>
>+        <parameter name="aDragSession"/>
>+        <body><![CDATA[
>+          var target = aEvent.relatedTarget;
>+          if (!target) {

can you document this check?



>Index: browser/components/places/content/utils.js
>===================================================================
>@@ -1790,25 +1790,27 @@ var PlacesUtils = {
>         element.className = "menuitem-iconic bookmark-item";
>       }
>       else if (this.containerTypes.indexOf(type) != -1) {
>         element = document.createElement("menu");
>         element.setAttribute("container", "true");
> 
>         if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY)
>           element.setAttribute("query", "true");
>-	else if (aNode.itemId != -1) {
>+        else if (aNode.itemId != -1) {
>           if (this.nodeIsLivemarkContainer(aNode))
>             element.setAttribute("livemark", "true");
>           else if (this.bookmarks
>                      .getFolderIdForItem(aNode.itemId) == this.tagsFolderId)
>             element.setAttribute("tagContainer", "true");
>         }
> 
>         var popup = document.createElement("menupopup");
>+        popup.setAttribute("style",
>+                           "-moz-binding: url('chrome://browser/content/places/menu.xml#popup-droptarget')");

I would rather use an attribute + corresponding style rule (in places.css, the one in content).

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #27)
> > 
> >   <binding id="places-menupopup"
> >-           extends="chrome://global/content/bindings/popup.xml#popup">
> >+           extends="chrome://browser/content/places/menu.xml#popup-droptarget">
> 
> please revert the bindings order.

do you mean make places-menupopup extend popup and popup-droptarget extend places-menupopup?

Comment 29

[Bill Gianopoulos [:WG9s]]

(In reply to comment #25)
> Created an attachment (id=303231) [details]
> patch

With this patch installed there are still issues under Linux.  Trying to drag a bookmark past a separator in the Bookmarks menu seems to result in the bookmarks menu being dismissed.

Comment 30

[Bill Gianopoulos [:WG9s]]

(In reply to comment #29)
> (In reply to comment #25)
> > Created an attachment (id=303231) [details] [details]
> > patch
> 
> With this patch installed there are still issues under Linux.  Trying to drag a
> bookmark past a separator in the Bookmarks menu seems to result in the
> bookmarks menu being dismissed.
> 
Ignore this comment.  I think this may be caused by interaction with code from another bug that I have in my tree.

Comment 31

[Asaf Romano (gone)]

(In reply to comment #28)
> (In reply to comment #27)
> > > 
> > >   <binding id="places-menupopup"
> > >-           extends="chrome://global/content/bindings/popup.xml#popup">
> > >+           extends="chrome://browser/content/places/menu.xml#popup-droptarget">
> > 
> > please revert the bindings order.
> 
> do you mean make places-menupopup extend popup and popup-droptarget extend
> places-menupopup?
> 

no, just the order of bindings in the file.

Comment 32

[Marco Bonardo [:mak]]

Attachment: patch

Addressed comments, cleaned up controller.js (it was still setting no more used styles)

Comment 33

[Simon Bünzli]

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

> #   Asaf Romano <[e-mail]>
>+#   Marco Bonardo <[e-mail]>
> #

All yours? ;-)

Comment 34

[Marco Bonardo [:mak]]

no, really  wanted to put simon's name there but i don't know what email use, is [e-mail] correct?

Comment 35

[Simon Bünzli]

(In reply to comment #34)
>+#   Simon Bünzli <[e-mail]>

Thanks.

Comment 36

[Marco Bonardo [:mak]]

Attachment: patch

i've found your mail in other sources, fixed credits :)

Comment 37

[Simon Bünzli]

Beware the encoding (UTF-8): so "ü" instead of "ü".

Comment 38

[Marco Bonardo [:mak]]

Attachment: patch

those 2 dots

Comment 39

[Asaf Romano (gone)]

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

>Index: browser/components/places/content/menu.xml
>===================================================================

>-  <binding id="places-menupopup"
>+  <binding id="popup-droptarget"
>            extends="chrome://global/content/bindings/popup.xml#popup">
>+    <content>
>+      <xul:hbox flex="1">
>+        <xul:vbox class="menupopup-drop-indicator-bar" hidden="true">
>+          <xul:image class="menupopup-drop-indicator" mousethrough="always"/>
>+        </xul:vbox>
>+        <xul:arrowscrollbox class="popup-internal-box" flex="1" orient="vertical">
>+          <children/>
>+        </xul:arrowscrollbox>
>+      </xul:hbox>
>+    </content>
>+
>+    <implementation>
>+
>+      <field name="acceptedDropFlavours">PlacesUtils.GENERIC_VIEW_DROP_TYPES</field>
>+

Just inline it in the one place it's used.

>+      <field name="_indicatorBar">null</field>
>+      <field name="_scrollBox">null</field>
>+
>+      <method name="onDragOver">
>+        <parameter name="aEvent"/>
>+        <parameter name="aFlavour"/>
>+        <parameter name="aDragSession"/>
>+        <body><![CDATA[
>+          if (!this._indicatorBar)
>+            this._indicatorBar = document.getAnonymousElementByAttribute(this,
>+                                             "class", "menupopup-drop-indicator-bar");

this can simply be: <field name="_indicatorBar"> document.getAnonymousElementByAttribute(this, "class", "menupopup-drop-indicator-bar");</field>

ditto for the scrollbox field.

>+      <method name="onDragExit">
>+        <parameter name="aEvent"/>
>+        <parameter name="aDragSession"/>
>+        <body><![CDATA[
>+          // if we have not moved to a valid new target clear the drop indicator
>+          // this happens when moving out of the popup
>+          var target = aEvent.relatedTarget;
>+          if (!target && this._indicatorBar)

What's the this._indicatorBar check for?

>+      <method name="hideDropIndicator">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          // hide the dropmarker if current node is not a places bookmark item
>+          var target = aEvent.target;
>+          var dropPoint = this._DNDObserver._getDropPoint(aEvent);
>+          return dropPoint == null || dropPoint.folderNode || target == null ||

Ugh, any chance you could merge PlacesMenuDNDObserver into this binding?

>+                 (target.localName != "menuseparator" && !/bookmark-item/.test(target.className)) ||

I don't like this, how about a target.node check?

Comment 40

[Marco Bonardo [:mak]]

(In reply to comment #39)
> Ugh, any chance you could merge PlacesMenuDNDObserver into this binding?

i tried that yesterday, since now we have a binding for D&D i thought that would be a fine idea to merge the DNDObserver into it. But DNDObserver uses some features from the places-menupopup binding, so i should invert the binding (popup-droptarget should extend places-menupopup) and make all items bind to places-menupopup. Pitifully doing that makes the dropmarker be binded to the main bookmarks menu, and does not appear on submenus, i still have to do some test, probably i'm missing something from the full menu views management, will come with a fixed patch for other comments, and will still try to merge things in as a side patch.

Comment 41

[Marco Bonardo [:mak]]

s/and make all items bind to places-menupopup/and make all items bind to popup-droptarget

Comment 42

[Asaf Romano (gone)]

I don't get that, the dnd observer gets references to both the popup and the root popup (which is the view), there's no need to change that, i.e. you could keep a reference within a droptarget element to its root view.

Comment 43

[Asaf Romano (gone)]

And that also means that even if you did that (invert the binding order), you wouldn't get the correct results, there'd still be only one view per the entire bookmarks menu hierarchy, with which the dnd call should interact. 

Comment 44

[Marco Bonardo [:mak]]

thank you for hints, i have checked again my work from yesterday, and with some change based on your comments i got it working (woooa! that work was not totally useless :)). I still need to do some cleanup to remove duplicated code but actually it's quite working and cleaner.

Comment 45

[Marco Bonardo [:mak]]

Attachment: WIP

on the road for merging, still have to debug overfolder code and i have some problem with Bug 415390.

Comment 46

[Marco Bonardo [:mak]]

Attachment: patch with merge

this is the first reviewable version after the merge.

i've tested most functionality using Mats's patch for thread manager and disabling  onDragExit in browser-places.js (it is really bogus, closing the popups when it should not!). With these i tested dragging between locationbar, bookmarks menu, bookmarks toolbar, content.

The dropmarker is working really fine, the overFolder code is quite working (i think that there *could* still be there some really minor bug when closing popups), the code size and readability is a win.

I hope we can put this in before beta4.

Comment 47

[Asaf Romano (gone)]

Comment on attachment 304479 [details] [diff] [review]
patch with merge

>Index: browser/components/places/content/menu.xml
>===================================================================
> >-  <binding id="places-menupopup"
>+  <binding id="popup-droptarget"
>            extends="chrome://global/content/bindings/popup.xml#popup">

Let's rename this binding to something like "places-popup-base".

>+      <field name="_indicatorBar">
>+        document.getAnonymousElementByAttribute(this, "class",
>+                                                "menupopup-drop-indicator-bar");
>+      </field>
>+
>+      <field name="_scrollBox">
>+        document.getAnonymousElementByAttribute(this, "class",
>+                                                "popup-internal-box");
>+      </field>
>+
>+      <!-- This is the view that manage the popup -->
>+      <field name="_rootView">PlacesUtils.getViewForNode(this);</field>
>+

Please move the startMarker and endMarker fields (set to -1) to this binding.

>+      <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 == null)
>+            return;
>+

Prefer !dropPoint.

>+          }
>+          else {
>+            // We are not dragging over a folder
>+            // Clear out old _overFolder information
>+            this._overFolder.clear();
>+          }
>+
>+          // Check if we should hide the drop indicator for this target
>+          if (!aDragSession.canDrop ||
>+              dropPoint == null || dropPoint.folderNode ||

!dropPoint

>+      <method name="onDragExit">
>+        <parameter name="aEvent"/>
>+        <parameter name="aDragSession"/>
>+        <body><![CDATA[
>+          PlacesControllerDragHelper.currentDropTarget = null;
>+          this.removeAttribute("dragover");
>+          // remove dragover-into style from previous target
>+          aEvent.target.removeAttribute("dragover-into");
>+
>+          // if we have not moved to a valid new target clear the drop indicator
>+          // this happens when moving out of the popup
>+          var target = aEvent.relatedTarget;
>+          if (!target)
>+            this._indicatorBar.hidden = true;
>+
>+          // Close any folder being hovered over
>+          if (this._overFolder.node)
>+            this._overFolder.closeTimer = this._overFolder
>+                                              .setTimer(this._overFolder.hoverTime);
>+

For readability, please add braces around this block

>+          // The autoopened attribute is set when this folder was automatically
>+          // opened after the user dragged over it.  If this attribute is set,
>+          // auto-close the folder on drag exit.
>+          if (this.hasAttribute("autoopened"))
>+            this._overFolder.closeMenuTimer = this._overFolder
>+                                                  .setTimer(this._overFolder.hoverTime);

Ditto.

>+        ]]></body>
>+      </method>
>+

>+      <!-- 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;

Either here or in a follow up, we should cache these once in placesutils.

>+        ]]></body>
>+      </method>
>+
>+      <!-- Check if we should hide the drop indicator for the target -->
>+      <method name="_hideDropIndicator">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          var target = aEvent.target;
>+
>+          // in some view we have _startMarker and _endMarker, we should not
>+          // draw the drop indicator outside of them
>+          var betweenMarkers = true;
>+          if (this._startMarker > 0 &&

I think this should be >= 0 or != -1 (after moving the fields).

>+              target.boxObject.y < this.childNodes[this._startMarker].boxObject.y)
>+            betweenMarkers = false;
>+          if (this._endMarker > 0 &&

Ditto.

>+              target.boxObject.y > this.childNodes[this._endMarker].boxObject.y)
>+            betweenMarkers = false;
>+
>+          // hide the dropmarker if current node is not a places bookmark item
>+          return target == null || !betweenMarkers || !this.canDrop();

Make that !(target && betweenMarkers && this.canDrop())

>+                else if (aEvent.layerY < nodeY + (nodeHeight * 0.75)) {
>+                  // Drop inside this folder.
>+                  dropPoint.ip = new InsertionPoint(xulNode.node.itemId, -1, 1);
>+                  dropPoint.beforeIndex = i;
>+                  dropPoint.folderNode = xulNode;
>+                  return dropPoint;
>+                }
>+              } else {

}
else


>+      <!-- Sub-menus should be opened when the mouse drags over them, and closed
>+           when the mouse drags off.  The overFolder object manages opening and
>+           closing of folders when the mouse hovers. -->
>+      <field name="_overFolder"><![CDATA[({
>+        _self: this,
>+        _folder: {node: null, openTimer: null, hoverTime: 350, closeTimer: null},
>+        _closeMenuTimer: null,
>+
>+        get node() {
>+          return this._folder.node;
>+        },
>+        set node(val) {
>+          this._folder.node = val;
>+        },
>+

Setters should return val, i.e. |return this._folder.node = val;|

>+        set openTimer(val) {
>+          this._folder.openTimer = val;
>+        },
>+

>+        set hoverTime(val) {
>+          this._folder.hoverTime = val;
>+        },

>+        set closeTimer(val) {
>+          this._folder.closeTimer = val;
>+        },
>+

>+        set closeMenuTimer(val) {
>+          this._closeMenuTimer = val;
>+        },

Ditto

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

>@@ -1227,292 +1228,16 @@ function PlacesMenuDNDObserver(aView, aP
>   this._popup = aPopup;
>   this._popup.addEventListener("draggesture", this, false);
>   this._popup.addEventListener("dragover", this, false);
>   this._popup.addEventListener("dragdrop", this, false);
>   this._popup.addEventListener("dragexit", this, false);
> }
> 
> /**
>- * XXXmano-please-rewrite-me: This code was ported over from menu.xul in bug 399729.
>- * Unsurprisngly it's still mostly broken due to bug 337761, thus I didn't bother
>- * trying to cleaning up this extremely buggy  over-folder detection code yet.
>- */
>-PlacesMenuDNDObserver.prototype = {
  
The factory for it should be removed too.

r=mano otherwise.

Comment 48

[Marco Bonardo [:mak]]

Attachment: for check-in

bringing over Mano's review

apart from review comments i fixed a PlacesMenuDNDObserver call into toolbar.xml (it became visible after removing the factory).

Comment 49

[Marco Bonardo [:mak]]

> >+      <!-- 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;
> 
> Either here or in a follow up, we should cache these once in placesutils.

spunned-off Bug 418671

Comment 50

[Marco Bonardo [:mak]]

Attachment: for check-in

fixed attribute name and other comments based on suggestion from Mano on IRC

Comment 51

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.204
mozilla/browser/components/places/content/menu.xml 1.101
mozilla/browser/components/places/content/places.css 1.12
mozilla/browser/components/places/content/toolbar.xml, 1.123
mozilla/browser/components/places/content/utils.js 1.102
mozilla/browser/themes/gnomestripe/browser/browser.css 1.180
mozilla/browser/themes/pinstripe/browser/browser.css 1.124
mozilla/browser/themes/winstripe/browser/browser.css 1.173

Comment 52

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre] (nightly) (W2Ksp4)

V.Fixed.

Comment 53

[Andreas Goetz]

Ahh- now this is nice. Further improvements I'd appreciate:
- drag/drop across subfolders
- keep menu open after drag

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I cannot verify this part on OS X until drag&drop is possible for the Bookmarks menu. Mano, I cannot find the bug which covers this issue. Could we add it as depend on that one?

Serge, if a bug covers multiple OS it should also be verified using such builds.

Comment 55

[Asaf Romano (gone)]

This is a windows (and to some extend, linux) bug. I'm not sure what we consider as our current support for d&d in menus under OS X (in Fx2 it was just disabled).

Comment 56

[Jan Darmochwal]

The patch for this bug has probably caused bug 426268.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #55)
> This is a windows (and to some extend, linux) bug. I'm not sure what we
> consider as our current support for d&d in menus under OS X (in Fx2 it was just
> disabled).

Asaf, so is there an existing bug around which covers this RFE? I wasn't able to find one while searching in Bugzilla.