420341 - Drag and drop link/bookmark issues, regressed by bug 389931

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022702 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Start to drag a bookmark within a folder. (Bookmarks menu, or Personal Toolbar)
Soon, the bookmark tooltip appears.

1) The tooltip is unwanted.
2) I think the underlying bug prevents bug 194319 from working too.

I guess this SeaMonkey bug is the "equivalent" of Firefox bug 419740 !

***

Another symptom:
trying to drag a link from the content area to a folder of the PT will, "99%" of the time, NOT open the folder.
That is major !

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> trying to drag a link from the content area to a folder of the PT will, "99%"
> of the time, NOT open the folder.
> That is major !

To compare with FF:

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022604 Minefield/3.0b4pre] (nightly) (W2Ksp4)

has a "similar" bug.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre] (nightly) (W2Ksp4)

Does not have this bug anymore.

Comment 2

[Martijn Wargers (dead)]

I think that could be fixed by a somewhat different fix for bug 301482 (see comment there).

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> I think that could be fixed by a somewhat different fix for bug 301482 (see
> comment there).

Agreeing that the tooltip issue "is" bug 301482.
Let's forget it here.

*****

I'm not 100% sure that this is the cause of the current bug,
but I think this should be done (anyway):

Back out the following lines (and related blocks):
<http://mxr.mozilla.org/mozilla/search?string=isTimerSupported>
which includes backing out bug 232795.

As I understand it |isTimerSupported| (from <bookmarksMenu.js> v1.1) was a workaround,
which bug 389931 "obsoletes".

Should I try and do that ?

Comment 4

[Martijn Wargers (dead)]

Yes, afaik, all the isTimerSupported stuff can be removed now, since all 3 platforms now support timers while dragging something.
If you could whip up a patch for that, that would be great.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <bookmarksMenu.js> ("-w")

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022702 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

This patch fixes the PT folders not opening.
NB: I'll do the code vertical alignment before checkin.

After this patch, we get a bug 419740 like behavior :-/
I would like a hint to fix this ... See FF bug 418156.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> After this patch, we get a bug 419740 like behavior :-/

It seems the menu disappears only when hovering the unwanted tooltip;
so, I think fixing bug 301482 could indirectly fix this too.

> I would like a hint to fix this ... See FF bug 418156.

I tried to remove the lines
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/browser/navigator.xul&rev=1.457#275>
<http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/browser/navigatorOverlay.xul&rev=1.334#485>
It mostly worked, but I got side effects: a case of menupopups not disappearing immediately, "dropmarker(s)" staying forever on the PT, ...

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 306746 [details] [diff] [review]
(Av1) <bookmarksMenu.js> ("-w")

(Looking for r+sr.)

Comment 9

[[e-mail]]

Comment on attachment 306746 [details] [diff] [review]
(Av1) <bookmarksMenu.js> ("-w")

On Windows this certainly seems to help but my Linux build won't behave with or without the patch nor do I have a Mac so I'm adding more requests.

Comment 10

[[e-mail]]

Comment on attachment 306746 [details] [diff] [review]
(Av1) <bookmarksMenu.js> ("-w")

>-    if (this.isPlatformNotSupported)
>-      return;
Did you mean this? Or perhaps we should "support" all platforms now?

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> (From update of attachment 306746 [details] [diff] [review])
> On Windows this certainly seems to help but my Linux build won't behave with or
> without the patch nor do I have a Mac so I'm adding more requests.

This very patch is a Windows only "cleanup":
it should have no impact on Linux or MacOSX.

Furthermore, this patch is not intended to fully solve this bug (even if I had hoped so):
I see it as a prerequisite / first step.

(In reply to comment #10)
> (From update of attachment 306746 [details] [diff] [review])
> >-    if (this.isPlatformNotSupported)
> >-      return;
> Did you mean this? Or perhaps we should "support" all platforms now?

Yes, I did: because this |return| becomes useless once there's no code after it.

If you mean removing |isPlatformNotSupported: navigator.platform.indexOf("Mac") != -1, // see bug 136524| and all its uses,
that would be a separate patch, to be made/tested by a MacOSX "user".

Comment 12

[Stefan [:stefanh]]

If this doesn't do anything on mac, the only thing I can do here is to check if it *really* doesn't do anything on mac :-)

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> If this doesn't do anything on mac, the only thing I can do here is to check if
> it *really* doesn't do anything on mac :-)

