The bookmarks menu from the widget closes too fast for drag&drop

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mak, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {regression})

unspecified
Firefox 30
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29+ verified, firefox30 verified)

Details

(Whiteboard: [Australis:P2])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
dragging a link to the widget opens the panel, but it closes too fast when dragging the link towards the menu, it should allow more time to complete the action.
(Reporter)

Updated

5 years ago
Whiteboard: [Australis:M?][Australis:P?]
Whiteboard: [Australis:M?][Australis:P?] → [Australis:M?][Australis:P5]
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P3]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 940148
(Assignee)

Updated

4 years ago
Blocks: 872617
(Assignee)

Comment 2

4 years ago
I poked at this, and it seems that the panel appearing causes the problem; several dragleave events fire because the mouse is over items in the panel or the panel arrow, and then for some reason another dragleave event fires with the navbar customize target as its relatedTarget, so even detecting these things in the panel as 'safe' doesn't really help. :-(

This is likely to do with the other and various bugs on file about how clicking the menu sometimes activates the first item in the panel instead of just showing the panel, as well as the panel moving after having been shown (even if I can't see that on my machine, it's clearly happening if there are mouseleave events targeted at it without my having moved my mouse)
(Assignee)

Updated

4 years ago
Blocks: 941051
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P2]
(Assignee)

Comment 3

4 years ago
It's now *possible* to drag items into the menu, but on my first few attempts the menu still flickered and disappeared, so this isn't fixed yet...
(Assignee)

Comment 4

4 years ago
So I poked at this today, and even if I fix PlacesMenuDNDHandler's dragenter to cancel any dragleaves for the same node, the bookmarks popup actually closes *without* the timer (set up by dragleave) having been called (the timer's callback console.logs stuff, so I'm sure it doesn't get called).

That ends up meaning: I don't know what's actually closing the menu. :-\
(Assignee)

Comment 6

4 years ago
(In reply to Marco Bonardo [:mak] from comment #5)
> may this be involved?
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> content/menu.xml#240

Looks plausible. I'll have to have another look tomorrow.
Flags: needinfo?(mak77)
(Assignee)

Comment 7

4 years ago
Err, totally meant to needinfo myself here.
Flags: needinfo?(mak77) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 8

4 years ago
Created attachment 8370865 [details] [diff] [review]
adjust bookmarks drag code to not close the menu when over the menu button,

This fixes the issue for me. Both close menu helpers needed some adjusting, but this seems only slightly ugly, and seems to work pretty well. Note that I added a guard inside the function I added on the PlacesControllerDragHelper because I'm not sure that always lives in a browser window and can assume that the relevant API exists. It might be unnecessary.
Attachment #8370865 - Flags: review?(mak77)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 9

4 years ago
Comment on attachment 8370865 [details] [diff] [review]
adjust bookmarks drag code to not close the menu when over the menu button,

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

::: browser/base/content/browser-places.js
@@ +740,5 @@
>    _closeDelayMs: 500,
>    _loadTimer: null,
>    _closeTimer: null,
> +  _closingTimerNode: null,
> +  _overNode: null,

_overNode should not be needed, since we already have it, see below :)

@@ +756,5 @@
> +    if (this._closeTimer && this._closingTimerNode === event.currentTarget) {
> +      this._closeTimer.cancel();
> +      this._closingTimerNode = null;
> +      this._closeTimer = null;
> +    }

please add a comment above this explaining the logic behind it

::: browser/components/places/content/controller.js
@@ +1350,5 @@
>      }
>      return false;
>    },
>  
> +  draggingInAnchor: function PCDH_draggingInAnchor(node) {

I don't think we need a further method here, you should fix draggingOverChildNode to properly detect we are dragging over the node... Even if its name seems to suggest it's only for child nodes of the passed-in node, it is also checking the node itself

Actually it's possible the issue here is just that we should set PlacesControllerDragHelper.currentDropTarget in PlacesControllerDragHelper.onDragOver
To clarify, currentDropTarget is a hint we give to draggingOverChildNode, we usually set it in the Places views during dragover/drop events, and unset it onDragEnd

::: browser/components/places/content/menu.xml
@@ +238,5 @@
> +            let helper = PlacesControllerDragHelper;
> +            let notDragging = helper.getSession() &&
> +                              !helper.draggingOverChildNode(popup.parentNode);
> +            notDragging = notDragging && !helper.draggingInAnchor(popup);
> +            if (notDragging) {

I don't feel the need for this refactoring honestly.
Attachment #8370865 - Flags: review?(mak77) → review-
(Reporter)

Comment 10

4 years ago
PS: in a few words currentDropTarget is your _overNode
(Assignee)

Comment 11

4 years ago
(In reply to Marco Bonardo [:mak] from comment #10)
> PS: in a few words currentDropTarget is your _overNode

No, because it gets set to null. I checked. The popup drag handlers don't apply to the anchor.
(Assignee)

Comment 12

4 years ago
Created attachment 8371480 [details] [diff] [review]
adjust bookmarks drag code to not close the menu when over the menu button,

You were right. This is much simpler, and seems to work as well.
Attachment #8371480 - Flags: review?(mak77)
(Assignee)

Updated

4 years ago
Attachment #8370865 - Attachment is obsolete: true
(Reporter)

Comment 13

4 years ago
Comment on attachment 8371480 [details] [diff] [review]
adjust bookmarks drag code to not close the menu when over the menu button,

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

thanks

::: browser/base/content/browser-places.js
@@ +754,5 @@
>  
> +    /*
> +     * If we re-enter the same menu or anchor before the close timer runs out,
> +     * we should ensure that we do not close:
> +     */

please use // line comments here, makes easier to comment out parts of code when debugging
Attachment #8371480 - Flags: review?(mak77) → review+
(Assignee)

Comment 14

4 years ago
remote:   https://hg.mozilla.org/integration/fx-team/rev/cf6970290636
remote:   https://hg.mozilla.org/integration/fx-team/rev/1245f32928f9

:-(
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
(Assignee)

Comment 15

4 years ago
Created attachment 8371501 [details] [diff] [review]
Patch for Aurora
(Assignee)

Comment 16

4 years ago
Comment on attachment 8371501 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Dragging to the bookmarks menu doesn't work well
Testing completed (on m-c, etc.): Local
Risk to taking this patch (and alternatives if risky): relatively low. Dragging can't be much worse than it is now.
String or IDL/UUID changes made by this patch: None.
Attachment #8371501 - Flags: review+
Attachment #8371501 - Flags: approval-mozilla-aurora?
Waiting for the patch to be applied in m-c.
status-firefox29: --- → affected
tracking-firefox29: --- → +
https://hg.mozilla.org/mozilla-central/rev/cf6970290636
https://hg.mozilla.org/mozilla-central/rev/1245f32928f9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
Attachment #8371501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/02ba61d76468
status-firefox29: affected → fixed
status-firefox30: --- → fixed
Verified with latest builds on Nightly and Aurora
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
You need to log in before you can comment on or make changes to this bug.