405087 - Drag and drop on bookmarks menu has the wrong target

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Roy Chang]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

When dragging and dropping bookmarks within the bookmarks menu to reorganize bookmarks, the bookmark folder that is being pointed to by the mouse pointer is not the selected one to move the bookmark, but instead, the folder below that folder is selected and will contain the moved bookmark.

Reproducible: Always

Steps to Reproduce:
1. Go to Bookmarks menu
2. Click and drag a bookmark on the main tree of the bookmarks list
3. Drop the dragged bookmark to the desired folder (as specified by where the mouse pointer is pointed to). 
Actual Results:  
This will not work and will lead to the bookmark being placed in the folder/tree directly underneath the desired folder.

Expected Results:  
Should have moved the bookmark to the folder which the mouse pointer was pointed at.

Comment 1

[Ria Klaassen (not reading all bugmail)]

I assume this is a duplicate but I can't find it atm.

Comment 3

[Ria Klaassen (not reading all bugmail)]

This is not a dupe of bug 337761, for this bug was not yet present when bug 337761 was filed. I'll try to find it when I have more time. 

Comment 4

[Ria Klaassen (not reading all bugmail)]

Regression range for this is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1186186860&maxdate=1186187819 so it is caused by bug 385536
Bug 391024 seems closely related although not quite the same, and there is no dependency added for this issue to the bug that caused this, so -> New.

Comment 5

[Neil Deakin]

Probably an issue with http://lxr.mozilla.org/mozilla/source/browser/components/places/content/controller.js
in _getDropPoint. It calculates the point to drop using boxobjects and clientY.
 

Comment 7

[lazierthanthou]

