873501 - Allow items to be dropped on pre-existing spacers, separators and springs

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

These widgets are in toolkit customization but aren't found in the Australis customization.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

According to shorlander, this is by design. We don't want these elements available by default in the palette, and we should take them out where they already exist in currentsets.

Comment 2

[u428464]

The "activity indicator" should also be removed. Don't know if it could be handled here or if it needs another bug.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Guillaume C. [:ge3k0s] from comment #2)
> The "activity indicator" should also be removed. Don't know if it could be
> handled here or if it needs another bug.

Good call; thanks ge3k0s. I've filed bug 873518.

Comment 4

[Dão Gottwald [:dao]]

We actually have a flexible space in the add-on bar's default set. A similar setup might make sense for customization areas added by add-ons that don't already have flexible elements such as the tab bar or the location bar.

What's the downside of maintaining support for some or all of these items?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #4)
>
> What's the downside of maintaining support for some or all of these items?

I believe the downside is that then we'd have to maintain support for them. :) It's a non-zero cost.

There might be more rationale from the UX team, but for me, this is the big one: it means we have to deal with these special items and their uniquely generated IDs.

Comment 6

[Dão Gottwald [:dao]]

Have you looked into how the traditional toolbar customization code handles these items? In principle, the new code should be able to work the same way, I would think/hope. It's code to write, but the basic idea of how this code should work should be pretty straightforward.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #6)
> Have you looked into how the traditional toolbar customization code handles
> these items? In principle, the new code should be able to work the same way,
> I would think/hope. It's code to write, but the basic idea of how this code
> should work should be pretty straightforward.

Agreed - I don't think keeping these special widgets would incur a great cost. I saw some discussion in #fx-team about the IDs of the special widgets - that's lifted directly from the old code, and IMO doesn't need to change. I think the main thing is just adding support in the customization mode UI.

So, I think this shouldn't be a technical decision. If we remove them, it sure be purely for UX reasons.

Comment 8

[:Gijs (he/him)]

We discussed this in the triage meeting today, and the preliminary plan is to not have separators, springs and spaces in the customization palette, but not to remove them either. However, we will need to update bits of the customization code to deal with dropping items on springs (right now we look for the generated ID in the placements code and that fails for obvious reasons).

Comment 9

[:Gijs (he/him)]

Attachment: Patch

This uses generated IDs in placement strings, with a magical prefix. I've tested by temporarily reapplying (the required bits of) my patch for bug 872209, and this fixes any/all dnd issues we used to have with springs/spacers/separators, it seems. I've also written some tests (although they're not really exhaustive) which will come as a separate patch in a bit...

Comment 10

[:Gijs (he/him)]

Attachment: Tests

Our first tests! The cleanup of these (and possibly the implementation of the create toolbar bit) will need to be improved once we figure out bug 877178. Other than that, these pass! :-)

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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

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

LGTM!

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 755404 [details] [diff] [review]
Tests

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

::: browser/components/customizableui/test/browser_873501_handle_specials.js
@@ +1,1 @@
> +

Needs license header

@@ +1,4 @@
> +
> +let gTests = [
> +  {
> +    desc: "Add a toolbar with a spring",

This file is called handle_specials, but we're only testing spring. We should probably test spacer and separator too.

@@ +27,5 @@
> +
> +      // Try moving the downloads button to this new toolbar, between the two springs:
> +      CustomizableUI.addWidgetToArea("downloads-button", kToolbarName, 1);
> +      widgets = CustomizableUI.getWidgetsInArea(kToolbarName);
> +      is(widgets.map(x => x.id).join(","), [springId, "downloads-button", spring2Id].join(","),

I can see this being something that comes up again and again in these tests.

Perhaps we can add something to head.js, like "assertToolbarPlacements(someToolbarId, arrayOfWidgetIds)"

@@ +43,5 @@
> +  waitForExplicitFinish();
> +  registerCleanupFunction(cleanup);
> +  runTests(gTests);
> +}
> +

Nit - remove the extra newline

::: browser/components/customizableui/test/head.js
@@ +1,4 @@
> +// Avoid leaks by using tmp for imports...
> +
> +let tmp = {};
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", tmp);

You don't use Promise in here, so I guess you don't need to import it.

@@ +46,5 @@
> +      yield test.teardown();
> +  }
> +}
> +
> +

Nit - remove newline

@@ +53,5 @@
> +    ok(false, "Unexpected exception: " + ex);
> +    finish();
> +  });
> +}
> +

Nit - remove extra newline.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Also, +1 for breaking the ice on tests. :)

Comment 14

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

(In reply to Mike Conley (:mconley) from comment #13)
> Also, +1 for breaking the ice on tests. :)

Another +1 can't hurt :-)

Comment 15

[:Gijs (he/him)]

Attachment: Tests v1.1

Alright, taken aboard the feedback, written a separate function that lets you use regexes for the placements matching as well, and added tests for the separators and spacers. More passes! :-)

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 755859 [details] [diff] [review]
Tests v1.1

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

::: browser/components/customizableui/test/browser_873501_handle_specials.js
@@ +34,5 @@
> +    run: function() {
> +      CustomizableUI.addWidgetToArea("separator", kToolbarName, 1);
> +      CustomizableUI.addWidgetToArea("separator", kToolbarName, 3);
> +      assertAreaPlacements(kToolbarName,
> +                           [/customizableui-special-spring\d+/,

I'm not too jazzed about these tests needing to know what the previous tests added to the toolbar. Sounds fragile. Can we clear the toolbar after each test?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 755859 [details] [diff] [review]
Tests v1.1

Should have cleared the r? on that.

Comment 18

[:Gijs (he/him)]

This got pushed because it turns out resetting is not possible properly without the defaults or the unregistering work. I've filed a followup (bug 877851) to fix this.

Comment 19

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

https://hg.mozilla.org/projects/ux/rev/746b7883634b
https://hg.mozilla.org/projects/ux/rev/e2fc66f343c9

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/746b7883634b
https://hg.mozilla.org/mozilla-central/rev/e2fc66f343c9