WRT this patch, yes.

_After_ this check, could you try and test with |isPlatformNotSupported: false| too ?

Comment 14

[Andrew Schultz]

> Did you mean this? Or perhaps we should "support" all platforms now?

See

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/common/bookmarks/bookmarksMenu.js&rev=1.32&mark=367,368#358

With the patch applied (and that bit removed), bookmark DND within the menu does work, but the menu does close once I drop (which, if I read comment 0 correctly, is what this patch is supposed to fix).

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)
> > Did you mean this? Or perhaps we should "support" all platforms now?
> 
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/common/bookmarks/bookmarksMenu.js&rev=1.32&mark=367,368#358

No, my patch "is" about the next lines:
{{
369                     // c) on Windows, there is no hang or crash associated with this, so we'll leave 
370                     // the functionality there. 
}}

But I agree your link might be related to |isPlatformNotSupported| (Mac test).
(On Linux, someone would have to check if current X-server still crash.)

> With the patch applied (and that bit removed), bookmark DND within the menu
> does work, but the menu does close once I drop (which, if I read comment 0
> correctly, is what this patch is supposed to fix).

I'm not sure what and how you tested...

My (Windows specific) patch (only) fixes:

(In reply to comment #0)
> Another symptom:
> trying to drag a link from the content area to a folder of the PT will, "99%"
> of the time, NOT open the folder.
> That is major !

and

(In reply to comment #6)
> Created an attachment (id=306746) [details]
> 
> This patch fixes the PT folders not opening.

Comment 16

[Andrew Schultz]

I was testing on linux.

Comment 17

[Stefan [:stefanh]]

Comment on attachment 306746 [details] [diff] [review]
(Av1) <bookmarksMenu.js> ("-w")

If I understood right, this patch should fix so a folder on the pt opens when you drag a link over it on windows. In that case, I can confirm that this doesn't seem to change anything on mac (the folder still doesn't  open). That's what the r+ is for. I also tried setting isPlatformNotSupported to false, and it actually seems to fix the issue on mac. I created an empty folder on the pt, then I dragged links to it. The folder opened, and the menu opened. I could then drop the link in a specific place in the menu. Could there be any other issues with this that I missed?

Comment 18

[Stefan [:stefanh]]

(In reply to comment #17)
> (From update of attachment 306746 [details] [diff] [review])
> If I understood right, this patch should fix so a folder on the pt opens when
> you drag a link over it on windows. In that case, I can confirm that this
> doesn't seem to change anything on mac (the folder still doesn't  open). That's
> what the r+ is for. I also tried setting isPlatformNotSupported to false, and
> it actually seems to fix the issue on mac. I created an empty folder on the pt,

> then drop the link in a specific place in the menu.

erm, I did had a few items in the folder by that time. 

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> I was testing on linux.

Then,
My patch should make no difference, on Linux.
See my bug 151336 comment 7 about (Linux bypassing) that other line ;-)

Comment 20

[[e-mail]]

Comment on attachment 306746 [details] [diff] [review]
(Av1) <bookmarksMenu.js> ("-w")

OK, this patch looks good, but would you mind doing another patch that includes isTimerSupported too?

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> (From update of attachment 306746 [details] [diff] [review])
> If I understood right,

You did.

> doesn't seem to change anything on mac (the folder still doesn't  open).

Could you check and comment on bug 151336, about "Mac bypassing" that other line too ?
(Would it work ? Would that behavior be wanted ?)

> I also tried setting isPlatformNotSupported to false, and
> it actually seems to fix the issue on mac.

Do you want me to submit a patch to remove this variable (and related code) ?
(== Would that new behavior be wanted ?)

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> (From update of attachment 306746 [details] [diff] [review])
> would you mind doing another patch that includes isTimerSupported too?

Did you mean "removes |isPlatformNotSupported|" too ?

Comment 23

[[e-mail]]

Sorry, yes, I confused the two variables. (Another good reason to ditch them!)

Comment 24

[Stefan [:stefanh]]

