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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 2

[:Gijs (he/him)]

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)

Comment 3

[:Gijs (he/him)]

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...

Comment 4

[:Gijs (he/him)]

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. :-\

Comment 5

[Marco Bonardo [:mak]]

may this be involved? http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/menu.xml#240

Comment 6

[:Gijs (he/him)]

(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.

Comment 7

[:Gijs (he/him)]

Err, totally meant to needinfo myself here.

Comment 8

[:Gijs (he/him)]

Attachment: 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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Bonardo [:mak]]

PS: in a few words currentDropTarget is your _overNode

Comment 11

[:Gijs (he/him)]

(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.

Comment 12

[:Gijs (he/him)]

Attachment: 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.

Comment 13

[Marco Bonardo [:mak]]

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

Comment 14

[:Gijs (he/him)]

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

:-(

Comment 15

[:Gijs (he/him)]

Attachment: Patch for Aurora

Comment 16

[:Gijs (he/him)]

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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Waiting for the patch to be applied in m-c.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf6970290636
https://hg.mozilla.org/mozilla-central/rev/1245f32928f9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/02ba61d76468

Comment 20

[Tracy Walker [:tracy]]

Verified with latest builds on Nightly and Aurora