Closed
Bug 148996
Opened 22 years ago
Closed 22 years ago
enable the feature of Drag and Drop on Bookmarks button on the personal toolbar
Categories
(Core :: DOM: Copy & Paste and Drag & Drop, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: yinbolian, Assigned: yinbolian)
Details
(Whiteboard: need sr=)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
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).
Assignee | ||
Comment 2•22 years ago
|
||
Pierre, Since you once use this patch for one week and encountered no
regression, Can you give it a review?
Status: NEW → ASSIGNED
Comment 3•22 years ago
|
||
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•22 years ago
|
||
cc'ing blizzard and bryner for their inputs/comments/reviews.
Assignee | ||
Comment 5•22 years ago
|
||
Attachment #86205 -
Attachment is obsolete: true
Assignee | ||
Comment 6•22 years ago
|
||
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).
Whiteboard: need r=
Comment 7•22 years ago
|
||
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.
Assignee | ||
Comment 8•22 years ago
|
||
Brian, Thanks. the patch v3 removes the "isPlatformNotSupported" property, and
correts the misspelling.
Attachment #86779 -
Attachment is obsolete: true
Comment 9•22 years ago
|
||
Comment on attachment 86781 [details] [diff] [review]
patch v3 (merge Brian's comments)
r=bryner. Please get blizzard to sr=.
Attachment #86781 -
Flags: review+
Comment 10•22 years ago
|
||
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.
Assignee | ||
Comment 11•22 years ago
|
||
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=
Whiteboard: need r= → need sr=
Comment 12•22 years ago
|
||
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!)
Attachment #86781 -
Flags: superreview+
Comment 13•22 years ago
|
||
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.
Assignee | ||
Comment 14•22 years ago
|
||
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•22 years ago
|
||
Ok, thought I am not sure if the fix I will attach in bug 136524 will be check
in first.
Assignee | ||
Comment 16•22 years ago
|
||
Attachment #86781 -
Attachment is obsolete: true
Assignee | ||
Comment 17•22 years ago
|
||
checked in trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•