BrowserAction icon sizes don't match Firefox defaults.

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bwinton, Assigned: johannh, Mentored)

Tracking

({dev-doc-complete})

Trunk
mozilla45
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox43 affected, firefox45 fixed)

Details

(Whiteboard: [webextension-polish][browserAction])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
The Chrome browserAction spec (https://developer.chrome.com/extensions/browserAction#icon) says "Browser action icons can be up to 19 dips", but Firefox's native icon sizes are:
18px/36px for the toolbar (regular and retina)
32px/64px for the menu panel (regular and retina)

We should choose better sizes, where possible, and not restrict ourselves to 19 and 38, so that the icons don't end up all blurry in the menu panel.

Comment 1

2 years ago
I highly agree not to restrict ourselves to 19 and 38. We should just allow them all added, and we'll use 18 and 32 when available.

Although are you sure the toolbar is 18 i thought it was 16x16 img with some padding.
Mentor: gkrizsanits@mozilla.com
(Assignee)

Comment 2

2 years ago
I'll take this one. :)
(Reporter)

Updated

2 years ago
Assignee: nobody → mail
Status: NEW → ASSIGNED

Updated

2 years ago
Whiteboard: [webextension-polish] → [webextension-polish][browserAction]
Once bug 1197422 lands, it should simply be a matter of adding the additional sizes to the array of accepted sizes, and adding a few more tests.
Depends on: 1197422

Updated

2 years ago
Depends on: 1214433

Updated

2 years ago
Blocks: 1214433
No longer depends on: 1214433
This should be safe to work on now, as long as it's on top of https://hg.mozilla.org/integration/fx-team/rev/958f04b7804fa587506b35b30ec616f986b637ca

I'd suggest choosing the correct icon with something along the following lines:

The preferred size should be `Math.round(16 * window.devicePixelRatio)`. If an icon of that exact size exists, use it. Otherwise, fall back to, in order of preference:

* An icon that's exactly twice that size.
* The smallest icon that's larger than that size.
* The largest icon that's smaller than that size.

Though it may be better to just try for the exact size, twice the preferred size, and then just fall back to the largest.
Mentor: kmaglione+bmo@mozilla.com
(Assignee)

Comment 5

2 years ago
I happen to have implemented _exactly_ that logic in bug 1192432. The function is called AddonManager.getPreferredIconURL and I reckon we can use it in this case as well.

We would need to wait for 1192432 to land.

Cheers
Oh, perfect.

Updated

2 years ago
Flags: blocking-webextensions+
Depends on: 1218175
No longer depends on: 1218175
(Assignee)

Comment 7

2 years ago
Created attachment 8681706 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils
(Assignee)

Comment 8

2 years ago
Comment on attachment 8681706 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

So 1192432 landed and I finally got around doing this. It's lacking tests right now because I'd first like to know if you think this would be a sane approach, Kris.

It should be pretty straightforward, we remove the SIZES array to not limit ourselves anymore and then delegate to AddonManager.getPreferredIconURL to sort out the correct icon for us (from the list of normalized icons).

The only thing I don't like is that getPreferredIconURL is expecting to receive a traditional addon object as it is used internally and so we have to cheat it a bit and pass {icons: icons}.

Note that I left the desired size at 19 for now, because this issue had differing opinions on what the actual desired size would be. But that's trivial to change.
Attachment #8681706 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8681706 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

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

(In reply to Johann Hofmann from comment #8)
> The only thing I don't like is that getPreferredIconURL is expecting to
> receive a traditional addon object as it is used internally and so we have
> to cheat it a bit and pass {icons: icons}.

I don't really have a problem with that, but you should update
the comment at the head of that function to make it clear that
it can take any object with an |icons| property, and not just an
Addon instance.

> Note that I left the desired size at 19 for now, because this issue had
> differing opinions on what the actual desired size would be. But that's
> trivial to change.

I think it would be better to change the desired size to 18. The
default size for a string, rather than an object, should stay at
19, though.

It's probably going to take a fair amount of work to support
32/64px images in the menu panel, though, so that should
probably be a separate bug.

As a side note, I see that |getPreferredIconURL| is using
|parseInt| to check that the sizes are actually integers. It
might be a good idea to add a stricter check there, since
|parseInt| will happilly accept values like "100foo" without
complaint.

Aside from those minor nits, this looks good.

::: browser/components/extensions/ext-utils.js
@@ +23,3 @@
>  
>    // Normalizes the various acceptable input formats into an object
> +  // containing icon URLs.

Please include some kind of description of the keys.

@@ +32,5 @@
>        if (imageData instanceof Cu.getGlobalForObject(imageData).ImageData) {
>          imageData = {"19": imageData};
>        }
>  
> +      for (let size in imageData) {

It would be good to check that |size| is an integer here. Below, too. Also, I'd prefer that you use |let size of Object.keys(imageData)|, for paranoia's sake.
Attachment #8681706 - Flags: feedback?(kmaglione+bmo) → feedback+
(Assignee)

Comment 10

2 years ago
(In reply to Kris Maglione [:kmag] from comment #9)
> Comment on attachment 8681706 [details] [diff] [review]
> Allow flexible icon sizes in ExtensionUtils
> 
> Review of attachment 8681706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's probably going to take a fair amount of work to support
> 32/64px images in the menu panel, though, so that should
> probably be a separate bug.
> 
> As a side note, I see that |getPreferredIconURL| is using
> |parseInt| to check that the sizes are actually integers. It
> might be a good idea to add a stricter check there, since
> |parseInt| will happilly accept values like "100foo" without
> complaint.
 
> It would be good to check that |size| is an integer here.

Turns out that isNaN would be working pretty well for both use cases, so we could put an isNaN check in there. Confirming that it's an integer would be harder and possibly involve bitwise operators or regexes. I'm not sure we should go down that route.

Although I have to say I like that people could input "32px" and it would be coerced into 32.

> I'd prefer that you use |let size of Object.keys(imageData)|, for paranoia's
> sake.

Haha good call.
(In reply to Johann Hofmann from comment #10)
> Turns out that isNaN would be working pretty well for both use cases, so we
> could put an isNaN check in there. Confirming that it's an integer would be
> harder and possibly involve bitwise operators or regexes. I'm not sure we
> should go down that route.

Alas, isNaN also returns false for things like '0x100'. Which isn't a big deal, I guess, but still not really desirable. Your best bet is probably to just go with `/^[1-9]\d*$/.test(key)`. This is JavaScript, after all. Sometimes hoping for a clean solution is just too much.

> Although I have to say I like that people could input "32px" and it would be
> coerced into 32.

It sounds nice, but I think it's asking for trouble. If we accept values like that, some number of people will start using them, and some of those will write tutorials, and people will copy code from those. At which point some of those people will wind up trying values like "1em". And eventually we'll make some change to the code so that it only accepts integers, and all of that code will break.

I know it sounds like a pretty minor thing to worry about, but it's the kind of thing that happens all the time, and makes it so difficult for us to change things without breaking add-ons.
(Assignee)

Comment 12

2 years ago
(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Johann Hofmann from comment #10)
> > Turns out that isNaN would be working pretty well for both use cases, so we
> > could put an isNaN check in there. Confirming that it's an integer would be
> > harder and possibly involve bitwise operators or regexes. I'm not sure we
> > should go down that route.
> 
> Alas, isNaN also returns false for things like '0x100'. Which isn't a big
> deal, I guess, but still not really desirable. Your best bet is probably to
> just go with `/^[1-9]\d*$/.test(key)`. This is JavaScript, after all.
> Sometimes hoping for a clean solution is just too much.

Ugh, okay, it's probably for the best. :)

> > Although I have to say I like that people could input "32px" and it would be
> > coerced into 32.
> 
> It sounds nice, but I think it's asking for trouble. If we accept values
> like that, some number of people will start using them, and some of those
> will write tutorials, and people will copy code from those. At which point
> some of those people will wind up trying values like "1em". And eventually
> we'll make some change to the code so that it only accepts integers, and all
> of that code will break.
> 
> I know it sounds like a pretty minor thing to worry about, but it's the kind
> of thing that happens all the time, and makes it so difficult for us to
> change things without breaking add-ons.

Yeah, absolutely. I meant that rather jokingly :D
(In reply to Johann Hofmann from comment #12)
> Yeah, absolutely. I meant that rather jokingly :D

Heh. Fair enough :)
(Assignee)

Comment 14

2 years ago
Created attachment 8683091 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils
(Assignee)

Updated

2 years ago
Attachment #8681706 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
Comment on attachment 8683091 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

Blake, would you like to ui-review? I'm not sure there's much to see, but it's much more flexible in the background now. Just wanted to pull you in on this since you created the bug.
Attachment #8683091 - Flags: ui-review?(bwinton)
Attachment #8683091 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 16

2 years ago
Comment on attachment 8683091 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

I'm going to give it a ui-r+, based on inspecting the tests.  ;)

As a side note, we might want to think about how we want to handle SVGs, which can be any resolution, particularly since the style guide is likely to recommend people use SVG icons, both for resolution independence, and to let us add a highlight colour to browserActions and pageActions when appropriate.  But that all seems like food for another bug.
Attachment #8683091 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 17

2 years ago
Haha ok, thanks :D

Yeah icons seems to be traditionally not very straightforward. Considering SVGs early on might save us some pain.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #16)
> As a side note, we might want to think about how we want to handle SVGs,
> which can be any resolution, particularly since the style guide is likely to
> recommend people use SVG icons, both for resolution independence, and to let
> us add a highlight colour to browserActions and pageActions when
> appropriate.

I don't think we need to do anything special for SVGs. It's conceivable that people may want to use different SVGs for different icon sizes, or use a PNG for smaller icons, where exact pixel placement may matter more, and SVGs for everything else. As it stands, people can just specify a string pointing to an SVG, rather than an object with multiple sizes, and get the correct behavior, so I can't think of any changes that would be especially useful.
Comment on attachment 8683091 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

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

This looks really good. Thanks. Just needs a couple of minor changes.

I'm not a reviewer for AddonManager, so flagging Mossop for that, but those changes look good to me too.

::: browser/components/extensions/ext-utils.js
@@ +36,5 @@
>        }
>  
> +      for (let size of Object.keys(imageData)) {
> +        if (!INTEGER.test(size)) {
> +          throw Components.Exception("Invalid icon size, must be an integer",

This isn't an XPCOM interface, so please use |new Error| instead of |Components.Exception|. Same below.

@@ +71,5 @@
> +        try {
> +          Services.scriptSecurityManager.checkLoadURIStrWithPrincipal(
> +            extension.principal, url,
> +            Services.scriptSecurityManager.DISALLOW_SCRIPT);
> +        } catch (e if !context) {

Since we're now throwing validation errors elsewhere in the function, can you move this try-catch to the top level, and return an empty object if you hit it? The message should be something like |`Invalid icon data: ${e}`|

::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
@@ +122,5 @@
> +        "48": "48.png",
> +        "128": "128.png" } },
> +        resolutions: {
> +          "1": browser.runtime.getURL("data/18.png"),
> +          "2": browser.runtime.getURL("data/48.png"), } },

Nice tests.

@@ +270,5 @@
> +    }
> +  });
> +
> +  yield extension.startup();
> +  yield extension.awaitFinish();

Unfortunately, there's currently a race in code like this. Please use |Promise.all([extension.startup(), extension.awaitFinish()])| so the result message doesn't get lost.

Also, it's best to pass an explicit message to |awaitFinish| so we're certain the correct extension is being loaded.
Attachment #8683091 - Flags: review?(kmaglione+bmo)
Attachment #8683091 - Flags: review?(dtownsend)
Attachment #8683091 - Flags: review+
(Assignee)

Comment 20

2 years ago
Great, will do!

> This isn't an XPCOM interface, so please use |new Error| instead of |Components.Exception|. Same below.

Oh cool, I was wondering about that. :)
(Reporter)

Comment 21

2 years ago
(In reply to Kris Maglione [:kmag] from comment #18)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #16)
> > As a side note, we might want to think about how we want to handle SVGs,
> > which can be any resolution, particularly since the style guide is likely to
> > recommend people use SVG icons, both for resolution independence, and to let
> > us add a highlight colour to browserActions and pageActions when
> > appropriate.
> 
> I don't think we need to do anything special for SVGs. It's conceivable that
> people may want to use different SVGs for different icon sizes, or use a PNG
> for smaller icons, where exact pixel placement may matter more, and SVGs for
> everything else. As it stands, people can just specify a string pointing to
> an SVG, rather than an object with multiple sizes, and get the correct
> behavior, so I can't think of any changes that would be especially useful.

I mostly agree, but the line that worried me a little was: 'this.convertImageDataToPNG(imageData[size], context);'  ;)
(In reply to Blake Winton (:bwinton) (:☕️) from comment #21)
> I mostly agree, but the line that worried me a little was:
> 'this.convertImageDataToPNG(imageData[size], context);'  ;)

That's only for ImageData objects :) They're always raster anyway. Image URLs stay as image URLs.
(Assignee)

Comment 23

2 years ago
Created attachment 8684083 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils
(Assignee)

Updated

2 years ago
Attachment #8683091 - Attachment is obsolete: true
Attachment #8683091 - Flags: review?(dtownsend)
(Assignee)

Comment 24

2 years ago
Comment on attachment 8684083 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

Mossop, this should be ready to review now

Kris, I implemented the changes you suggested, would be cool if you could take another look to confirm that's what you meant :)
Attachment #8684083 - Flags: review?(dtownsend)
Attachment #8684083 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8684083 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

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

Sorry, needs a bit more work. My fault for not being clearer.

::: browser/components/extensions/ext-browserAction.js
@@ +39,5 @@
>      popup = extension.baseURI.resolve(popup);
>    }
>  
> +  let icon = {};
> +  try {

I'd rather keep this try-catch inside the normalize function so we don't need it in two places. Sorry I wasn't clearer.

@@ +45,5 @@
> +      { path: options.default_icon },
> +      extension,
> +      null,
> +      true
> +    );

Please keep this to two lines.

::: browser/components/extensions/ext-utils.js
@@ +26,5 @@
>    // Normalizes the various acceptable input formats into an object
> +  // with icon size as key and icon URL as value.
> +  //
> +  // throws an error if an invalid icon size was provided or the
> +  // extension is not allowed to load the specified resources

Please capitalize the first word and add a period.

@@ +70,5 @@
> +        // The Chrome documentation specifies these parameters as
> +        // relative paths. We currently accept absolute URLs as well,
> +        // which means we need to check that the extension is allowed
> +        // to load them.
> +        try {

No need for a try-catch here once we have an outer one.

::: browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
@@ +386,5 @@
>      message: new RegExp(`Loading extension.*Access to.*'${url}' denied`),
>    });
>  
> +  // because the underlying method throws an error on invalid data,
> +  // only the first invalid URL of each component will be logged

