Closed Bug 918049 Opened 11 years ago Closed 11 years ago

Investigate issues with skipintoolbarset and our DnD code


(Firefox :: Toolbars and Customization, defect)

Not set



Firefox 28


(Reporter: Gijs, Assigned: Gijs)


(Blocks 1 open bug)


(Whiteboard: [Australis:P3][AustralisM9])


(1 file, 1 obsolete file)

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?
I like option 2 as I think it would match what the user wanted.
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.
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.
Attachment #813081 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Forgot to hg add the test.
Attachment #813082 - Flags: review?(jaws)
Attachment #813081 - Attachment is obsolete: true
Attachment #813081 - Flags: review?(jaws)
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.
Attachment #813082 - Flags: review?(jaws) → review+
Whiteboard: [Australis:P3][AustralisM?] → [Australis:P3][AustralisM9][fixed-in-ux]
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][AustralisM9][fixed-in-ux] → [Australis:P3][AustralisM9]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.