884204 - Unify "removable" property implementation

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Currently our use of removable="true"/"false" is a little messy. For API-provided widgets, we try to check the widget's property, but for XUL widgets we use the attribute. I'm not sure why we don't just use the attribute everywhere. Right now the story is 'complicated' for custom widgets, which up until just now had to provide both - and for bug 882960 we made sure custom widgets also get the right behaviour everywhere.

However, now we use properties for API-provided widgets, and attributes for XUL widgets.

I suspect the best way is actually to always (even for API-provided widgets) use attributes to check removability, and only use the property to set the attribute on API-created nodes, but not to later check if the item is removable. Ironically, this is me changing my mind from bug 875775 comment 9... Jared, Mike, what do you think?

Comment 1

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

What's wrong with using the property? The property getter just returns getAttribute("removable") == "true", right?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #1)
> What's wrong with using the property? The property getter just returns
> getAttribute("removable") == "true", right?

Not for the widget objects in gPalette, which is the property Gijs is referring to. But I do prefer to use that property - its the actual value (not a wrapper for it, like using attribute would be), faster (saves looking at the DOM), and works even when we don't have a browser window open.

If we're always looking this up via isWidgetRemovable(), I don't see the downside. We always have to treat the two widget types separately for getting other properties already anyway.

Comment 3

[:Gijs (he/him)]

(In reply to Blair McBride [:Unfocused] from comment #2)
> (In reply to Jared Wein [:jaws] from comment #1)
> > What's wrong with using the property? The property getter just returns
> > getAttribute("removable") == "true", right?
> 
> Not for the widget objects in gPalette, which is the property Gijs is
> referring to. But I do prefer to use that property - its the actual value
> (not a wrapper for it, like using attribute would be), faster (saves looking
> at the DOM), and works even when we don't have a browser window open.
> 
> If we're always looking this up via isWidgetRemovable(), I don't see the
> downside. We always have to treat the two widget types separately for
> getting other properties already anyway.

There's something to be said for this. But at the minute, we have issues in some cases with add-ons inserting toolbarbuttons, we run into them when running buildArea or when running the add-on bar's auto-remove code, and then we end up looking things up by ID, don't have a build window, and don't do the Right Thing, even though we have a real node object on the other end of this API chain. :-(

Then there's a styling aspect to it. I figured that for the 'constrain how we let you move this widget' stuff, it'd be quite useful to be able to select for these widgets using the attribute in CSS. As it is, we have no guarantees for type=custom API-built widgets, because the node creation is handled entirely by the widget's options object's onBuild function.

It's all just a little hairy. I'm not 100% convinced we need to change anything, but I think it's useful to discuss this and see if this is what we want, and/or if we want to do anything about the custom API widget case or not.

Comment 4

[Justin Dolske [:Dolske]]

Calling this P5 on the assumption there's no actual user impact (code/API sanity), but if we're going to change this it should happen ASAP to avoid API churn.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> There's something to be said for this. But at the minute, we have issues in
> some cases with add-ons inserting toolbarbuttons, we run into them when
> running buildArea or when running the add-on bar's auto-remove code, and
> then we end up looking things up by ID, don't have a build window, and don't
> do the Right Thing, even though we have a real node object on the other end
> of this API chain. :-(

This bit has been fixed, so calling this WFM.