334244 - Click-drag-release on bookmarks toolbar folder drags the folder

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Jesse Ruderman]

Firefox 2006-04-16 trunk on Mac.

Steps to reproduce:
1. Mouse down on a folder on the bookmarks toolbar.
2. Move the cursor down, as if you were intending to mouse up on an item in the menu.

Result: The folder is dragged.

See also bug 235244, where some people suggest that these steps should result in a drag while holding Cmd (Mac) or Alt/Shift (Windows/Linux) but not otherwise.  IIRC, before Places landed, Firefox on Windows let you drag with either Alt or Shift (Alt because it was standard on Windows, Shift because Alt+click has special meanings to some Linux window managers).

Worked correctly in 2006-04-10 trunk (6 days ago).  So this is a recent regression, maybe from bug 332873.

Comment 1

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

not a branch blocker

Comment 3

[Jo Hermans]

I just noticed this in a Minefield/3.0a5pre build (20070521), which is the first I tried after the Places got switched on again (older Minefield was 20070416). I remember this bug from before (when Places was enabled last year), but forgot about it when Places was disabled again. And now it seems to be back. I'm sure it wasn't present pre-Plavces, and neither in the latest Bon Echo build (2.0 branch).

Since I always use the folders on the toolbar as pulldown menu (click-drag downwards), I always end up dragging the folder into its own pulldown menu (which is impossible ofcourse). Very annoying. It generates this error in the console, maybe it helps :

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.moveFolder]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/controller.js :: PMFT_doTransaction :: line 1745"  data: no]

I know that is a different bug, but if we won't allow click-drag to move a folder (horizontally or vertically), it won't happen anymore.

Comment 5

[Nickolay_Ponomarev]

OS -> All, this also applies to Windows.

Comment 6

[Steve Won (stevewon on irc)]

Attachment: Patch

So I am checking if the event.target is a toolbarbutton and is a type of menu in the beginning of onDragStart... I am currently returning false there, but the patch also seems to work by returning true. Does the return value from onDragStart actually have much meaning here?

Comment 7

[Steve Won (stevewon on irc)]

Attachment: Patch.

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> Created an attachment (id=271236) [details]
> Patch
> 
> So I am checking if the event.target is a toolbarbutton and is a type of menu
> in the beginning of onDragStart... I am currently returning false there, but
> the patch also seems to work by returning true. Does the return value from
> onDragStart actually have much meaning here? 
> 

the other implementations in the tree that i looked at do not return a value.

Comment 9

[Steve Won (stevewon on irc)]

Attachment: "return false;" fixed to "return"

Returns nothing now, instead of false.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 271243 [details] [diff] [review]
"return false;" fixed to "return"

this will break the alt+drag scenario that's in the #ifdef, as we'd never get that far.

Comment 11

[Steve Won (stevewon on irc)]

Attachment: checkpt

Need to be tested on XP first. Not ready for review yet.

Comment 12

[Steve Won (stevewon on irc)]

Attachment: checkpt2

Comment 13

[Steve Won (stevewon on irc)]

Attachment: Patch

Thanks Seth for testing this on XP.

Comment 14

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

yes, tested checkpt2 on windows xp, and it looks good.  the bug is fixed, and
alt+click or ctrl+click still work.

one issue I see (with and without swon's patch) is that I don't think we should
be opening the folders on alt+click or ctrl+click.  

spun off to bug #387173

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 271280 [details] [diff] [review]
Patch

r=me, thanks!

Comment 16

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

since I tested it, it was in my local tree, so I've landed it for Steve.

Checking in toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.x
ml
new revision: 1.97; previous revision: 1.96
done

Comment 17

[Tracy Walker [:tracy]]

Verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071805 Minefield/3.0a7pre

Comment 18

[Christine Yen]

Possible regression? See bug 389914

Comment 19

[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