101059 - [FIX] Can't drag between folders on personal toolbar

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Yuying Long]

Build: 09-21 Branch build on WinXP-Ja

Steps:
1. Launch browser.
2. Create a personal toolbar folder. (though bookmark manager, or right click 
mouse on the top of toolbar area)
3. Go to any page, drag this URL to the folder that you created.

Result:
You can not drag it into the folder, it always be dragged as a seperate icon in 
toolbar.

This is WinXP only problem, on other platforms, e.g. Win2000, Mac or Linux, it 
can be dragged into a personal toolbar folder.

Comment 1

[Ben Ruppel]

Confirming. Weird.

Comment 2

[Eric Gerlach]

I get this on Win98 using 2001-09-30-trunk, ergo, not WinXP only.

Comment 3

[Ben Ruppel]

He he, win2k as well.  Seems this goes for the entire win32 family.  Surely it
worked a week ago.

Comment 4

[Chris Lyon]

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

Comment 5

[Chris Lyon]

Changing summary, OS and adding regression keyword.

Comment 6

[[not reading bugmail]]

it works for me on 2001100708 W2K. I can drag URL to manage bookmarks, the
personal toolbar, and when bookmarks->ptb folder.. it goes in my bookmarks.. 

Comment 7

[[not reading bugmail]]

its possible this a dup that got fixed recently.

It works for me, steps to reproduce:

1) left click url icon

2) continue holding down the mouse left button you should see little box while
dragging.


3) drag it over the bookmarks folder

4) bookmarks folder should open

5) continue down to personal toolbar

6) open folder

7) move right to opened list of bookmarks in the Personal toolbar

8) you should see a underline where you wanna drop it in. 

9) release the mouse and it is in there.

10) url is now in my personal toolbar folder..

---------

also do:
1, 2, then:

3) drag it in between 2 personal toolbar bookmarks

4) you should see the insert  [bookmark 1]|[bookmark 2] in between like this
example 

5) release the mouse button

6) bookmarks in my personal toolbar

Comment 8

[[not reading bugmail]]

its possible you are trying to use right click and you are suppose to left
click.. the hand icon will come in after mousing over the url icon. it will
change to different (dark shade of grey)..

Comment 9

[[not reading bugmail]]

I can also drag it to another window, and even From Mozilla's URL to Netscape's
URL bar.. maybe a mouse driver issue.. I'm using HID USB MS mouse drivers for my
Logitech Optical.  Did you install that Mozilla Gestures program?  Maybe another
that messed with mouse setting?  Just some ideas.

Comment 10

[[not reading bugmail]]

the only other thing I'm thinking is that Mozilla will not drop by trying to
highlight any one folder, not just personal toolbar, you have to open that
folder to put it in there.  You can highlight a folder in bookmarks sidebar and
drop it on the folder and that works.  I'm guess this is the issue.. I always
make sure I pop open the folder I want and stick it in the list where I want.

Comment 11

[Ben Ruppel]

Okay, here's what the problem is:  make sure you have a folder in your personal
toolbar folder.  So your personal toolbar toolbar will have a folder in it. 
Drag the link to your personal toolbar (not the personal toolbar in the
bookmarks or the bookmark manager.  The actual toolbar).  Note that insertion
points are possible to the right or left of the folder in the personal toolbar,
but not on top of the folder itself.  

This is a regression.  Before, you could drag a link to the folder itself.  The
folder didn't expand and this was not the best behavior, but at least you could
get a link into the right folder.  

The best behavior would be for personal toolbar folders to expand so that you
could place the link wherever you want in it/it's subfolders.  There's another
bug for that.

Comment 12

[[not reading bugmail]]

ok, I gotcha Ben. I tried your steps and I see the issue with a folder within
the personal toolbar folder. this folder will not open like it does in the
bookmarks list.  And it gets placed next to the folder at best.

Comment 13

[Ben Ruppel]

Cool.  I'm glad I was able to communicate it :)  

The old behavior (as far as I remember) was that the toolbar folder would be
darkened, and it would not expand.  If you dropped a link there, the link would
go at the bottom of that folder.

However, I'm pretty sure some other bugs are gonna make it so the folders
expand, so this bug will probably just go away when that happens.

Comment 14

[[not reading bugmail]]

yup, I agree with that.  

-Dennis

