PanelWideWidgetTracker should move wide widget up even when small widget is dragged on top of it if there are no more small widgets to fill up the row

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Found by Mike de Boer when testing the drag placeholder patch:

STR:

(from default state)
1. Move edit-controls to the end of the panel
2. Add another small button to the panel *by dragging it onto the wide widget* (so not on top of any of the previous small widgets)

AR:
small widget goes before the wide widget

ER:
small widget should go after the wide widget, because otherwise there'll be a "hole"

(when fixing this, we should add a testcase)
(Assignee)

Comment 1

4 years ago
Created attachment 832904 [details] [diff] [review]
fix forward-moving detection to check if we actually can,

This fixes stuff, and makes code nicer, and has a test.
Attachment #832904 - Flags: review?(mdeboer)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 832904 [details] [diff] [review]
fix forward-moving detection to check if we actually can,

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

Apart from two comments, this looks & works good. Not worth to r-

::: browser/components/customizableui/src/PanelWideWidgetTracker.jsm
@@ +147,5 @@
>        desiredPos += desiredChange;
>        CustomizableUI.moveWidgetWithinArea(aWidgetId, desiredPos);
>      }
>    },
> +

Good to abstract this an a separate method, but can you document what it does (docblock)?

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ +427,5 @@
> +                                 "edit-controls"];
> +      simulateItemDrag(editControls, target);
> +      assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterMove);
> +      let syncButton = document.getElementById("sync-button");
> +      placementsAfterMove = ["zoom-controls",

I know this is copy-paste, but can you just do
```js
let itemToDrag = "sync-button";
let button = document.getElementById(itemToDrag);
placementsAfterMove.push("sync-button");
simulateItemDrag(button, editControls);
...
```
Attachment #832904 - Flags: review?(mdeboer) → review+
typo:

```js
let itemToDrag = "sync-button";
let button = document.getElementById(itemToDrag);
placementsAfterMove.push(itemToDrag);
simulateItemDrag(button, editControls);
```
(Assignee)

Comment 4

4 years ago
w/ nits: https://hg.mozilla.org/projects/ux/rev/7bd5ee7b71e0
Whiteboard: [Australis:P3] → [Australis:P3][Australis:M9][fixed-in-ux]
(Assignee)

Comment 5

4 years ago
https://hg.mozilla.org/mozilla-central/rev/7bd5ee7b71e0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][Australis:M9][fixed-in-ux] → [Australis:P3][Australis:M9]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.