Closed Bug 885579 Opened 7 years ago Closed 6 years ago

Narrow widgets dropped on a wide widget should place the narrow widget above the wide widget

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P2])

Attachments

(2 files, 1 obsolete file)

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]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
I've been poking at this locally, working on a patch, but depends on bug 876926.
Assignee: nobody → gijskruitbosch+bugs
Depends on: 876926
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.
Attachment #806211 - Flags: review?(jaws)
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.
Attachment #806217 - Flags: review?(jaws)
Attachment #806211 - Attachment is obsolete: true
Attachment #806211 - Flags: review?(jaws)
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!
Attachment #806217 - Flags: review?(jaws) → review+
(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
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]
One day, I will learn that Sets broke the keyword rule and use 'delete' rather than 'remove'. One day.
Attachment #809070 - Flags: review?(mdeboer)
Attachment #809070 - Flags: review?(mdeboer) → review+
Depends on: 923859
https://hg.mozilla.org/mozilla-central/rev/8ae13888e4cb
https://hg.mozilla.org/mozilla-central/rev/92b0f1d2249c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.