Closed Bug 1313459 Opened 8 years ago Closed 7 years ago

Extend browser_action to allow putting buttons in other CUI areas

Categories

(WebExtensions :: Frontend, defect, P3)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed
webextensions ?

People

(Reporter: miketaylr, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

As a user I'd like an option to put my web extension button in the panel by default.

Right now the only way to do this is with CustomizableUI.jsm.
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Whiteboard: [design-decision-needed]
To be discussed at Jan 10 WebExtensions Triage meeting. 

Agenda: https://docs.google.com/document/d/18K97o1juaHSeYEkes1iMz8AayjuVkUuIK844ErGaa-c/edit#
To be more clear, the desired result would be the same as:

CustomizableUI.createWidget({..., defaultArea: CustomizableUI.AREA_PANEL}
@gijs: we feel it's a good idea to at least allow the panel as an option.  I was thinking that it may be nice to allow other area's as options as well, but we wanted to get your input on that.  good idea?  bad idea? other?
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P5
Whiteboard: [design-decision-needed] → [design-decision-approved] triaged
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> @gijs: we feel it's a good idea to at least allow the panel as an option.  I
> was thinking that it may be nice to allow other area's as options as well,
> but we wanted to get your input on that.  good idea?  bad idea? other?

Given that other add-ons can do this I don't see any reason webextensions shouldn't be able to. I assume right now we default to the toolbar?
Flags: needinfo?(gijskruitbosch+bugs)
Yes, default would be toolbar.
webextensions: --- → ?
Priority: P5 → P3
Assignee: nobody → mixedpuppy
Summary: Extend browser_action to allow putting buttons in hamburger panel by default → Extend browser_action to allow putting buttons in other CUI areas
Comment on attachment 8838647 [details]
Bug 1313459 support CUI areas for browserAction,

Also asking Gijs for feedback from a CUI perspective.
Attachment #8838647 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8838647 [details]
Bug 1313459 support CUI areas for browserAction,

https://reviewboard.mozilla.org/r/113482/#review115090

The webextensions bits look good, but somebody familiar with CustomizableUI should also have a glance.  I see you've got an f? to Gijs for that...

::: browser/components/extensions/ext-browserAction.js:43
(Diff revision 1)
>  }
>  
>  // WeakMap[Extension -> BrowserAction]
>  var browserActionMap = new WeakMap();
>  
> +var browserAreas = {

const

::: browser/components/extensions/ext-browserAction.js:72
(Diff revision 1)
>      title: options.default_title || extension.name,
>      badgeText: "",
>      badgeBackgroundColor: null,
>      icon: IconDetails.normalize({path: options.default_icon}, extension),
>      popup: options.default_popup || "",
> +    area: options.default_area && browserAreas[options.default_area],

If you add `"default": "navbar"` to the schema, then you don't need any conditionals here and below where you set the default.
Attachment #8838647 - Flags: review?(aswan) → review+
Comment on attachment 8838647 [details]
Bug 1313459 support CUI areas for browserAction,

https://reviewboard.mozilla.org/r/113482/#review115090

Yeah, this can wait for Gijs.
Attachment #8838647 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 8838647 [details]
Bug 1313459 support CUI areas for browserAction,

Sorry for the delay. Yeah, this looks OK to me, though it would be a good idea to add a test that the default (when no area is specified) is the nav-bar, maybe? If we think that's already covered by other tests then a comment to that effect might be helpful.

The other thing I feel like I should say is that apparently there are some photon designs (still in flux, as I understand it) that show us altering the customizable area concepts again. Without stating any opinions about that idea, I would keep that in mind when announcing / publishing / stabilizing this API. In other words, how would we deal with extra areas / some areas going away? I'm not that familiar with the internals of how we handle options, but migrating / interpreting option A as equivalent to option B might need to be possible in the future, if it isn't yet.
Attachment #8838647 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Keywords: dev-doc-needed
Whiteboard: [design-decision-approved] triaged → triaged
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3d9ddc7244ad
support CUI areas for browserAction, r=aswan
sorry had to back this out for browser chrome failures like https://treeherder.mozilla.org/logviewer.html#?job_id=78894877&repo=autoland&lineNumber=1903
Flags: needinfo?(mixedpuppy)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fc93c32ef2e
Backed out changeset 3d9ddc7244ad for bunch of browser chrome failures
> ::: browser/components/extensions/ext-browserAction.js:72
> (Diff revision 1)
> >      title: options.default_title || extension.name,
> >      badgeText: "",
> >      badgeBackgroundColor: null,
> >      icon: IconDetails.normalize({path: options.default_icon}, extension),
> >      popup: options.default_popup || "",
> > +    area: options.default_area && browserAreas[options.default_area],
> 
> If you add `"default": "navbar"` to the schema, then you don't need any
> conditionals here and below where you set the default.

This didn't actually work, and added back default value into the code.  However I had left the default value in schema, and while it didn't choke in my testing, it did when it landed with an invalid schema value.  I don't see default in any other schema as well.
Flags: needinfo?(mixedpuppy) → needinfo?(aswan)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14dbbe36e676
support CUI areas for browserAction, r=aswan
Backed out in https://hg.mozilla.org/integration/autoland/rev/e52cd3f75ebf for https://treeherder.mozilla.org/logviewer.html#?job_id=79222095&repo=autoland - might be down to just Win7 opt timeouts, or we might just be horribly slow about getting around to running it on enough other platforms, dunno.
third times a charm (x fingers).  But actually, the last try did work out.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c2c5efc2b575
support CUI areas for browserAction, r=aswan
https://hg.mozilla.org/mozilla-central/rev/c2c5efc2b575
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> This didn't actually work, and added back default value into the code. 
> However I had left the default value in schema, and while it didn't choke in
> my testing, it did when it landed with an invalid schema value.

sorry, i'm having trouble parsing the above...  what's the question?

> I don't see
> default in any other schema as well.

its not widely used but eg http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/browser/components/extensions/schemas/sessions.json#68
Flags: needinfo?(aswan)
Depends on: 1344408
Depends on: 1354109
I've added a bit here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action that talks about "default_area" but let me know it we need anything else. Thanks!
Flags: needinfo?(mixedpuppy)
Docs LGTM
Flags: needinfo?(mixedpuppy)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.