Closed
Bug 405087
Opened 17 years ago
Closed 17 years ago
Drag and drop on bookmarks menu has the wrong target
Categories
(Firefox :: Bookmarks & History, defect, P3)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 beta3
People
(Reporter: 162899, Assigned: mak)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Updated•17 years ago
|
Status: UNCONFIRMED → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
Updated•17 years ago
|
Whiteboard: [DUPEME]
Version: unspecified → Trunk
Comment 3•17 years ago
|
||
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]
Comment 4•17 years ago
|
||
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]
Updated•17 years ago
|
Flags: blocking1.9?
Comment 5•17 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
Flags: blocking-firefox3?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 M11
Comment 7•17 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.
Assignee | ||
Comment 8•17 years ago
|
||
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•17 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)
Assignee | ||
Comment 10•17 years ago
|
||
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?
Assignee | ||
Comment 11•17 years ago
|
||
do you have step to reproduce your problem? for me it's dropping to the right place...
Comment 12•17 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
Assignee | ||
Comment 13•17 years ago
|
||
ok, but this bug was about bookmark menu... that was what i didn't get :)
Comment 14•17 years ago
|
||
my mistake actually; i thought the bug was about bookmark toolbar.
Comment 15•17 years ago
|
||
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•17 years ago
|
||
All these problems were not present on 2 Aug 2007 (before bug 385536 was checked in).
Assignee | ||
Comment 17•17 years ago
|
||
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)
Assignee | ||
Comment 18•17 years ago
|
||
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?
Assignee | ||
Comment 20•17 years ago
|
||
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•17 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.
Assignee | ||
Comment 22•17 years ago
|
||
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...
Assignee | ||
Comment 23•17 years ago
|
||
this should be clear enough...
the submenu is much higher than the menu that opened it, but its y is almost equal!
Comment 24•17 years ago
|
||
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•17 years ago
|
||
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?
Assignee | ||
Comment 26•17 years ago
|
||
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•17 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•17 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•17 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.
Comment 30•17 years ago
|
||
(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.
Assignee | ||
Comment 33•17 years ago
|
||
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•17 years ago
|
||
(In reply to comment #33)
yes, all the problems seem to have been solved.
Comment 35•17 years ago
|
||
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+
Assignee | ||
Comment 36•17 years ago
|
||
Attachment #295223 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Assignee: nobody → mak77
Comment 37•17 years ago
|
||
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
Closed: 17 years ago → 17 years ago
Keywords: checkin-needed
OS: Windows Vista → All
Hardware: PC → All
Resolution: --- → FIXED
Comment 38•17 years ago
|
||
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 → ---
Assignee | ||
Comment 39•17 years ago
|
||
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
Assignee | ||
Comment 40•17 years ago
|
||
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
Closed: 17 years ago → 17 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
Assignee | ||
Comment 41•17 years ago
|
||
filled Bug 411219 "After a successful drop the bookmark menu is not rebuilt until restart" from comment #38
Comment 42•17 years ago
|
||
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
Comment 45•15 years ago
|
||
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.
Description
•