Closed Bug 977572 Opened 6 years ago Closed 6 years ago

Drop indicator of new Bookmarks Widget stays forever

Categories

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

30 Branch
x86_64
Windows 7
defect
Not set
trivial

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: alice0775, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P3-])

Attachments

(2 files, 1 obsolete file)

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
Attached image screenshot
(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...
Flags: needinfo?(alice0775)
Keywords: regression
Whiteboard: [Australis:P3-]
(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...
(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...
Flags: needinfo?(alice0775)
(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.
(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
s/dropdoen/dropdown/
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.
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
I wonder if this is again due to the opening animation that causes dragover to happen on the popup while the menu opens
(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...
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
I can no longer reproduce this on current trunk. Alice, can you?
Flags: needinfo?(alice0775)
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
Flags: needinfo?(alice0775)
Screen capture: http://youtu.be/ev2JZqZlRtU
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...
Attachment #8404198 - Flags: review?(mak77)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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
You were right, I believe. This seems to fix things as well, and is much less drastic.
Attachment #8404786 - Flags: review?(mak77)
Attachment #8404198 - Attachment is obsolete: true
Attachment #8404198 - Flags: review?(mak77)
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!
Attachment #8404786 - Flags: review?(mak77) → review+
Keywords: checkin-needed
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
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
Attachment #8404786 - Flags: approval-mozilla-beta?
Attachment #8404786 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/761b5314cb26
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 31
Attachment #8404786 - Flags: approval-mozilla-beta?
Attachment #8404786 - Flags: approval-mozilla-beta+
Attachment #8404786 - Flags: approval-mozilla-aurora?
Attachment #8404786 - Flags: approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.