235243 - Need drag and drop feedback for toolbar bookmarks

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

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

Currently when you drag around bookmarks on the Bookmarks Toolbar there is no
feedback as to where the bookmark will be placed. I suggest finding a way of
imitating the drop feedback used for the customizable main toolbars so that
there is less guesswork as to where something will end up. This should be
applied cross platform - a good basic solution even if ugly will do to get
started, Kevin Gerich will fix the mac theme if he has better ideas. 

Also, don't forget to show indication that an item is going to be filed into a
folder on the toolbar, as opposed to being dropped to either side of it.

Comment 1

[Jan Varga [:janv]]

Ben, I think this is just a regression. I checked Phoenix 0.5 and it works fine
in this version. It regressed in Firebird 0.6
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=browser.css&branch=&root=/cvsroot&subdir=mozilla/browser/base/skin/Attic&command=DIFF_FRAMESET&rev1=1.66&rev2=1.67
"-moz-appearance: none" was removed and it caused that all bookmarks are painted
as native toolbar items. So dragover-[left,right,top,bottom] doesn't apply to
them anymore although it's still specified in browser.css
I think there are 2 possibilities to fix this bug:
1. We can add "-moz-appearance: none" back
2. Rewrite drop feedback for toolbar bookmarks completely

Comment 2

[Jan Varga [:janv]]

I forgot to ask, is "border" supposed to work even with native toolbar items?

Comment 3

[Tracy Walker [:tracy]]

added related testrunner testcase  to URL

Comment 4

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

Jan - what is the cost/benefit of each approach, and how much time to code? I
assume -moz-appearance: none; is probably relatively simple. 

Comment 5

[Jan Varga [:janv]]

Yes, -moz-appearance: none; is simple. The other solution would require a lof of
time.

Comment 6

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

Let me ask around and see what people think we should do here. 

Comment 7

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

Let me ask around and see what people think we should do here. 

Comment 8

[Pierre Chanial]

Attachment: patch

This is typically a case in which having xul element absolute positionning
would be great to have.

This patch is rather long, but it is mostly code refactoring.
The new part relevant to the DND feedback is very slim.

To get the feedback on a 'native' widget, I toggle the -moz-appearance to
'none'.
There is a ugly hack (dimension substractions) to deal with the fact that the
border width may change and there is no easy to get the border width of a
'native' widget.

I did that only for menuitems (only 'native' in gtk2). For the toolbar, I
preferred to add an empty button in the bookmarks toolbar stack. The feedback
display was well integrated with the themes I checked and I kept it. For
consistency, we could still easily use the toolbarbutton borders.

In addition to the DND feedback, this patch:
- introduces a new var 'BookmarksInsertion' that includes the name 'Bookmarks'
only to avoid eventual naming conflicts. It 'encapsulates' DND feedback
display,  We only (!) need absolute xul positionning to make it an xbl widget
that we could reuse, for instance in the customize window.
- rewrite the rtl handling related to DND. BookmarksInsertion deals with this
subtleties and the "public" feedback orientation ("before"|"on"|"after")
returned by BookmarksInsertion.getOrientation refers now to the DOM.
- paste and copy commands on the menus now insert after the rclicked element.
The hack that appended everything at the end of the container has been removed
or more accurately replaced by another one that is at least invisible to the
user and we already used for DND. The problem is due a problem with the content
building of containers with siblings (open in tabs stuff) not generated by the
bookmarks template.
- usual small glitch fixing.

What this patch doesn't do (or is not the cause of :) ):
- the rtl part of the chevron handling... the fix that has been checked in
leaves a very big gap between the chevron and the last visible bookmark in the
toolbar.
- DND in bookmarks toolbar folders is broken on linux (only works the first
time). I will investigate it but it doesn't seem to be trivial.

Comment 9

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

Let's see this tested for a bit on the trunk before landing it on aviary. 

Comment 10

[Pierre Chanial]

I checked the patch in the trunk.
Note that the trunk currently suffers from a regression (not caused by my patch)
in that the context menus in the bookmarks toolbar won't popup. They still do in
the menupopups, though.
windows xp feedback would be highly appreciated :)

Comment 11

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

Comment on attachment 156577 [details] [diff] [review]
patch

>-      this._db = p.database;
>+
>+    this._db = p? p.database : null;

