890140 - [Australis customization] Extra-row placeholder even when last row is not full

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jesse Ruderman]

UX 25.0a1 (2013-07-02)

1. Make a new profile
2. Drag "Developer" from the customization palette to the bottom row of the menu.
3. Exit customization mode

4. Enter customization mode

Expected: 2 placeholders, filling out the row that contains only the Developer button.

Result: 3 placeholders, but if I drag any of them, the third one disappears.

Comment 1

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

Attachment: Patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 795784 [details] [diff] [review]
Patch

First off, why the firstChild check? Is this just for the wrapper case? And you removed the hidden check, is that genuinely no longer necessary? :-)

Then, in the case where:

| X X X |
| X X   |
| [ - ] |

Doesn't this (now) produce 1 placeholder? I think if you instead of the let of loop, you used:

let node = contents.lastChild;
let hangingChildren = 0;
while (node && !node.classList.contains("panel-combined-item") &&
       (!node.firstChild || !node.firstChild.classList.contains("panel-combined-item")))
  hangingChildren++;
  node = node.previousSibling;

hangingChildren = hangingChildren % kColumnsInMenuPanel;

// (essentially, counting from the last wide widget, or the top, whichever comes first)

this solves that case, too (that is, it will produce 3 placeholders, and we can later add code in bug 885578 to fill the gap).

Comment 3

[:Gijs (he/him)]

Also, could we have tests for this, pretty please? :-)

Comment 4

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

I'm picking this back up today.

Comment 5

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

Attachment: Patch v2

Comment 6

[:Gijs (he/him)]

Comment on attachment 820539 [details] [diff] [review]
Patch v2

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

In principle, this looks good to me for what it's designed to do, but I'm quite worried about the DnD placeholders that show up while dragging, which is apparently while the whole visiblePlaceholders thing was there (bug 877370). Please doublecheck that you're not breaking that.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1310,5 @@
> +      node = node.previousSibling;
> +    }
> +
> +    let orphanedItems = narrowItemsAfterWideItem % kColumnsInMenuPanel;
> +    // Always have at least 1 placeholder visible.

This comment didn't make sense to me and still doesn't.

@@ -1312,5 @@
>        let placeholderChild = doc.createElement("toolbarbutton");
>        placeholderChild.classList.add(kPlaceholderClass + "-child");
>        placeholder.appendChild(placeholderChild);
> -      // Always have at least 1 placeholder visible.
> -      placeholder.setAttribute("hidden", --visiblePlaceholders < 0);

Have you checked that this doesn't break the dragging placeholders when dragging items within the menupanel? That's why this code is here... see bug 877370.

::: browser/components/customizableui/test/browser.ini
@@ +8,3 @@
>  [browser_877447_skip_missing_ids.js]
>  [browser_878452_drag_to_panel.js]
>  [browser_876926_customize_mode_wrapping.js]

While we're here, can you be awesome and reorder these tests at the top so they're in bug # order? Thanks!

::: browser/components/customizableui/test/browser_890140_orphaned_placeholders.js
@@ +33,5 @@
> +      let btn = document.getElementById("developer-button");
> +      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
> +      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
> +
> +      let placementsAfterAppend = placements.concat(["developer-button", "sync-button"]);

You could just remove one item here instead of appending two, which would be slightly fewer loc, but I'm not too fussed.

@@ +53,5 @@
> +      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
> +    },
> +  },
> +  {
> +    desc: "A wide widget at the bottom of the panel should have three placeholders after it.",

Shouldn't the default state have 3 small items which also have 3 placeholders after it? Please add a test for that and/or change this test to test that.

Comment 7

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

https://hg.mozilla.org/projects/ux/rev/ac71c78d03ea

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 820539 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 820539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In principle, this looks good to me for what it's designed to do, but I'm
> quite worried about the DnD placeholders that show up while dragging, which
> is apparently while the whole visiblePlaceholders thing was there (bug
> 877370). Please doublecheck that you're not breaking that.

Double-checked, this patch changes it so we don't have unused hidden placeholders anymore.

> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +1310,5 @@
> > +      node = node.previousSibling;
> > +    }
> > +
> > +    let orphanedItems = narrowItemsAfterWideItem % kColumnsInMenuPanel;
> > +    // Always have at least 1 placeholder visible.
> 
> This comment didn't make sense to me and still doesn't.

Well, it was there to say that we should never have 0 placeholders (obviously). I don't care strongly about it, so I've removed it.

> @@ -1312,5 @@
> >        let placeholderChild = doc.createElement("toolbarbutton");
> >        placeholderChild.classList.add(kPlaceholderClass + "-child");
> >        placeholder.appendChild(placeholderChild);
> > -      // Always have at least 1 placeholder visible.
> > -      placeholder.setAttribute("hidden", --visiblePlaceholders < 0);
> 
> Have you checked that this doesn't break the dragging placeholders when
> dragging items within the menupanel? That's why this code is here... see bug
> 877370.

Yep.

> ::: browser/components/customizableui/test/browser.ini
> @@ +8,3 @@
> >  [browser_877447_skip_missing_ids.js]
> >  [browser_878452_drag_to_panel.js]
> >  [browser_876926_customize_mode_wrapping.js]
> 
> While we're here, can you be awesome and reorder these tests at the top so
> they're in bug # order? Thanks!

Done.

> :::
> browser/components/customizableui/test/browser_890140_orphaned_placeholders.
> js
> @@ +33,5 @@
> > +      let btn = document.getElementById("developer-button");
> > +      let panel = document.getElementById(CustomizableUI.AREA_PANEL);
> > +      let placements = getAreaWidgetIds(CustomizableUI.AREA_PANEL);
> > +
> > +      let placementsAfterAppend = placements.concat(["developer-button", "sync-button"]);
> 
> You could just remove one item here instead of appending two, which would be
> slightly fewer loc, but I'm not too fussed.

Updated the test to have both cases.

> @@ +53,5 @@
> > +      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
> > +    },
> > +  },
> > +  {
> > +    desc: "A wide widget at the bottom of the panel should have three placeholders after it.",
> 
> Shouldn't the default state have 3 small items which also have 3
> placeholders after it? Please add a test for that and/or change this test to
> test that.

Done.

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/ac71c78d03ea