875775 - Allow widgets created by createWidget to specify if they're removable or not.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Since bug 866978, we respect the removable="true" attribute on XUL nodes to know whether or not we can take certain items out of their customizable areas.

We do not, however allow widget created via createWidget to set this attribute. We should allow this.

Comment 1

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

Is this something that is needed for m6 though? I don't think we have any widgets that need this API change right away.

Comment 2

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

Good point. I was being overzealous. :)

Comment 3

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

Taking this since I just wontfix'ed bug 875809.

Comment 4

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

Attachment: Patch

Up for grabs to whoever can review it sooner :)

Comment 5

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

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

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +684,5 @@
>      node.setAttribute("widget-type", aWidget.type);
>      if (aWidget.disabled) {
>        node.setAttribute("disabled", true);
>      }
> +    node.setAttribute("removable", !!aWidget.removable);

Do we really need the coercion here? We seem to have done a pretty good job of ensuring that it's only a boolean that gets assigned to aWidget.removable.

Comment 6

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

Removed the coercion and landed on the UX branch,
https://hg.mozilla.org/projects/ux/rev/1c5ed65f7bea

Comment 7

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #6)
> Removed the coercion and landed on the UX branch,
> https://hg.mozilla.org/projects/ux/rev/1c5ed65f7bea

I believe this block needed to be updated, too:

https://hg.mozilla.org/projects/ux/file/1c5ed65f7bea/browser/components/customizableui/src/CustomizableUI.jsm#l1475

Comment 8

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

Attachment: Patch part 2, followup

Thanks for catching that Gijs. I didn't notice it because all of our current widgets are removable :)

Comment 9

[:Gijs (he/him)]

Comment on attachment 755370 [details] [diff] [review]
Patch part 2, followup

Hrm. r+ if there is really no sane way to get the spec of the API-provided widget and actually check the removable property. Which would seem strange to me in and of itself, tbh...

Comment 10

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

Attachment: Patch part 2, followup v1.1

Yeah, this is better.

Comment 11

[:Gijs (he/him)]

Comment on attachment 755382 [details] [diff] [review]
Patch part 2, followup v1.1

>diff --git a/browser/components/customizableui/src/CustomizableUI.jsm b/browser/components/customizableui/src/CustomizableUI.jsm

>+      return gPalette.get(aWidgetId).removable;
>+    } else if (provider == CustomizableUI.PROVIDER_XUL) {

Nit: please don't use else after a return.

With that, r=me

Comment 12

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

Removed the 'else' and landed on UX:
https://hg.mozilla.org/projects/ux/rev/a7d470940b94

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/1c5ed65f7bea
https://hg.mozilla.org/mozilla-central/rev/a7d470940b94