I just wanna say maybe too, it also appears the mozilla community hopes that a
lot of bugs will just go away and become invalid for the same reason, the closer
Mozilla gets to 1.0 and beyond.

:)

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter 
email notifications caused by this by searching for 'ilikegoats'.

Comment 16

[[not reading bugmail]]

Paul,

this is a dup of bug 106826, it works now with the checkin fix from Joe.

Comment 17

[lohphat]

Yes, but what's broken now is that you can't drag a link between folders.

If there are side-by-side folders, the move-to highlight selects the folders to
drang the link to, but does not ofer a virtical bar insert between them.

Comment 18

[Joe Hewitt (gone)]

mine

Comment 19

[Chris Lyon]

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

Comment 20

[Peter Trudelle]

nsbeta1- per Nav triage team

Comment 21

[Chris Lyon]

removing self from cc list

Comment 22

[Pierre Chanial]

taking, patch will follow

Comment 23

[Pierre Chanial]

Attachment: Patch v1.0

This patch implement dropping between folders in the personal toolbar and its
menus.
I simply emulate dragexit and dragenter during dragover when the mouse enters
the left, central and right region.
This patch fixes the method determineDropPosition so that the vertical position
of the mouse with respect to the pointed element is correctly calculated using
the menu origin (and not the window one as before)
It also provide a band aid for bug 148289.

r and sr needed.

Comment 24

[timeless]

Comment on attachment 85930 [details] [diff] [review]
Patch v1.0

+	     target.setAttribute("dragover-right" , "true"); 
+	   break;
indent break;

+    if (this.isContainer(aEvent.target)) 
+      border = Math.min(size/5,18); 
+    else 
+      border = size/2; 
someone will say that this looks like magic numbers. please comment explaining
why you chose them (either here in the bug or in your patch.. depending...).

Comment 25

[Pierre Chanial]

> indent break;
Will do it after Ben's review

> +    if (this.isContainer(aEvent.target)) 
> +      border = Math.min(size/5,18); 
> +    else 
> +      border = size/2; 
> someone will say that this looks like magic numbers. please comment explaining
> why you chose them (either here in the bug or in your patch.. depending...).

I guess the "magic" is for the container case.
- Maximum border set to "18": only relevant for the toolbarbuttons (horizontal case)
18 = favicon size (16) + left padding (2)
This limit ensures that for large toolbar button width, if the mouse pointer is
on the right of the icon, it will be in the DROP_ON area.

- "Math.min": only relevant for the horizontal case.
It ensures that for small toolbar button widths, we still have three distinct
areas (BEFORE, ON, AFTER).

- size/"5": determined from the menus (vertical case)
The most logical choice would have been 4 instead of 5 (outliners use 4:
DROP_BEFORE(25%), DROP_ON(50%) and DROP_AFTER(25%)) I preferred to use 5 based
on my experience in the vertical case (menus). It increases the DROP_ON area for
better horizontal sliding ease towards the subfolders. (6 instead of 5 decrease
too much the DROP_BEFORE and DROP_AFTER areas)

I will add in the patch 
+// see bug 101059, comment 25

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

+    if (this.determineDropPosition(aEvent) != this.mCurrentDropPosition) {
+      this.onDragExit(aEvent, aDragSession);
+      this.onDragEnter(aEvent, aDragSession);
+    }
This looks a little sleight-of-hand-ish... add a comment explaining?

as for what timeless said about magic numbers. Concerns me a little - is it
really a perf hit if you measure on the fly? If so, can you at least put the
number in a constant e.g. const kIconWidth = 16; to make the code a little
easier to read? Also, as this is a skinability constraint, can you add a comment
to the appropriate .css file? 

Comment 27

[Pierre Chanial]

Attachment: Patch v1.1

patch including Ben's and timeless' comments: the drop-before region width is
calculated on the fly by accessing the anonymous node.

Comment 28

[timeless]

Comment on attachment 90491 [details] [diff] [review]
Patch v1.1

eventually hard coding icon size will be bad.

if someone wants to make a vision accessible skin w/ very large fonts and large
small icons (32x32 icons, 64x64 images for buttons) we shouldn't block them,
but we can cross that bridge later...

Comment 29

[Pierre Chanial]

timeless was confused: the patch does not make anymore assumption on the css.

Comment 30

[Blake Ross]

Comment on attachment 90491 [details] [diff] [review]
Patch v1.1

sr=blake

Comment 31

[Pierre Chanial]

checked in by timeless