(In reply to comment #21)
> (In reply to comment #17)
> > (From update of attachment 306746 [details] [diff] [review] [details])
> > If I understood right,
> 
> You did.
> 
> > doesn't seem to change anything on mac (the folder still doesn't  open).
> 
> Could you check and comment on bug 151336, about "Mac bypassing" that other
> line too ?
> (Would it work ? Would that behavior be wanted ?)

I haven't checked, but it might work. Seems that mac Minefield suport this.
> 
> > I also tried setting isPlatformNotSupported to false, and
> > it actually seems to fix the issue on mac.
> 
> Do you want me to submit a patch to remove this variable (and related code) ?
> (== Would that new behavior be wanted ?)
>

Yeah.

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-w) <bookmarksMenu.js>

Av1 ("-w"), with comment 20 suggestion:

*Removes |isTimerSupported|, as (first) bug 389931 Windows regression fix.
*Removes |isPlatformNotSupported|, as MacOSX feature enabling, per current manual test result.
*(NB: I'll do the code vertical alignment before checkin.)

NB: WRT full platform parity, see bug 151336 about the last remaining check...

Comment 26

[[e-mail]]

Comment on attachment 308650 [details] [diff] [review]
(Bv1-w) <bookmarksMenu.js>

(Note: you can request multiple reviews when attaching, simply separate the addresses with commas.)

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <bookmarksMenu.js> [Checkin: Comment 30]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031101 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bv1-w, with related whitespace nits, plus a few code reorderings.

Comment 28

[Stefan [:stefanh]]

Serge,
The check-in keyword means that someone not cc:ed on this bug might check in the patch for you. If it was me, I might have checked in attachment #308650 [details] [diff] [review] - that's the only non-obsoleted attachment that has r+sr (I wouldn't check in anything that wasn't r+sr:ed and maybe I was in a hurry...).

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)

What you write makes sense: that's the "how to" issue with "-w" patches.

Yet, that 'someone' usually is reed nowadays, and we/he is used to process it that way (using my |[c-n: Bv1]| whiteboard hint) ;-)

Comment 30

[Reed Loden [:reed]]

Checking in suite/common/bookmarks/bookmarksMenu.js;
/cvsroot/mozilla/suite/common/bookmarks/bookmarksMenu.js,v  <--  bookmarksMenu.js
new revision: 1.33; previous revision: 1.32
done

Comment 31

[Reed Loden [:reed]]

(In reply to comment #29)
> What you write makes sense: that's the "how to" issue with "-w" patches.

Yeah, if I see a "-w" patch and a normal patch, it's assumed that the normal patch is what gets landed.

> Yet, that 'someone' usually is reed nowadays, and we/he is used to process it
> that way (using my |[c-n: Bv1]| whiteboard hint) ;-)

Very true. :)

Comment 32

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031301 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed, as far as what Bv1 was supposed to unregress from bug 389931.

Comment 33

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <bookmarksMenu.js> (Don't Close Target On Drop)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031700 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

1) Bv1 followup:
Now that we have full platform parity, (after resolving bug 151336),
let's add (back) a feature which used to be included in the Windows timer hack (removed by my Bv1 patch) only...
See
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/suite/common/bookmarks/bookmarksMenu.js&rev=1.32&mark=656-665#641>

