Closed
Bug 977572
Opened 7 years ago
Closed 7 years ago
Drop indicator of new Bookmarks Widget stays forever
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
VERIFIED
FIXED
Firefox 31
People
(Reporter: alice0775, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [Australis:P3-])
Attachments
(2 files, 1 obsolete file)
36.29 KB,
image/png
|
Details | |
1.21 KB,
patch
|
mak
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
![]() |
Reporter | |
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
(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...
Assignee | ||
Comment 3•7 years ago
|
||
(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...
![]() |
Reporter | |
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 5•7 years ago
|
||
(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.
![]() |
Reporter | |
Comment 6•7 years ago
|
||
(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
![]() |
Reporter | |
Comment 7•7 years ago
|
||
s/dropdoen/dropdown/
Comment 8•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
I wonder if this is again due to the opening animation that causes dragover to happen on the popup while the menu opens
Assignee | ||
Comment 11•7 years ago
|
||
(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•7 years ago
|
||
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
Assignee | ||
Comment 13•7 years ago
|
||
I can no longer reproduce this on current trunk. Alice, can you?
Flags: needinfo?(alice0775)
![]() |
Reporter | |
Comment 14•7 years ago
|
||
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)
![]() |
Reporter | |
Comment 15•7 years ago
|
||
Screen capture: http://youtu.be/ev2JZqZlRtU
Assignee | ||
Comment 16•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 17•7 years ago
|
||
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
Assignee | ||
Comment 18•7 years ago
|
||
You were right, I believe. This seems to fix things as well, and is much less drastic.
Attachment #8404786 -
Flags: review?(mak77)
Assignee | ||
Updated•7 years ago
|
Attachment #8404198 -
Attachment is obsolete: true
Attachment #8404198 -
Flags: review?(mak77)
Assignee | ||
Comment 19•7 years ago
|
||
(and, fwiw, https://developer.mozilla.org/en-US/docs/Web/API/Node.contains is nice. Whee!)
Comment 20•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 21•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/761b5314cb26
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
Assignee | ||
Comment 22•7 years ago
|
||
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?
Comment 23•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/761b5314cb26
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 31
Assignee | ||
Updated•7 years ago
|
Updated•7 years ago
|
Attachment #8404786 -
Flags: approval-mozilla-beta?
Attachment #8404786 -
Flags: approval-mozilla-beta+
Attachment #8404786 -
Flags: approval-mozilla-aurora?
Attachment #8404786 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 24•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3617c980e9e1 https://hg.mozilla.org/releases/mozilla-beta/rev/5eb91b9f89ec
Comment 25•7 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•