243720 - bookmark can't move into sub-folder in personal toolbar (drag and drop)

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[baffclan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040515
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a) Gecko/20040515

bookmark can't move into sub-folder in personal toollbar.


Reproducible: Always
Steps to Reproduce:
1. bookmark drag a folder on personal tollbar.
2. folder is opend.
3. and dragging on sub-folder.
4. can not open subfolder.
5. can not drop bookmark.

Actual Results:  
Can not move bookmark sub-folder in personal toollbar.

Expected Results:  
Can move any bookmark.

Comment 1

[Andreas Kunz]

Confirming, this happens on current trunk, but _not_ on the 1.7 branch. Bug
201854, the only change in bookmarksMenu.js since the branch was cut, therefore
cannot be responsible (it is also on the branch).
Bug 221619 was the only other one touching the bookmarks js files in the last
weeks, but looks unlikely to be the reason.
-> NEW

Comment 2

[Andreas Kunz]

This regressed between builds 2004-05-13-08 and 2004-05-13-15 - narrow
timeframe, but still a lot of checkins...

I just verified my patch for bug 210719 is _not_ the culprit by reversing the
patch and recompiling.
The same way I checked bug 242833 and bug 107518.

What is left? Bug 191839 is pretty large, apart from that only smaller fixes
that do not seem related...

Comment 3

[Hermann Schwab]

Firefox 1.8a: 2004051408 is showing the same problem

Comment 4

[Andreas Kunz]

Finally found it (by selectively removing patches and rebuilding): bug 242833
caused this regression. Sounds most logical, anyway. Not sure why I tested so
many other checkins before...

Comment 5

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

> 1. bookmark drag a folder on personal tollbar.

What does this mean? I can't drag a toolbar folder.

Comment 6

[Hermann Schwab]

(In reply to comment #5)
> > 1. bookmark drag a folder on personal tollbar.
> 
> What does this mean? I can't drag a toolbar folder.

Drag a icon from the Location Bar into a folder in a folder in the Personal Toolbar.
The folder in the PT opens, but folders in this folder don´t open, when trying
to drop a bookmark.

Comment 7

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

I tried this on Linux and it seems to still work. Could it be Windows only?

Comment 8

[Hermann Schwab]

(In reply to comment #7)
> I tried this on Linux and it seems to still work. Could it be Windows only?

Also don´t work:
Instead of dragging the icon down to a folder in PT, you can drag it left and up
to the Bookmarks menu, that opens, and you can drag down and drop where you
want, but folders don´t open.

This regressed  1.8a: 2004051408 on Mozilla and Firefox, tested in Windows

Working: Folders in Personal Toolbar 
open when hovered, for selecting or dropping  a bookmark

Working: Folders in folders in Personal Toolbar 
open when hovered, for selecting a bookmark only.

Regressed: Folders in folders in Personal Toolbar 
don´t open when hovered, while dragging a bookmark from the Location bar.


Workaround 1: 
if you have Bookmarks Manager minimized in the Windows taskbar, drag the icon
there, Bookmarks Manager maximizes, and you can drag the bookmark where you want
to, folders in folders are opening when hovered.

Workaround 2:
drop the icon between two folders, so later you know that that isn´t at the
right place. Later open Bookmarks Manager, and drag and drop these misplaced
bookmarks.

Comment 9

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

Yeah, that stuff definitely works for me in Linux.

Comment 10

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

Someone should try the patch that I'm about to attach in bug 243757

Comment 11

[Andreas Kunz]

roc:
no luck.
However, maybe my knowledge about bookmarksMenu.js can help you track it down;
getBTOrientation in:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarksMenu.js#191
is responsible for deciding if the bookmark insert line for dropping is BEFORE
or AFTER the item or the item can be dropped ON/into a container.
Somehow this might never return ON because it gets wrong numbers, which would
cause this bug.

Comment 12

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

Thanks. I think I found the problem.

Comment 13

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

This code assumes that clientX/Y is relative to the parent box object. In the
old code, we made clientX/Y relative to the popup. In the new code, I intended
it to be relative to the document origin, but instead it's just wrong. The
question is, what should clientX/Y be in this case? Is it reasonable to make it
relative to the popup, or is that a genuine violation of the spec? If we make it
relative to the document origin, is it possible to fix current users?

Comment 14

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

I think the thing to do would be to make clientX/Y relative to the document
origin, not the popup origin, and in this code compare boxObject.screenX/Y to
event.screenX/Y

Comment 15

[Frank Burleigh]

*** Bug 244729 has been marked as a duplicate of this bug. ***

Comment 16

[Stephan Slabihoud (:slabbi)]

Still in
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040607

Robert, have you identified the bug yet?

Comment 17

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

Yes I have. But I haven't fixed it :-(

Comment 18

[Peter Lairo]

Strange, I am getting this bug on-and-off in various nightlies (mostly on!).
Currently, it's not working (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.8a2) Gecko/20040628 Firefox/0.8.0+).

Comment 19

[Jesse Ruderman]

*** Bug 245754 has been marked as a duplicate of this bug. ***

Comment 20

[Peter Lairo]

This is a major usability problem (at least for me). Requesting blocking status.

Comment 21

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

I thought I could reproduce this before. I can't reproduce it now, on a ~1-week
old trunk build.

Comment 22

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

Attachment: potential fix

This patch *should* fix the bug if there still is one. So if you're still
having problems with this bug, please apply this patch. If the patch fixes the
bug for you, we'll check it in.

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> Created an attachment (id=152999)
> potential fix
> 
> This patch *should* fix the bug if there still is one.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a1) Gecko/20040520] (release) (W98SE)