Please always capitalize the first word of comments, and add a period when it's more than one line.
Attachment #8684083 - Flags: feedback?(kmaglione+bmo) → review-
(Assignee)

Comment 26

2 years ago
(In reply to Kris Maglione [:kmag] from comment #25)
> Comment on attachment 8684083 [details] [diff] [review]
> Allow flexible icon sizes in ExtensionUtils
> 
> Review of attachment 8684083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, needs a bit more work. My fault for not being clearer.

No worries, it's good that you're thorough. :)

> ::: browser/components/extensions/ext-browserAction.js
> @@ +39,5 @@
> >      popup = extension.baseURI.resolve(popup);
> >    }
> >  
> > +  let icon = {};
> > +  try {
> 
> I'd rather keep this try-catch inside the normalize function so we don't
> need it in two places. Sorry I wasn't clearer.
> 

I actually assumed you meant that, but the problem is that the setIcon function in Chrome is throwing an error if the icon size is incorrect. If I put a big try-catch around that here it won't fail in Firefox anymore but rather just log a warning. Since it's a public facing API I think we should follow the behavior of the Chrome API.

That's also the reason why I 

- didn't remove the inner try-catch and made it throw a better worded error
- don't catch any exception in the normalize call in setIcon(), extension authors are supposed to handle it here
(In reply to Johann Hofmann [:johannh] from comment #26)
> I actually assumed you meant that, but the problem is that the setIcon
> function in Chrome is throwing an error if the icon size is incorrect. If I
> put a big try-catch around that here it won't fail in Firefox anymore but
> rather just log a warning. Since it's a public facing API I think we should
> follow the behavior of the Chrome API.

That's what the `if !context` guard in the current try-catch is for. Although, as catch guards are a non-standard feature, that should probably be replaced with something like  `if (context) { throw e; }` in the catch block.

> - didn't remove the inner try-catch and made it throw a better worded error

That's such a rare corner case that I'm not worried about adding a second try-catch just for a better error. We need the check for security, but if people hit it, it's almost certainly because they're doing something shady.
(Assignee)

Comment 28

2 years ago
(In reply to Kris Maglione [:kmag] from comment #27)
> (In reply to Johann Hofmann [:johannh] from comment #26)
> > I actually assumed you meant that, but the problem is that the setIcon
> > function in Chrome is throwing an error if the icon size is incorrect. If I
> > put a big try-catch around that here it won't fail in Firefox anymore but
> > rather just log a warning. Since it's a public facing API I think we should
> > follow the behavior of the Chrome API.
> 
> That's what the `if !context` guard in the current try-catch is for.
> Although, as catch guards are a non-standard feature, that should probably
> be replaced with something like  `if (context) { throw e; }` in the catch
> block.

Ooh right, I'm braindead. Didn't think of using that. Sorry, I'll prepare a new patch.

> > - didn't remove the inner try-catch and made it throw a better worded error
> 
> That's such a rare corner case that I'm not worried about adding a second
> try-catch just for a better error. We need the check for security, but if
> people hit it, it's almost certainly because they're doing something shady.

Ok, if you say so :)
Comment on attachment 8684083 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

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

I'll look once the new patch is ready.
Attachment #8684083 - Flags: review?(dtownsend)
(Assignee)

Comment 30

2 years ago
Created attachment 8686052 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils
(Assignee)

Updated

2 years ago
Attachment #8684083 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Comment on attachment 8686052 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

Ok, implemented the things you remarked. :)

Want to give it another look before we pass it on to :Mossop?
Attachment #8686052 - Flags: review?(kmaglione+bmo)
Comment on attachment 8686052 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

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

Looks good. Thanks again!
Attachment #8686052 - Flags: review?(kmaglione+bmo)
Attachment #8686052 - Flags: review?(dtownsend)
Attachment #8686052 - Flags: review+
Comment on attachment 8686052 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils

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

r=me for the AddonManager pieces
Attachment #8686052 - Flags: review?(dtownsend) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af39280ac004
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Hi, this failed to apply:

applying name.patch
patching file browser/components/extensions/ext-utils.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file browser/components/extensions/ext-utils.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh name.patch
Tomcats-MacBook-Pro-2:fx-team Tomcat$ 


can you take a look, thanks!
Flags: needinfo?(mail)
Keywords: checkin-needed
(Assignee)

Comment 36

2 years ago
Sure, I'll update the patch. Thanks!
Flags: needinfo?(mail)
(Assignee)

Comment 37

2 years ago
Created attachment 8691454 [details] [diff] [review]
Allow flexible icon sizes in ExtensionUtils
(Assignee)

Updated

2 years ago
Attachment #8686052 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55ebf990594
(Assignee)

Comment 39

2 years ago
So the try is looking good I guess. Kris, could you give it a quick look to make sure I merged correctly? Thanks!
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Looks good to me
Flags: needinfo?(kmaglione+bmo)

Comment 41

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/04d7b2ac3ff8
Keywords: checkin-needed

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04d7b2ac3ff8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

2 years ago
Iteration: --- → 45.2 - Nov 30

Comment 43

2 years ago
Thanks Johann! Your contribution has been added to https://wiki.mozilla.org/Add-ons/Contribute/Recognition#November_2015
Duplicate of this bug: 1234088
Keywords: dev-doc-needed
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action, in particular https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Choosing_icon_sizes

Let me know if this is all right. Your comment 4 talked about picking an icon twice the best size, but I didn't see that in the code: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#2301.
Flags: needinfo?(kmaglione+bmo)
(In reply to Will Bamberg [:wbamberg] from comment #45)
> Your comment 4 talked about picking an icon twice the best size, but I
> didn't see that in the code:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> AddonManager.jsm#2301.

Ah. Indeed, it does not. *shrug*

> ->
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> browser_action, in particular
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> browser_action#Choosing_icon_sizes
> 
> Let me know if this is all right.

It should also mention menu panel buttons, which use 32px or 64px icons,
depending on pixel density.
Flags: needinfo?(kmaglione+bmo)
OK, I've added that: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action#Choosing_icon_sizes .

(ultimately this is the kind if thing we should have in a separate UX guide, I think)
Flags: needinfo?(kmaglione+bmo)
(In reply to Will Bamberg [:wbamberg] from comment #47)
> OK, I've added that:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/
> browser_action#Choosing_icon_sizes .

Looks good.

> (ultimately this is the kind if thing we should have in a separate UX guide,
> I think)

Agreed. I think Blake's working on that.
Flags: needinfo?(kmaglione+bmo) → needinfo?(bwinton)
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Comment 49

a year ago
It'll live somewhere under http://firefoxux.github.io/StyleGuide/#/home (That part in particular is planned to be filled in by the end of this week. :)
Flags: needinfo?(bwinton)
Duplicate of this bug: 1272219
Duplicate of this bug: 1272222
You need to log in before you can comment on or make changes to this bug.