Jared Wein [:jaws] (please needinfo? me) Reporter
	Description • 11 years ago

STR:

[X  X  X]
[X  X  X]
[X][X][X]
[X][X][X]
[X][X][X]

Dragging Y on top of the second [X  X  X], should produce the following:

[X  X  X]
[Y][X][X]
[X  X  X]
[X][X][X]
[X][X][X]
[X]

Actual:
[X  X  X]
[X  X  X]
[Y][X][X]
[X][X][X]
[X][X][X]
[X]

	:Gijs (he/him) Assignee
	Comment 1 • 11 years ago

I've been poking at this locally, working on a patch, but depends on bug 876926.

	:Gijs (he/him) Assignee
	Comment 2 • 11 years ago

So this patch became a bit bigger than I anticipated. Essentially, I've moved the tracking of wide widgets to its own file. This means it only happens once for each wide widget affected by the addition/moving/removal of an item. While I was at it, I've also fixed dragging small items onto wide widgets, which is what this bug is about. I've tried to comment in order to clarify stuff, but please let me know if there's stuff that isn't clear. Separately, I should probably add tests to check the showInPrivateBrowsing stuff I added.

	:Gijs (he/him) Assignee
	Comment 3 • 11 years ago

Forgot that I'd made a small change to the test to make it run faster, so this didn't apply to trunk cleanly. Fixed here.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 4 • 11 years ago

Comment on attachment 806217 [details] [diff] [review]
Narrow widgets dropped on a wide widget should place the narrow widget above the wide widget,

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

::: browser/components/customizableui/src/PanelWideWidgetTracker.jsm
@@ +155,5 @@
> +  },
> +  trackWidget: function(aWidgetId) {
> +    gWideWidgets.add(aWidgetId);
> +  },
> +  untrackWidget: function(aWidgetId) {

trackWidget and untrackWidget aren't referenced elsewhere. Are these unnecessary now?

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ -156,4 @@
>        simulateItemDrag(developerButton, zoomControls);
>        // Currently, the developer-button is placed after the zoom-controls, but it should be
>        // placed like expectedPlacementsAfterInsert describes.
> -      todoAssertAreaPlacements(CustomizableUI.AREA_PANEL, expectedPlacementsAfterInsert);

Sweet!

	:Gijs (he/him) Assignee
	Comment 5 • 11 years ago

(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 806217 [details] [diff] [review]
> Narrow widgets dropped on a wide widget should place the narrow widget above
> the wide widget,
> 
> Review of attachment 806217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/PanelWideWidgetTracker.jsm
> @@ +155,5 @@
> > +  },
> > +  trackWidget: function(aWidgetId) {
> > +    gWideWidgets.add(aWidgetId);
> > +  },
> > +  untrackWidget: function(aWidgetId) {
> 
> trackWidget and untrackWidget aren't referenced elsewhere. Are these
> unnecessary now?

Good point. I changed how I did that, so I've removed these.

https://hg.mozilla.org/projects/ux/rev/8ae13888e4cb

	:Gijs (he/him) Assignee
	Comment 6 • 11 years ago

One day, I will learn that Sets broke the keyword rule and use 'delete' rather than 'remove'. One day.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 7 • 11 years ago

https://hg.mozilla.org/projects/ux/rev/92b0f1d2249c

	:Gijs (he/him) Assignee
	Comment 8 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/8ae13888e4cb
https://hg.mozilla.org/mozilla-central/rev/92b0f1d2249c