This is a fix for v1.8a1 :-)
I did not try any v1.8a2 nighlies (with/without the patch)...

Comment 24

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

Serge, you mean you see the bug without this patch, and this patch makes the bug
go away?

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> Serge, you mean you see the bug without this patch, and this patch makes the bug
> go away?

Exactly: your patch fixes my v1.8a1 :-)

Comment 26

[[e-mail]]

Comment on attachment 152999 [details] [diff] [review]
potential fix

>-        coordValue = overButtonBoxObject.x;
>-        clientCoordValue = aEvent.clientX;
>+        coordValue = overButtonBoxObject.screenX;
>+        clientCoordValue = aEvent.screenX;
No point penalizing X users for code that works.
r=me on the menu change.

Comment 27

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

Okay, I checked that in. I'll leave this open because I need to fix
clientX/clientY in popups.

The patch needs aviary checkin.

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> Okay, I checked that in. I'll leave this open because I need to fix
> clientX/clientY in popups.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040714] (release) (W98SE)

Your
{{
2004-07-17 05:25	roc+%cs.cmu.edu 	mozilla/ xpfe/ components/ bookmarks/
resources/ bookmarksMenu.js 	1.15
}}
causes
{{
Warning: reference to undefined property overButtonBoxObject.clientX
Source File: chrome://communicator/content/bookmarks/bookmarksMenu.js
Line: 216
}}

while your proposed patch, using |overButtonBoxObject.screenX|, seemed to work...

Comment 29

[[e-mail]]

roc, you didn't undo the first change properly :-(
Also xpfe rules don't allow r+sr yet, unless jag forgot to tell me ;-)

Comment 30

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

Sorry. I backed it out.

Comment 31

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

Attachment: correct fix

Okay

Comment 32

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

Comment on attachment 153566 [details] [diff] [review]
correct fix

who should I get to sr this, then?

Comment 33

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

checked in. thanks guys.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is not reproduced with 2004071808-trunk/WinXP :-)

Comment 35

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

That patch, or something like it, should go into the Aviary branch, assuming
Firefox supports dragging bookmarks into the menu.

Comment 36

[Andreas Kunz]

Yes, Firefox seems to support it.
That part of bookmarksMenu.js is identical in /xpfe and /browser, so I guess the
patch is the same; I was just about to make a diff for the /browser version...

Comment 37

[Andreas Kunz]

Attachment: fix for firefox (what an alliteration!)

Will request review once it's tested.

Comment 38

[Andreas Kunz]

Comment on attachment 154037 [details] [diff] [review]
fix for firefox (what an alliteration!)

Ok, tested the patch, works just like in Seamonkey.

I'm not familiar with the Firefox review rules and the reviewers'
responsiveness ... I'll pick blake and bryner for this small and easy port of
roc's patch.

Comment 39

[Andreas Kunz]

Comment on attachment 154037 [details] [diff] [review]
fix for firefox (what an alliteration!)

I was told this only needs review (+approval) and vlad would be the man for
it...

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 154037 [details] [diff] [review]
fix for firefox (what an alliteration!)

looks fine to me, r=vladimir

Comment 41

[Asa Dotzler [:asa]]

Comment on attachment 154037 [details] [diff] [review]
fix for firefox (what an alliteration!)

a=asa

Comment 42

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

In on aviary and firefox trunk.