2) unrelated cleanup:
Removes empty and unused |onDropSetFeedBack()|, per
<http://mxr.mozilla.org/seamonkey/search?string=onDropSetFeedBack&case=on&tree=seamonkey>

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) <bookmarksMenu.js> (Don't Close Target On Drop)

Cv1, but with an early return.

Comment 35

[[e-mail]]

Comment on attachment 310239 [details] [diff] [review]
(Cv1a) <bookmarksMenu.js> (Don't Close Target On Drop)

>+  dontCloseTargetOnDrop : false,
I think this would be better named mDidDrop. sr=me with this fixed.

Comment 36

[Stefan [:stefanh]]

Comment on attachment 310239 [details] [diff] [review]
(Cv1a) <bookmarksMenu.js> (Don't Close Target On Drop)

Testing this patch reminds me that bringing up context menus in menus feels evil. But that has nothing to with this patch, of course...

Comment 37

[Andrew Schultz]

Attachment: stacks

I'm seeing 2 calls to onDragExitSetTimer as shown in the attachment.  This patch prevents the first but not the second.

Comment 38

[Andrew Schultz]

Comment on attachment 311110 [details]
stacks

>#0: function anonymous(aDragSession=XPComponent:{21}, aTarget=XULElement:{0}) in <chrome://communicator/content/bookmarks/bookmarksMenu.js> line 614
>#1: function anonymous(aDragSession=XPComponent:{21}, aEvent=MouseEvent:{0}) in <chrome://communicator/content/bookmarks/bookmarksMenu.js> line 416
>#2: function anonymous(aDragDropObserver=Object:{25}, aEvent=MouseEvent:{0}) in <chrome://global/content/nsDragAndDrop.js> line 484
>#3: function ondragexit(event=MouseEvent:{0}) in <chrome://navigator/content/navigator.xul> line 1

ugh.  Actually, it's this same stack twice

Comment 39

[Andrew Schultz]

Attachment: native stack

This is the first call into OnDragExitSetTimer.  The top of the stack is calling into venkman where it hit my breakpoint.

The second stack is identical except that 

#43 0x018ebb07 in nsCommonWidget::DispatchEvent (this=0xa205be8, aEvent=0xbfdd18cc, aStatus=@0xbfdd1920) at /build/andrew/moz-debug/mozilla/widget/src/gtk2/nsCommonWidget.cpp:158
#44 0x018dade6 in nsWindow::OnDragDropEvent (this=0xa205be8, aWidget=0xa07e148, aDragContext=0xa141e98, aX=168, aY=639, aTime=3618233147, aData=0x0) at
/build/andrew/moz-debug/mozilla/widget/src/gtk2/nsWindow.cpp:2871 

becomes

#40 0x018ebb07 in nsCommonWidget::DispatchEvent (this=0xa205be8, aEvent=0xbfdd1824, aStatus=@0xbfdd1880)
    at /build/andrew/moz-debug/mozilla/widget/src/gtk2/nsCommonWidget.cpp:158
#41 0x018da3d6 in nsWindow::OnDragLeave (this=0xa205be8)
    at /build/andrew/moz-debug/mozilla/widget/src/gtk2/nsWindow.cpp:2924
#42 0x018dae44 in nsWindow::OnDragDropEvent (this=0xa205be8, aWidget=0xa07e148, aDragContext=0xa299000, aX=237, aY=685, aTime=3618343593, aData=0x0)
    at /build/andrew/moz-debug/mozilla/widget/src/gtk2/nsWindow.cpp:2885

so the stacks fork at
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.265&mark=2871,2885#2862

and PresShell::HandleEventInternal calls nsEventDispatcher::Dispatch directly (line 5945), so frames 34-36 are missing.

Comment 40

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> (From update of attachment 310239 [details] [diff] [review])
> >+  dontCloseTargetOnDrop : false,
> I think this would be better named mDidDrop. sr=me with this fixed.

Neil,
Just noticed this: what about merging
*|onDragEnterSetTimer()| into |onDragEnter()|.
*|onDragExitSetTimer()| into |onDragExit()|.
as they are their only callers ?

*****

(In reply to comment #37)
> Created an attachment (id=311110) [details]
> stacks
> 
> I'm seeing 2 calls to onDragExitSetTimer as shown in the attachment.  This
> patch prevents the first but not the second.

Andrew,
This means |onDragExit()| is called twice.
But other than that, well ... Should that be filed as a "separate" Linux only bug ?
(I don't know what else I could do here.)

Comment 41

[Andrew Schultz]

Attachment: patch that makes linux happy

> But other than that, well ... Should that be filed as a "separate" Linux only
> bug ?
> (I don't know what else I could do here.)

Right.  Neil and I discussed this yesterday and came up with this workaround.  This should behave like the last patch except when onDragExit gets called more than once.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 311270 [details] [diff] [review]
patch that makes linux happy

Thanks.
This will eventually not be needed, now that bug 424684 is fixed.

Comment 43

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1b) <bookmarksMenu.js> (Don't Close Target On Drop) [Checkin: Comment 45]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041602 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Cv1a, with my comment 40 suggestion(s).

Keeping: "stefanh: review+".

Comment 44

[Andrew Schultz]

Comment on attachment 316121 [details] [diff] [review]
(Cv1b) <bookmarksMenu.js> (Don't Close Target On Drop)
[Checkin: Comment 45]

looks OK to me (and works on linux)

Comment 45

[Karsten Düsterloh]

Comment on attachment 316121 [details] [diff] [review]
(Cv1b) <bookmarksMenu.js> (Don't Close Target On Drop)
[Checkin: Comment 45]

Landed on trunk.

Comment 46

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041902 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed