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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M5])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Created attachment 743234 [details] [diff] [review]
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?
Attachment #743234 - Flags: feedback?(jaws)
Attachment #743234 - Flags: feedback?(bmcbride)
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.)
Attachment #743234 - Flags: feedback?(bmcbride) → feedback+
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).
Created attachment 747122 [details] [diff] [review]
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.
Attachment #743234 - Attachment is obsolete: true
Attachment #743234 - Flags: feedback?(jaws)
Attachment #747122 - Flags: review?(bmcbride)
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().
Attachment #747122 - Flags: review?(bmcbride) → review+
No longer blocks: 860814
Whiteboard: [Australis:M5]
Blocks: 872617
Created attachment 751275 [details] [diff] [review]
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.
Attachment #747122 - Attachment is obsolete: true
Attachment #751275 - Flags: review+
Summary: Let widgets know when they're in a toolbar, the menu panel, or the overflow panel → Let widgets know when they're in a toolbar or the menu panel, and what the appropriate anchor is.
https://hg.mozilla.org/projects/ux/rev/19fac4398eb0
Whiteboard: [Australis:M5] → [Australis:M5][fixed-in-ux]
Created attachment 751409 [details] [diff] [review]
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
https://hg.mozilla.org/mozilla-central/rev/19fac4398eb0
https://hg.mozilla.org/mozilla-central/rev/c82232ceed5e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M5][fixed-in-ux] → [Australis:M5]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.