977572 - Drop indicator of new Bookmarks Widget stays forever

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, trivial)

Description

[Alice0775 White]

STR
1. Drag a link over Bookmarks Widget
2-a. Drop the link on Bookmarks Widget
2-b. Press ESC to cancel D&D
2-c. Drop the link on out side of Bookmarks Widget
3. Open Bookmarks Widget

Actual Results:
Drop indicator stays

Expected Results:
Drop indicator should vanish

Comment 1

[Alice0775 White]

Attachment: screenshot

Comment 2

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #0)
> STR
> 1. Drag a link over Bookmarks Widget
> 2-a. Drop the link on Bookmarks Widget
> 2-b. Press ESC to cancel D&D
> 2-c. Drop the link on out side of Bookmarks Widget
> 3. Open Bookmarks Widget
> 
> Actual Results:
> Drop indicator stays
> 
> Expected Results:
> Drop indicator should vanish

AIUI, this does not happen if you drag over the inside of the widget's panel before doing any of 2a/2b/2c, is that right?

I suspect this is caused by my changes to make the panel not close too quickly when dragging to it...

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> I suspect this is caused by my changes to make the panel not close too
> quickly when dragging to it...

Hmm, actually, either that or the pointerEvents change...

Comment 4

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #2)

> AIUI, this does not happen if you drag over the inside of the widget's panel
> before doing any of 2a/2b/2c, is that right?
> 

Yes, that is.

I can see drop indicator since Australis merge day...

Comment 5

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #4)
> (In reply to :Gijs Kruitbosch from comment #2)
> 
> > AIUI, this does not happen if you drag over the inside of the widget's panel
> > before doing any of 2a/2b/2c, is that right?
> > 
> 
> Yes, that is.
> 
> I can see drop indicator since Australis merge day...

This is odd. Maybe I don't understand correctly. Before bug 899530 was fixed (which was long after Australis merge day), the widget wouldn't stay open for very long if you just hovered the dropmarker (icon next to the star). Are you saying if you wait for it to close and then drop it / hit escape, there's a permanent drop indicator inside the widget when you then open the widget, even on the builds before bug 899530 was fixed? That would really surprise me.

Comment 6

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Alice0775 White from comment #4)
> > (In reply to :Gijs Kruitbosch from comment #2)
> > 
> > > AIUI, this does not happen if you drag over the inside of the widget's panel
> > > before doing any of 2a/2b/2c, is that right?
> > > 
> > 
> > Yes, that is.
> > 
> > I can see drop indicator since Australis merge day...
> 
> This is odd. Maybe I don't understand correctly. Before bug 899530 was fixed
> (which was long after Australis merge day), the widget wouldn't stay open
> for very long if you just hovered the dropmarker (icon next to the star).
> Are you saying if you wait for it to close and then drop it / hit escape,
> there's a permanent drop indicator inside the widget when you then open the
> widget, even on the builds before bug 899530 was fixed? That would really
> surprise me.

STR(on the builds before bug 899530)
1. Drag a link to the widget (icon next to the star)
2. Wait to open dropdown ( slightly difficult because the dropdoen close to fast )
3. Dragover the drop down to show the drop indicator
4. Drag horizontally and drop outside of the drop down 
5. Open the widget

Comment 7

[Alice0775 White]

s/dropdoen/dropdown/

Comment 8

[Marco Bonardo [::mak]]

the marker should be hidden when the drag ends by _cleanupDragDetails

btw I don't understand how can you drop link outside the widget if you just cleared D&D with ESC, you should not be dragging anything anymore.

Comment 9

[Marco Bonardo [::mak]]

ah the problem seems the fact the dragend handler we invoke is relative to the button itself, but the ones clearing the indicator are relative to the popup
When dragging over the button we should not unhide the indicator

Comment 10

[Marco Bonardo [::mak]]

I wonder if this is again due to the opening animation that causes dragover to happen on the popup while the menu opens

Comment 11

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #10)
> I wonder if this is again due to the opening animation that causes dragover
> to happen on the popup while the menu opens

I would have thought that stopping pointerEvents would have stopped that...

Comment 12

[Marco Bonardo [::mak]]

could be, I didn't test whether that's related, was just a guess. Surely a dragover is happening on the popup since we unhide the indicator on it

Comment 13

[:Gijs (he/him)]

I can no longer reproduce this on current trunk. Alice, can you?

Comment 14

[Alice0775 White]

I can still reproduce the problem in Nightly31.0a1.
https://hg.mozilla.org/mozilla-central/rev/5811efc11011
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140409030203

Comment 15

[Alice0775 White]

Screen capture: http://youtu.be/ev2JZqZlRtU

Comment 16

[:Gijs (he/him)]

Attachment: catch drag end and drop events from bookmarks drag handler so we can clean up,

Here's what I made of this. This seems to fix things for me. Marco, does this look OK? I'm using the system event listeners because various drag handlers have a tendency of calling stoppropagation/preventDefault, which means a 'normal' event listener might not get called in some of the more creative cases...

Comment 17

[Marco Bonardo [::mak]]

Off-hand, it looks a little bit excessive to use system listeners here... though let's see...
if we get a dragover, I'd expect we'd also get a dragLeave/dragExit
My suspect is that this code isn't working
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#459

458         let target = event.relatedTarget;
459         if (!target)
460           this._indicatorBar.hidden = true;

in this case it's possible relatedTarget is a valid target, just it's not a descendant of the popup? I wonder if:
1. this code is indeed executed but the if makes us skip hiding the indicatorbar
2. the if can just be improved to hide in any case we are dragging out of the current popup

Comment 18

[:Gijs (he/him)]

Attachment: catch drag end and drop events from bookmarks drag handler so we can clean up,

You were right, I believe. This seems to fix things as well, and is much less drastic.

Comment 19

[:Gijs (he/him)]

(and, fwiw, https://developer.mozilla.org/en-US/docs/Web/API/Node.contains is nice. Whee!)

Comment 20

[Marco Bonardo [::mak]]

Comment on attachment 8404786 [details] [diff] [review]
catch drag end and drop events from bookmarks drag handler so we can clean up,

Review of attachment 8404786 [details] [diff] [review]:
-----------------------------------------------------------------

it looks ok, and yes, contains is nice!

Comment 21

[Matthew N. [:MattN] (PM me if requests are blocking you)]

https://hg.mozilla.org/integration/fx-team/rev/761b5314cb26

Comment 22

[:Gijs (he/him)]

Comment on attachment 8404786 [details] [diff] [review]
catch drag end and drop events from bookmarks drag handler so we can clean up,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' bookmarks widget
User impact if declined: drag and drop placeholder sometimes hangs around when dragging to the bookmarks button's dropmarker menu
Testing completed (on m-c, etc.): (soon) on m-c
Risk to taking this patch (and alternatives if risky): very small patch, very low risk
String or IDL/UUID changes made by this patch: none

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/761b5314cb26

Comment 24

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3617c980e9e1
https://hg.mozilla.org/releases/mozilla-beta/rev/5eb91b9f89ec

Comment 25

[Camelia Badau [:cbadau], Release Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Verified as fixed on Windows 7 x86-64 using Firefox 29.0b8 and the 04/15 Firefox 30.0a2 and 31.0a1.