Minor nit: "p ? p.database : null" (here and other places.. space before the ?,
this ain't lisp :)

>+    // hide the 'open in tab' menuseparator because bookmarks
>+    // can be inserted after it if they are dropped after the last bookmark
>+    // a more comprehensive fix would be in the menupopup template builder
>+    var menuSeparator = null;
>+    var menuTarget = document.popupNode.parentNode;
>+    if (menuTarget.hasChildNodes() &&
>+        menuTarget.lastChild.getAttribute("class") == "openintabs-menuitem") {
>+      menuSeparator = menuTarget.lastChild.previousSibling;
>+      menuTarget.removeChild(menuSeparator);
>+    }

So the idea here is to hide the separator, but leave "Open in Tabs"?  I
guess this fixes the RDF container issues with adding an item to the
end of a container; though it seems odd that dropping on "Open in
Tabs" would add the item to the end of the container.  I'd prefer
"Open in Tabs" to not show up at all; it looks odd to have it there
when doing a drop, especially without the separator.

Other than that, I'll have to play with it for a bit off the trunk. After this
patch, can you drag to the Bookmarks menu and drop an item at the end of the
menu?

Comment 12

[Pierre Chanial]

(In reply to comment #11)
> (From update of attachment 156577 [details] [diff] [review])
> >-      this._db = p.database;
> >+
> >+    this._db = p? p.database : null;
> 
> Minor nit: "p ? p.database : null" (here and other places.. space before the ?,
> this ain't lisp :)

as you wish.

> So the idea here is to hide the separator, but leave "Open in Tabs"?  I
> guess this fixes the RDF container issues with adding an item to the
> end of a container;

Err no, removing the menuseparator instead of the menuseparator+menuitem 'open
in tab' is only for cosmetic reasons. The transaction is processed before the
menupopup closes, and this one is also updated before it closes. So removing the
menusepator only just make the hack nearly invisible to the user.

That's not an RDF container issue. Without this hack, the bookmarks datasource
is ok after that the transaction is performed. However the xul content that is
built on the datasources is not.
The problem is in nsXULContentBuilder.cpp, I have not investigate yet, but imo,
it would be highly risky to modify the content builder at this stage.

> though it seems odd that dropping on "Open in
> Tabs" would add the item to the end of the container.
no, canDrop is false on the the 'Open in Tabs' menuitem. And if it doesn't, it's
a bug.

> I'd prefer "Open in Tabs" to not show up at all;
I disagree, folders are just automatically opened. And I don't see why the
behaviour would be different when it is done manually.

> it looks odd to have it there when doing a drop, especially without the separator.
the menuseparator only disappears ondrop. In fact, that's barely noticeable.
While dragging over the menupopup, the menuseparator+menuitem 'open in tabs' are
still there.

> Other than that, I'll have to play with it for a bit off the trunk.
Cool!

>After this patch, can you drag to the Bookmarks menu and drop an item at the
end of the menu?
Hu, there's some funkiness (bookmarks toolbar insertion button should be hidden)
with the bookmarks menu. I'll patch that.
But other that the funkiness, yes, you can.

Comment 13

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

(In reply to comment #12)
> > So the idea here is to hide the separator, but leave "Open in Tabs"?  I
> > guess this fixes the RDF container issues with adding an item to the
> > end of a container;
> 
> Err no, removing the menuseparator instead of the menuseparator+menuitem 'open
> in tab' is only for cosmetic reasons. The transaction is processed before the
> menupopup closes, and this one is also updated before it closes. So removing the
> menusepator only just make the hack nearly invisible to the user.

Ah! I misunderstood what the intent here was.  Since it only hides it on onDrop,
should be fine (ignore my other coments, then :).

> >After this patch, can you drag to the Bookmarks menu and drop an item at the
> end of the menu?
> Hu, there's some funkiness (bookmarks toolbar insertion button should be hidden)
> with the bookmarks menu. I'll patch that.
> But other that the funkiness, yes, you can.

Interesting, this was the impetus for the original -1 hack.. I'll look for the
original bug and figure out what's going on.

Comment 14

[Pierre Chanial]

> > >After this patch, can you drag to the Bookmarks menu and drop an item at the
> > end of the menu?
> > Hu, there's some funkiness (bookmarks toolbar insertion button should be hidden)
> > with the bookmarks menu. I'll patch that.
> > But other that the funkiness, yes, you can.
> 
> Interesting, this was the impetus for the original -1 hack.. I'll look for the
> original bug and figure out what's going on.
> 

I just fixed the issue I was refering to: I forgot to add the menuseparators in
the feedback discrimination switch. Hovering on a menuseparator has the effect
of making the insertion button appear in the bookmarks toolbar with a large
width when and where it shouldn't. That's not related to the -1 hack.
DND in folders on GTK2 is still half (well, more than one half) broken, I'll
investigate it, but as Vlad said, it probably never worked.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 156577 [details] [diff] [review]
patch

please don't request approval until you have a fully reviewed patch. thanks.

Comment 16

[Gérard Talbot]

It's probably too late to change anything in this bug but anyway I'll mention
the idea of using specific, custom cursors for bookmark drag-N-drop, no-drop,
copying, moving, etc. like in bug 230337.

Comment 17

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

I just want to ask - what does the MiniT tab d&d extension do? It adds a little
arrow icon above the tab strip to show you where your tab is going to be
dropped.. it appears to be floating over the toolbar controls themselves...
something like that may be practical. 

Comment 18

[Ian Neal]

This fix was reversed out when aviary landed on the trunk

Comment 19

[Ian Neal]

Attachment: Patch against current trunk v0.2

Not sure if this is sufficent to get things working again.

Comment 20

[amano]

Why don't you ask for r/sr?

Comment 21

[Ian Neal]

Comment on attachment 169845 [details] [diff] [review]
Patch against current trunk v0.2

This, as far as I can tell, relands the bits of the previous patch that got
reversed out when aviary landed. Dragging and dropping bookmarks does not work
at the moment and this MAY fix part of the problem.

Comment 22

[amano]

Hmm. The current state here is, that Vladimir is (hopefully) reviewing this
patch, right?

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Chris]

this has been resolved, no?

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050522
Firefox/1.0+ ID:2005052219

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to comment #24)
> this has been resolved, no?
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050522
> Firefox/1.0+ ID:2005052219

Nope, still seeing it here. Better question is does the patch need to be
re-diffed again due to bitrot?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050523
Firefox/1.0+

Comment 26

[Pete Hammer]

Dragging a URL or favicon to sub-folders of the folders on the toolbar does not
work for me, in either firefox or mozilla (mozilla bug #199998, never fixed).  

Dragging to the toolbar, and to folders on the toolbar, works fine.  As far as I
know, dragging to sub-folders has NEVER worked on the Mac, 1.0 - 1.7, OS9 and
OSX.   It worked on at least some of the windows builds I've used.  So make this
Mac only or whatever, but at least clarify what it is.

I think that bug kept getting confused as to what exactly is wrong.  I repeat:
dragging to the toolbar, and to FOLDERS on the toolbar works, attempts to drag
into a SUBFOLDER of a toolbar folder DOESN'T work - the folders don't pop open.
So I'm going to open a new one in mozilla, again.

Comment 27

[Asa Dotzler [:asa]]

if we fix this, it'll need to happen in 1.8b4 so the aviary-1.1? flag is redunant.

Comment 28

[Wayne Mery (:wsmwk)]

(In reply to comment #26)
> Dragging a URL or favicon to sub-folders of the folders on the toolbar does not
> work for me, in either firefox or mozilla (mozilla bug #199998, never fixed).  

This bug is about lack of visual feedback, not lack of funtionality

Comment 29

[Pete Hammer]

For lack of drag/drop subfolder functionality in OSX, I opened a new bug: bug 299185

"Toolbar folders don't open when a url is dragged onto them - prevents placement
of bookmarks in subfolders or where desired in order in folders"

Comment 30

[Wayne Mery (:wsmwk)]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050907
Firefox/1.6a1

WFM

Comment 31

[Andreas Wuest]

(In reply to comment #30)

> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050907
> Firefox/1.6a1
> 
> WFM

W not FM on MacOS X. Since this report deals with all platforms and the problem
seems to be solved for all other platforms except Mac, I created bug #311329 to
deal with Mac only, also giving some more insight into the expected behaviour.

Comment 32

[Gary van der Merwe]

Does not WFM either. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
Gecko/20051003 Firefox/1.4.1

Wayne: maybe you have confused the functionality that this bug refers to? It is
specific to the bookmarks toolbar, NOT the tree in the side bar, and the manage
bookmarks Window.

P.S. 
I have tested "Patch against current trunk v0.2" and it works well.
Maybe we should prod Vladimir, or ask someone else to review? Allthough I doubt
that this would be approved for 1.5.

Comment 33

[Gary van der Merwe]

Also dose not WFM on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20051005 Firefox/1.6a1.

Comment 34

[Wayne Mery (:wsmwk)]

(In reply to comment #32)
> Does not WFM either. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
> Gecko/20051003 Firefox/1.4.1
> 
> Wayne: maybe you have confused the functionality that this bug refers to? It is
> specific to the bookmarks toolbar, NOT the tree in the side bar, and the manage
> bookmarks Window.

I don't use sidebar.
I wonder why this WFM (both XP and w2k), and doesn't WF no one else?
XP currently Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050925 Firefox/1.6a1 (+extensions EBS, bookmark search, and EHM, history
manager)
w2k same extensions

comment 26 WFM
"dragging to the toolbar, and to FOLDERS on the toolbar works, attempts to drag
into a SUBFOLDER of a toolbar folder DOESN'T work - the folders don't pop open.
So I'm going to open a new one in mozilla, again."

suite (nightly) also WFM.  hope this fix doesn't break me :)

Comment 35

[Andreas Wuest]

(In reply to comment #34)

> comment 26 WFM
> "dragging to the toolbar, and to FOLDERS on the toolbar works, attempts to drag
> into a SUBFOLDER of a toolbar folder DOESN'T work - the folders don't pop open.
> So I'm going to open a new one in mozilla, again."
> 
> suite (nightly) also WFM.  hope this fix doesn't break me :)

But as far as I can tell, comment #26 is solely talking about platform Mac, not
windows!!

I have no recent material to compare with, but with an about a month old build
on platform Linux, dragging into FOLDERS on the bookmarks toolbar and into
SUBFOLDERS therein works.

Comment 36

[Wayne Mery (:wsmwk)]

(In reply to comment #35)
> (In reply to comment #34)
> 
> > comment 26 WFM
> > "dragging to the toolbar, and to FOLDERS on the toolbar works, attempts to drag
> > into a SUBFOLDER of a toolbar folder DOESN'T work - the folders don't pop open.
> > So I'm going to open a new one in mozilla, again."
> > 
> > suite (nightly) also WFM.  hope this fix doesn't break me :)
> 
> But as far as I can tell, comment #26 is solely talking about platform Mac, not
> windows!!

my bad. but comment 32/33 IS windows.  

Like you, just trying to get a handle on who's what platform here, and if anyone
windows is reporting doesn't WFM.  It would help if each comment was clear on
that point ... unfortunately not everyone includes their the build string (or at
least the platform)

Comment 37

[Mike Connor [:mconnor]]

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

Comment 38

[Andreas Wuest]

(In reply to comment #37)

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

Are you sure? Maybe we are not talking about the same thing here. But the
behaviour as specified in bug 311329 has worked at least since Firefox 1.0.0 on
platform Linux.

Comment 39

[Mike Connor [:mconnor]]

part one of the bug is a dupe of this, part two is a dupe of a different bug.

Comment 40

[Wayne Mery (:wsmwk)]

vlad is there a problem with the updated patch?

Comment 41

[Pete Hammer]

Latest seamonkey build breaks even dragging to any folder on the toolbar in OSX.  Anything dragged to a toolbar folder just shows up next to the folder on the root level of the toolbar.

We're going backwards folks.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060215 SeaMonkey/1.5a

Comment 42

[Pete Hammer]

Latest seamonkey build breaks strangely just dragging to a folder on the toolbar. I dragged to a toolbar folder and the popup was in the way (center of the toolbar), so the folder highlight didn't activate.  I drop the URL anyway, and it shows up next to the folder on the root level of the toolbar.

OK, so I missed the first time, and didn't drop the favicon on the highlighted toolbar folder.  OK, now I drag it from the toolbar onto the folder next to it.  Instead of dropping into the folder, now it drops in front of it...  and the folder (which still has everything in it) now shows just a single bookmark icon instead of a folder icon.  This is why URLs won't drop onto it now...  They will still drop on the others. (Still won't open, as the original issue here).

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060215 SeaMonkey/1.5a

Comment 43

[mmortal03]

(In reply to comment #42)
> Latest seamonkey build breaks strangely just dragging to a folder on the
> toolbar. I dragged to a toolbar folder and the popup was in the way (center of
> the toolbar), so the folder highlight didn't activate.  I drop the URL anyway,
> and it shows up next to the folder on the root level of the toolbar.
> 
> OK, so I missed the first time, and didn't drop the favicon on the highlighted
> toolbar folder.  OK, now I drag it from the toolbar onto the folder next to it.
>  Instead of dropping into the folder, now it drops in front of it...  and the
> folder (which still has everything in it) now shows just a single bookmark icon
> instead of a folder icon.  This is why URLs won't drop onto it now...  They
> will still drop on the others. (Still won't open, as the original issue here).
> 
> Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060215
> SeaMonkey/1.5a
> 

Yeah, that is part of what I reported as Bug 327860.

Comment 44

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 45

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

Comment on attachment 169845 [details] [diff] [review]
Patch against current trunk v0.2

Clearing old bugs from review queue (Places taking over here)

Comment 46

[Worcester12345]

If "(Places taking over here)" is true, shouldn't "Version" be changed to trunk?

Comment 47

[Tracy Walker [:tracy]]

dnd feedback in toolbar is wfm on branches and on trunk

Comment 48

[(not reading, please use [e-mail] instead)]

> dnd feedback in toolbar is wfm on branches and on trunk

tracy, heads up, I think there is a bug logged about there not being dnd feedback on the toolbar the first time you try it.

I'm seeing that issue with bug Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070523 Minefield/3.0a5pre

Comment 49

[(not reading, please use [e-mail] instead)]

> I think there is a bug logged

tracy, see bug #338302

note, that bug is about the tab drop indicator not being there the first time, but it seems related.

I'll open another one about the bookmark personal toolbar drop indicator.

Comment 50

[(not reading, please use [e-mail] instead)]

> I'll open another one about the bookmark personal toolbar drop indicator

see bug #381817

Comment 51

[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