148996 - enable the feature of Drag and Drop on Bookmarks button on the personal toolbar

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Bolian Yin]

Bug 96504 "after dragging proxy icon onto bookmarks button, bookmarks menu
refuses to close", has disabled this feature for mozilla 1.0
This bug is re-enable this feature on trunk.

Comment 1

[Bolian Yin]

Attachment: fix the mozilla wrong behavior when gtk is dragging.

This patch is almost the same one I posted in bug #96504
(http://bugzilla.mozilla.org/attachment.cgi?id=77435&action=view), with one
line modification in JS file to enable the DnD on the Bookmark button (as
Pierre suggested in bug #96504).

Comment 2

[Bolian Yin]

Pierre, Since you once use this patch for one week and encountered no
regression, Can you give it a review?

Comment 3

[Pierre Chanial]

There are two problems with the current patch:
o when submenus overlap, the deepest menu gets selected. This is a known gtk bug
present in version 1.2.10. The good news is that it seems to be fixed in dev
gtk+ (see details in http://bugzilla.gnome.org/show_bug.cgi?id=56349 as chris
pointed to me) The bad news is that mozilla is not build with the gtk+ latest
version. Maybe it could be interesting if there were a easy way so that mozilla
builds with a patched version of gtk1.2. I am not sure that the annoyance of
this bug can justify such a change, but it would be definitely nice.
o there is still a bug that shows a funny behavior:
step to reproduce (apply attachment 86205 [details] [diff] [review] first):
- start mozilla
- drag a bookmark on the bookmark button, wait so that the menu opens
- hold down, hover on the menu
- still hold down, very slowly, move the DND pointer back to the bookmark button.

expected: nothing
current: when the mouse leaves the menupopup (exactly between the menupopup and
the toolbarbutton) DND "mode" stops. click somewhere to get rid of the menu. you
will see that the DND mouse cursor was hidden by the menu and is still there.
click on the cursor: DND "mode" is reactivated and the DND continues *mouse up*

I think that this patch could go into the trunk under the condition that someone
is working on the problem I have described. For the moment, I have no time to
dedicate on this, and I will need to learn how gtk works. Bolian, could you
investigate this problem?

I am not entitled to review gtk patches at the moment. The only thing I can say
is that I used it for more than one month and even under heavy testing, it does
not cause any regression.
But looking at the patch I wonder if it is necessary to define the method
DragInProgress. Isn't it tracked elsewhere? (just a thought, I haven't looked by
myself)
There are some extra " " blanks that should be removed after the "(" in if
statements, also.

Comment 4

[Pierre Chanial]

cc'ing blizzard and bryner for their inputs/comments/reviews.

Comment 5

[Bolian Yin]

Attachment: patch v2 (fix drag stop problem found by Pierre)

Comment 6

[Bolian Yin]

the patch v2 (attachment 86779 [details] [diff] [review]) use a more simple and accurate way in 
nsWindow::DragInProgress().  the new one also also fix the funny behavior
problem found by Pierre in the old patch (see comment #3).

Comment 7

[Brian Ryner (not reading)]

Patch looks good overall.  A couple of small comments:

- you may as well remove the isPlatformNotSupported property and the check for
that property entirely, if we have this working on all platforms.

- dragging is misspelled as "draging" several places in nsWindow.cpp and nsWindow.h.

Comment 8

[Bolian Yin]

Attachment: patch v3 (merge Brian's comments)

Brian, Thanks. the patch v3 removes the "isPlatformNotSupported" property, and
correts the misspelling.

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 86781 [details] [diff] [review]
patch v3 (merge Brian's comments)

r=bryner.  Please get blizzard to sr=.

Comment 10

[Pierre Chanial]

I tested attachment 86781 [details] [diff] [review] with the tip: excellent, Bolian!!! It works perfectly!

I tried to see if your patch made also possible the reordering of bookmarks
inside an open menupopup. I could do so with the first version of your patch.
Unfortunately, I hit another grabbing problem and I had to cancel by pressing ESC.
To enable dragging from inside an open menupopup you just have to replace
- navigator.platform != "Win32"
+ navigator.platform.indexOf("Mac") != -1
in
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigatorDD.js#233

This is only a side comment for future work, it should obviously not prevent the
current patch from being checked in.

Comment 11

[Bolian Yin]

Path detail:
In normal case, when the menu popup, mozilla will make a grab.  But this cause
problem when a gtk drag is in progress, the patch disable the mozilla menu grab
in this peculiar case.

blizzard, this patch has been waiting for so long a time, can you review it?

seek sr=

Comment 12

[Christopher Blizzard (:blizzard)]

Comment on attachment 86781 [details] [diff] [review]
patch v3 (merge Brian's comments)

This patch needs to be re-merged with the tip but it should work.

sr=blizzard

(yay!)

Comment 13

[Pierre Chanial]

We should not get rid of isPlatFormNotSupported:
Mac platform has a completely broken drag into folder feature (bug 136524).
Other exotic platforms could need this option, too.
Though, I fear that disabling this feature on Mac need to be reviewed by other
people but this patch should not wait any longer. Bolian, I suggest that you
simply leave |isPlatFormNotSupported: false| and I will update it for Mac in the
former bug.

Comment 14

[Bolian Yin]

Pierre, This bug will not be checked into trunk until the trunk is open for
Mozilla 1.2 development. But as you said, this feature will not work on Mac. 
So, how about you update |isPlatFormNotSupported| first (you should include
Linux in it, of course), and then I checked in this patch will the remove of
"linux" from |isPlatFormNotSupported|.  Is that work for you?  Thanks.

Comment 15

[Pierre Chanial]

Ok, thought I am not sure if the fix I will attach in bug 136524 will be check
in first.

Comment 16

[Bolian Yin]

Attachment: sync with trunk

Comment 17

[Bolian Yin]

checked in trunk.