Drag and drop on bookmarks menu has the wrong target

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Bookmarks & History
P3
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Roy Chang, Assigned: mak)

Tracking

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
I assume this is a duplicate but I can't find it atm.
Whiteboard: [DUPEME]
Status: UNCONFIRMED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 337761
Whiteboard: [DUPEME]
Version: unspecified → Trunk
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. 
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Whiteboard: [DUPEME]
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.
Assignee: nobody → jag
Blocks: 385536
Status: UNCONFIRMED → NEW
Component: Bookmarks → XP Toolkit/Widgets
Ever confirmed: true
Product: Firefox → Core
QA Contact: bookmarks → xptoolkit.widgets
Whiteboard: [DUPEME]
Flags: blocking1.9?

Comment 5

11 years ago
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.
 
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee: jag → nobody
Component: XP Toolkit/Widgets → Places
Flags: blocking1.9+
Product: Core → Firefox
QA Contact: xptoolkit.widgets → places
Duplicate of this bug: 405949
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M11

Comment 7

11 years ago
(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.
Created attachment 292742 [details] [diff] [review]
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
Attachment #292742 - Flags: review?(mano)

Comment 9

11 years ago
(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)
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?
do you have step to reproduce your problem? for me it's dropping to the right place...

Comment 12

11 years ago
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
ok, but this bug was about bookmark menu... that was what i didn't get :)

Comment 14

11 years ago
my mistake actually; i thought the bug was about bookmark toolbar.
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.
All these problems were not present on 2 Aug 2007 (before bug 385536 was checked in).
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
Attachment #292742 - Flags: review?(mano)
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...
Try using getBoundingClientRect instead of boxObject coordinates?
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

11 years ago
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.
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...
Created attachment 292946 [details]
(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 25

11 years ago
Created attachment 292972 [details]
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?
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

11 years ago
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?

Updated

11 years ago
Duplicate of this bug: 394401

Updated

11 years ago
Summary: Issue with drag and drop bookmarks to bookmark folders within the bookmarks menu → Drag and drop on bookmarks menu puts items in wrong spot

Comment 29

11 years ago
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. 
(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.

Updated

11 years ago
Blocks: 409073
Duplicate of this bug: 409692
Duplicate of this bug: 410292
Created attachment 295223 [details] [diff] [review]
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.
Attachment #292742 - Attachment is obsolete: true
Attachment #295223 - Flags: review?(mano)

Comment 34

11 years ago
(In reply to comment #33)
yes, all the problems seem to have been solved.
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.
Attachment #295223 - Flags: review?(mano) → review+
Created attachment 295458 [details] [diff] [review]
addressed review comments
Attachment #295223 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: nobody → mak77
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago10 years ago
Keywords: checkin-needed
OS: Windows Vista → All
Hardware: PC → All
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Summary: Drag and drop on bookmarks menu puts items in wrong spot → Drag and drop on bookmarks menu has the wrong target
filled Bug 411219 "After a successful drop the bookmark menu is not rebuilt until restart" from comment #38
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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 412564
Duplicate of this bug: 414718
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
You need to log in before you can comment on or make changes to this bug.