Closed Bug 875775 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M6])

Attachments

(2 files, 1 obsolete file)

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.
Is this something that is needed for m6 though? I don't think we have any widgets that need this API change right away.
Good point. I was being overzealous. :)
Whiteboard: [Australis:M6] → [Australis:M?]
Taking this since I just wontfix'ed bug 875809.
Assignee: mconley → jaws
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Attached patch PatchSplinter Review
Up for grabs to whoever can review it sooner :)
Attachment #754869 - Flags: review?(mconley)
Attachment #754869 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #754869 - Flags: review?(mconley) → review+
Attachment #754869 - Flags: review?(gijskruitbosch+bugs)
Removed the coercion and landed on the UX branch,
https://hg.mozilla.org/projects/ux/rev/1c5ed65f7bea
Whiteboard: [Australis:M?] → [Australis:M?][fixed-in-ux]
Whiteboard: [Australis:M?][fixed-in-ux] → [Australis:M6][fixed-in-ux]
(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
Attached patch Patch part 2, followup (obsolete) — Splinter Review
Thanks for catching that Gijs. I didn't notice it because all of our current widgets are removable :)
Attachment #755370 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #755370 - Flags: review?(gijskruitbosch+bugs) → review+
Yeah, this is better.
Attachment #755370 - Attachment is obsolete: true
Attachment #755382 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #755382 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/1c5ed65f7bea
https://hg.mozilla.org/mozilla-central/rev/a7d470940b94
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.