Closed Bug 873501 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M6])

Attachments

(2 files, 1 obsolete file)

These widgets are in toolkit customization but aren't found in the Australis customization.
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.
Summary: Separator, Flexible Space, and Space are missing from the Australis customization palette → Write a migration to remove separators, flexible spaces and spaces from currentsets.
The "activity indicator" should also be removed. Don't know if it could be handled here or if it needs another bug.
(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.
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?
(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.
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.
(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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?] → [Australis:M6]
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).
Attached patch PatchSplinter Review
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...
Attachment #755400 - Flags: review?(mconley)
Attached patch Tests (obsolete) — Splinter Review
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! :-)
Attachment #755404 - Flags: review?(mconley)
Comment on attachment 755400 [details] [diff] [review]
Patch

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

LGTM!
Attachment #755400 - Flags: review?(mconley) → review+
Summary: Write a migration to remove separators, flexible spaces and spaces from currentsets. → Allow items to be dropped on pre-existing spacers, separators and springs
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.
Attachment #755404 - Flags: review?(mconley)
Also, +1 for breaking the ice on tests. :)
(In reply to Mike Conley (:mconley) from comment #13)
> Also, +1 for breaking the ice on tests. :)

Another +1 can't hurt :-)
Attached patch Tests v1.1Splinter Review
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! :-)
Attachment #755404 - Attachment is obsolete: true
Attachment #755859 - Flags: review?(mconley)
Attachment #755859 - Attachment is patch: true
Attachment #755859 - Attachment mime type: text/x-patch → text/plain
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 on attachment 755859 [details] [diff] [review]
Tests v1.1

Should have cleared the r? on that.
Attachment #755859 - Flags: review?(mconley)
Depends on: 877851
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.
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
Blocks: 877851
No longer depends on: 877851
https://hg.mozilla.org/mozilla-central/rev/746b7883634b
https://hg.mozilla.org/mozilla-central/rev/e2fc66f343c9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.