(In reply to comment #5)
> Probably an issue with
> http://lxr.mozilla.org/mozilla/source/browser/components/places/content/controller.js
> in _getDropPoint. It calculates the point to drop using boxobjects and clientY.
> 
> 

If I expand a folder in the bookmark toolbar and drag an item (from
within that folder) to a different position in the same folder, the item is
dropped much below where the mouse pointer is.

I investigated this a little bit. In Mozilla Firefox 3 Beta
1\chrome\browser.jar\content\browser\places\controller.js in the function
_getDropPoint: function TBV_DO_getDropPoint(event):
I find that on dropping a dragged item at a particular entry, I get the
following values:
event.target.boxObject.y = 209 (= a)
event.target.boxObject.height = 20 (= b)
event.clientY = 250 (= c)
I was expecting that a <= c <= a + b, which is not the case.
also, i found that event.target refers to the correct item. I did so by looking
at event.target.label. The dropped item is getting added a couple of items
below the mouse pointer because event.clientY is not on event.target at all.
Thus comparing event.clientY with nodeY in the function is incorrect.
event.target and its properties should be used to identify the target.
But, if we want to find whether mouse is in upper half or lower half,
event.clientY is needed which, IMHO, appears to return incorrect value.

I had posted this comment on bug 337761. Repeating it here after being helpfully told that what I am talking about is perhaps this bug.

Comment 8

[Marco Bonardo [:mak]]

Attachment: subtract this._popup.boxObject.y from clientY

this is working fine here, event.clientY is related to client area, while nodeY is relative to the popup

Comment 9

[lazierthanthou]

(In reply to comment #8)
> Created an attachment (id=292742) [details]
> subtract this._popup.boxObject.y from clientY
> 
> this is working fine here, event.clientY is related to client area, while nodeY
> is relative to the popup
> 

I tried it, but this too detected an incorrect node (the droppoint was one entry off the mark instead of 4 as in the original code)

Comment 10

[Marco Bonardo [:mak]]

it was working fine, will test again, thank you for hints and testing. did you testing this with current real dragging or reading out values in debugging?

Comment 11

[Marco Bonardo [:mak]]

do you have step to reproduce your problem? for me it's dropping to the right place...

Comment 12

[lazierthanthou]

Marco, I changed controller.js exactly as in your attachment. Then, I performed the following steps:
1. click on a folder in the bookmark toolbar to expand it
2. drag an item A and drop it between items P and Q. None of A, P & Q are folders.
3. expectation: item A should land between P and Q.
4. actual result: item A lands after item R, where R is the item below Q.

Moreover, this behaviour is always reproducible; under any folder in the bookmark toolbar; under any folder within a folder in the toolbar; on Fx 3.0b1 and 3.0b2 rc1 on Win XP

Comment 13

[Marco Bonardo [:mak]]

ok, but this bug was about bookmark menu... that was what i didn't get :)

Comment 14

[lazierthanthou]

my mistake actually; i thought the bug was about bookmark toolbar.

Comment 15

[Ria Klaassen (not reading all bugmail)]

The insert point behaviour is different in several folders and also dragging up or dragging down might result in a different insertion point. E.g. when I drag a bookmark in a subfolder in the bookmarks menu up, it is inserted 9 items to high!
But when I try a folder in the same bookmarks menu that looks totally similar (only a bit larger), it inserts only one item too high. The behaviour is quite consequent for every folder.

Comment 16

[Ria Klaassen (not reading all bugmail)]

All these problems were not present on 2 Aug 2007 (before bug 385536 was checked in).

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 292742 [details] [diff] [review]
subtract this._popup.boxObject.y from clientY

clearing review, this works fine on the menu, but not on toolbar. i'll try to investigate some more

Comment 18

[Marco Bonardo [:mak]]

the problem is that for menu the boxObject.Y is on the popup, while on the toolbar the boxObject.Y is on the toolbar object, so there is an offset of about the height of the toolbar... since an item is about 20 px and the toolbar is about 24 px there is "about" 1 item offset on the toolbar...

this can be solved with a workaround (i also have a working patch that subtract the toolbar height offset), but should that work like that? 
i'd expect this._popup.boxObject.Y return the start of the popup and not of the hbox that opens the popup...

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try using getBoundingClientRect instead of boxObject coordinates?

Comment 20

[Marco Bonardo [:mak]]

thx Roc, but still the same values with getBoundingClientRect. 
i have menu and bookmark toolbar open (no other toolbars), calling this._popup.boxObject.y for the bookmarks menu = 21, for a bookmarks toolbar popup = 23 (while it should be 23 + toolbarbutton height)... i doubt that there is a problem in getting the _popup y, or _popup is pointing to the wrong node for the toolbar (next thing i'll check)... 

Comment 21

[Neil Deakin]

Yes, there are problems where the boxobject coordinates (which should be the same as getBoundingClientRect returns) are not calculated properly for menus. Bug 393451 is for submenus.

The value of boxObject.y should be the vertical offset in pixels from the upperleft corner of the popup to the upperleft corner of the document. clientY should be similar but to the event position.

It's likely that the existing bookmarks code is relying on some broken coordinate calculation behaviour for popups. In particular, this line:
   var nodeY = xulNode.boxObject.y - this._popup.boxObject.y;
seems to compare an items height from the top of the popup to clientY when it shouldn't be subtracting anything.

Comment 22

[Marco Bonardo [:mak]]

Bug 393451 is not completely applicable here, the problem i see is that boxObject.y of the toolbarbutton popupmenu is equal to the boxObject.y of the toolbarbutton, while the popupmenu is under the button. So the only way of having both the menu and the toolbar working is to add an offset for the toolbar popupmenu items...

but, as Ria said, this happens also on submenus (at least on a second level submenu), the boxObject.y is taken from the parent and not from the menu itself, while Bug 393451 says that top is correct...

Comment 23

[Marco Bonardo [:mak]]

Attachment: (screenshot) wrong coords

this should be clear enough... 
the submenu is much higher than the menu that opened it, but its y is almost equal!

Comment 24

[Asaf Romano (gone)]

er, this._popup.parentnode is just the <menu> element, not the parent popup, see
http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js#1680

Comment 25

[Neil Deakin]

Attachment: reduced testcase

This bug is caused because the toolbar has 'overflow: hidden' on it which causes the view offsets to be calculated different. The frame's position is correct and the popup's view coordinates are correct. But the popup code makes a call to nsFrame::GetOriginToViewOffset which, for the overflow case, results in (0,0) being returned because the parent view is not found in the hierarchy (see the big disabled part of that method).

roc, what is the nsFrame::GetOriginToViewOffset method supposed to be doing? The only caller is from nsMenuPopupFrame, maybe it's completely wrong?

Comment 26

[Marco Bonardo [:mak]]

apart from the toolbar that can be work-arounded with an offset until it is not fixed, in sub-sub-sub(etc.)menus (from the bookmark menu) following a moved-up menu (because it does not fit on screen), all objectBox.y are relative to the menu as it was never been moved-up, they are all wrong and pointing to non-sense locations...

Comment 27

[lazierthanthou]

a small repitition, but it seems important to me:
if a = event.target.boxObject.y,
   b = event.target.boxObject.height,
   c = event.clientY
then a <= c <= a + b should be satisfied at all times.

And, in the particular case of bookmark toolbars, this relation is not satisfied. I feel that this needs fixing. If fixed, all other things will easily fall into place. Can someone guide me to where the code for assigning properties of event is?

Comment 29

[Andreas]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121905 Minefield/3.0b3pre

Additionaly to comment #0, the name of the folder below the one pointed by the mouse, disappears. 

Comment 30

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #29)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121905
> Minefield/3.0b3pre
> 
> Additionaly to comment #0, the name of the folder below the one pointed by the
> mouse, disappears. 
> 
This is bug 405969.

Comment 33

[Marco Bonardo [:mak]]

Attachment: fix bad coords in _getDropPoint

well, this fixes everything, menu, submenus and toolbar folders, with real minor code changes.

i've changed everything to relative coords since absolute ones are not reliable on menus, i've tested this with latest patch from Simon Bünzli for Bug 389290 (to get back drop indicators), and it's working really fine.

Comment 34

[lazierthanthou]

(In reply to comment #33)
yes, all the problems seem to have been solved.

Comment 35

[Asaf Romano (gone)]

Comment on attachment 295223 [details] [diff] [review]
fix bad coords in _getDropPoint

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.191
>diff -u -8 -p -r1.191 controller.js
>--- browser/components/places/content/controller.js	28 Dec 2007 01:53:59 -0000	1.191
>+++ browser/components/places/content/controller.js	3 Jan 2008 14:07:54 -0000
>@@ -1297,42 +1297,42 @@ PlacesMenuDNDObserver.prototype = {
>       // Ignore static content at the top and bottom of the menu.
>       start = this._view._startMarker + 1;
>       if (this._view._endMarker != -1)
>         end = this._view._endMarker;
>     }
> 
>     for (var i = start; i < end; i++) {
>       var xulNode = this._popup.childNodes[i];
>-      var nodeY = xulNode.boxObject.y - this._popup.boxObject.y;
>+      var nodeY = xulNode.boxObject.y - this._popup.childNodes[0].boxObject.y;

Let's cache this outside of the loop, also s/childNodex[0]/firstChild/.

r=mano otherwise.

Comment 36

[Marco Bonardo [:mak]]

Attachment: addressed review comments

Comment 37

[Reed Loden [:reed]]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.192; previous revision: 1.191
done

Comment 38

[Ria Klaassen (not reading all bugmail)]

In the latest trunk build I still see a problem; the item doesn't seem to move when I drag it. Only after a browser restart I see that the item had moved.
I tested this on a new profile with a freshly imported bookmarks.html.

Comment 39

[Marco Bonardo [:mak]]

i think that this bug should be changed to "DD&D on bookmark menu hilight the wrong position", and then fill a new bug on the new issue that is now visible... it should be better

Comment 40

[Marco Bonardo [:mak]]

i'm going mark this as resolved as "Drag and drop on bookmarks menu has the wrong target" and open spinoff bug on the rebuild problem in comment #38

Comment 41

[Marco Bonardo [:mak]]

filled Bug 411219 "After a successful drop the bookmark menu is not rebuilt until restart" from comment #38

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010813 Minefield/3.0b3pre.

There is still a lot of drag and drop weirdness though.

Comment 45

[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