918049 - Investigate issues with skipintoolbarset and our DnD code

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Might be 2 different issues, really:

1) Drag skipintoolbarset items onto other (non-skipintoolbarset) items
2) Drop stuff on skipintoolbarset items


In both cases, though, we're going to run into the problem that trying to find these items in our placement code isn't going to work, so finding a drop target will likely be problematic. I'm not 100% sure what the solution should be.

The no-nonsense, "buh-bye" solution would be to just hide all these items in customization mode. Their place in the document is usually managed by other code, and we should probably not let users mess with it. On the other hand, hiding them entirely means they take up no space in the toolbar, which has side-effects, etc. If they're not invisible we can drop stuff on them (even if we stop people from dragging the items themselves).

Painstakingly trying to do "something" would probably be:

1) if the item itself is skipintoolbarset, shortcircuit, don't bother finding placements or anything, just try to append it where the user dropped it.
2) if dropping on a skipintoolbarset item, find the next sibling, and drop on (i.e. before) that instead. For bonus points, insert before the skipintoolbarset item in the window where dragging happens (by the nature of these items, I think all bets are off for what happens in other windows). Note that there's a still-edgier-edgecase here when you drop an item that's right of a skipintoolbarset item on that item (the placeholder will pretend to insert to the left of the item, I think). Placement-wise, that'll make 0 difference, but if we're going here we should probably ensure that something happens or that the DnD feedback makes the fact that nothing will happen intuitive.

Opinions? Other ideas?

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I like option 2 as I think it would match what the user wanted.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hmm, and if we do show skipintoolbarset items but don't allow dragging... "Why can't I drag this?!"

And if we do show skipintoolbarset items but do allow dragging... "Why didn't that stay where I put it?!"

I don't think there is a perfect solution to this :\ Just a least-surprising solution.

Comment 3

[:Gijs (he/him)]

Attachment: deal with skipintoolbarset items better when dnd'ing,

Hrmpf. Not a giant fan of this code, but I'm fairly sure this takes care of the cases I noted in comment #0. Passes all our tests, too, and has its own test to check that we don't break this.

Comment 4

[:Gijs (he/him)]

Attachment: deal with skipintoolbarset items better when dnd'ing,

Forgot to hg add the test.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 813082 [details] [diff] [review]
deal with skipintoolbarset items better when dnd'ing,

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

::: browser/components/customizableui/test/browser_918049_skipintoolbarset_dnd.js
@@ +8,5 @@
> +  {
> +    desc: "Attempting to drag a skipintoolbarset item should work.",
> +    setup: function() {
> +
> +      navbar = document.getElementById("nav-bar");

nit: remove the blank line above.

Comment 6

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/52281082b7da

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/52281082b7da