866245 - Let widgets know when they're in a toolbar or the menu panel, and what the appropriate anchor is.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Blair first mentioned this in https://bugzilla.mozilla.org/show_bug.cgi?id=865926#c2, and I'll paste his comment here:

My general idea there was to provide APIs for widgets/add-ons, so its easier to do the right thing - which is probably better suited for a separate (related) bug. 

Was thinking that CustomizableUI could set some attributes on widgets when they're placed in an area or when they're moved into the overflow panel. Something like:
* customizableui-areatype - can be toolbar, panel, overflow
* customizableui-anchorid - ID of the anchor, or empty

And CustomizableUI could have some APIs to help out, something like:
* CustomizableUI.openWidgetPanel(aWidgetNode)
* CustomizableUI.isWidgetInOverflow(aWidgetId)

(I find its generally useful to set both declarative metadata, and provide methods to get the same info - it better covers use cases we haven't thought of, which is often an issue add-on authors hit.)

I think being too smart with the overflow stuff is going to land us in hot water - there's just far too many edge cases. I'd rather we had a solid solution that worked good enough, even if the UX of some add-ons isn't ideal - and just help add-ons adapt to the new world order (by providing easy to use APIs and some tech evangelism).

--snip--

This sounds like a fine idea to me.

Comment 1

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

Attachment: Patch v1

This is my first shot at it. I've changed the area properties from a Set to a Map, so areas can register themselves giving their type and the ID for the anchor that should be used by their widgets.

I've also added two new API functions - getWidgetAnchor(aNode) and isWidgetInOverflow: function(aNode). Are there other functions we should expose here?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 743234 [details] [diff] [review]
Patch v1

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

Looks good. Not sure what other APIs will be useful - we can always add more later, if/when we see a need.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +287,5 @@
>      this._defineBuiltInWidgets();
>      this.loadSavedState();
>  
> +    this.registerArea(CustomizableUI.AREA_PANEL, [["type", "menu-panel"],
> +                                                  ["anchor", "PanelUI-menu-button"]]);

This makes for a rather awkward API. I think it should either take an object like:

  this.registerArea(CustomizableUI.AREA_PANEL, {type: "menu-panel",
                                                anchor: "PanelUI-menu-button"});

Or just take a Map (or both!). Would need to make a clone of the Map (sadly, have to do that manually), however - to avoid it being modified by outside code.

(To be clear: I think the internal storage of these should indeed be a Map.)

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 743234 [details] [diff] [review]
Patch v1

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1566,5 @@
> +  },
> +  getWidgetAnchor: function(aNode) {
> +    return CustomizableUIInternal.getWidgetAnchor(aNode);
> +  },
> +  isWidgetInOverflow: function(aNode) {

Just had a thought: These APIs don't really fit here, given how the rest of the API is designed. We're not really using them yet, and they need fleshed out more, but... these two APIs would be better suited to be part of WidgetSingleWrapper/XULWidgetSingleWrapper (and therefore named differently).

Comment 4

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

Attachment: Patch v1.1

Ok, we register areas with an Object now (I agree, this is much better), and anchors / overflowed getters have been added to the widget wrappers. I'm using the overflowedItem class that jaws introduced / is introducing in bug 865926 to determine if a widget is overflowed.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 747122 [details] [diff] [review]
Patch v1.1

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +363,5 @@
>      this.loadSavedState();
>  
> +    this.registerArea(CustomizableUI.AREA_PANEL, {
> +      anchor: "PanelUI-menu-button",
> +      type: "menu-panel"

Lets have these types ("menu-panel", "toolbar") as constants on CustomizableUI (AREATYPE_TOOLBAR, etc) - they'll be useful for other things too.

@@ +656,5 @@
>        if (gPalette.has(aWidgetId) || this.isSpecialWidget(aWidgetId)) {
>          container.removeChild(widgetNode);
>        } else {
> +        widgetNode.removeAttribute("customizableui-areatype");
> +        widgetNode.removeAttribute("customizableui-anchorid");

Refactor this out to removeLocationAttributes().

Comment 6

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

Attachment: Patch v1.2 (r+'d by Unfocused)

De-bitrotted.

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #5)
> Comment on attachment 747122 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 747122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +363,5 @@
> >      this.loadSavedState();
> >  
> > +    this.registerArea(CustomizableUI.AREA_PANEL, {
> > +      anchor: "PanelUI-menu-button",
> > +      type: "menu-panel"
> 
> Lets have these types ("menu-panel", "toolbar") as constants on
> CustomizableUI (AREATYPE_TOOLBAR, etc) - they'll be useful for other things
> too.

Good idea. Done.

> 
> @@ +656,5 @@
> >        if (gPalette.has(aWidgetId) || this.isSpecialWidget(aWidgetId)) {
> >          container.removeChild(widgetNode);
> >        } else {
> > +        widgetNode.removeAttribute("customizableui-areatype");
> > +        widgetNode.removeAttribute("customizableui-anchorid");
> 
> Refactor this out to removeLocationAttributes().

Done.

I've also removed the overflow stuff until the nav-bar overflow stuff gets more firm. I'll file a follow-up bug to add an overflowed getter to the widgets.

Comment 7

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

https://hg.mozilla.org/projects/ux/rev/19fac4398eb0

Comment 8

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

Attachment: Follow-up

Landed a follow-up due to a bad copypaste error slipping in. onWidgetAdded was busted in CustomizableUI.jsm due to the wrong variable names being used.

https://hg.mozilla.org/projects/ux/rev/c82232ceed5e

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/19fac4398eb0
https://hg.mozilla.org/mozilla-central/rev/c82232ceed5e