1370224 - Browser and page action should default to extension icon rather before puzzle piece

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[sblask]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170524180630

Steps to reproduce:

I am trying to make a webextension compatible to Chrome and Firefox. As default_icon for a browser action in manifest.json is listed as optional in Chrome and there is nothing about it under Browser Compatibility on https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/browser_action I expected it to be optional in Firefox as well.


Actual results:

There is no icon in the toolbar if no default_icon is configured.  


Expected results:

The extensions default icon should be used if no default_icon for the browser action is defined.

Comment 1

[Kris Maglione [:kmag]]

This property is definitely optional, and the toolbar button definitely appears if you omit it. Can you test in a clean profile, and provide more detailed steps to reproduce?

Comment 2

[sblask]

I expressed myself badly. The extension still works and there is an icon, just not the icon of the extension, but the default green jigsaw-puzzle icon.

I encountered it with https://github.com/sblask/firefox-simple-form-fill - edit manifest.json, remove line 17 and then run "npm run start".

Comment 3

[Trishul]

I want to try my hands on this.

Comment 4

[apoorvasingh2811]

Hi, I'm a beginner and would like to work on this as my first bug. Can I take it up?

Comment 5

[:Tomislav Jovanovic :zombie]

Hi Trishul and apoorvasingh,

Sorry for the slow reply, but if either of you are still interested in working on this, just reply and we'll assign the bug to you.

If this is your first bug, you can find some introduction docs at:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Feel free to ask if you have any questions, or if you get stuck, and use the need-info flag, for either me or :mstriemer (mentor).

Comment 6

[apoorvasingh2811]

(In reply to Tomislav Jovanovic :zombie from comment #5)
> Hi Trishul and apoorvasingh,
> 
> Sorry for the slow reply, but if either of you are still interested in
> working on this, just reply and we'll assign the bug to you.
> 
> If this is your first bug, you can find some introduction docs at:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
> 
> Feel free to ask if you have any questions, or if you get stuck, and use the
> need-info flag, for either me or :mstriemer (mentor).

I'm interested in working on this. Can you please give me some pointers?

Comment 7

[Caitlin Neiman [:caitmuenster]]

Apoorva, I completely missed your reply to this! I am so sorry. 

Like zombie said in comment 5, the best place to start is onboarding to the Firefox code base (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction). You might also want to read this blog post about landing patches to WebExtensions APIs: https://blog.mozilla.org/addons/2018/08/14/building-extension-apis-oriol-brafau/

Let us know if you're still interested in this bug and :needinfo zombie if you need any help with next steps after you have set up your dev environment.

Comment 8

[Caitlin Neiman [:caitmuenster]]

Hey Mark! Can you provide some more information about where the code is located? 

If this is your first good-first-bug, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for instructions on how to get started.

Comment 9

[Nicklas Boman [:smurfd]]

Hey, i could have a go at this.
Is it possible in this code where change is needed?

https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-browserAction.js#83-95

Comment 10

[Mark Striemer [:mstriemer]]

The icon gets cached in `this.defaults.icon` in ext-browserAction.js. It's currently just looking at `browser_action.default_icon` from the manifest but it should also check the extensions icon.

You can get the extension's `icons` object with `extension.manifest.icons` and that object should be stored as the `path` if there is no `default_icon` [1].

[1] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/browser/components/extensions/parent/ext-browserAction.js#86

Comment 11

[Nicklas Boman [:smurfd]]

Attachment: Bug1370224_181019_2301.patch

Hey, not sure if i have over-simplified this.
But the attached patch gives the expected results. 

What kind of tests should i run?

Comment 12

[Mark Striemer [:mstriemer]]

That looks good, thanks!

I've started a try build [1] with your changes which should run all the related tests to this. If there are some failures you might be able to just fix those tests without writing a new one. Otherwise following the simple test case [2] as an example you could write up a new one. There are some other icon related tests in that folder that could help as well.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9d9a818387fa42024de6a43894083f62bdcee43
[2] https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_browserAction_simple.js

Comment 13

[Nicklas Boman [:smurfd]]

The test seems to have passed, right? \o/

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9018728 [details] [diff] [review]
Bug1370224_181019_2301.patch

I stumbled across this, this is fine.

Comment 15

[Mark Striemer [:mstriemer]]

Yup, can you add a new test that checks that the extension's icon is used?

It looks like there's a variable sitting around called `imageBuffer` you can use as an icon. You can set that as the extension's icon in the test from browser_ext_browserAction_simple.js and test that the icon is used with `getBrowserActionWidget(extension).forWindow(window)`, I think you'll want to check `node` on the result but you can check other calls of getBrowserActionWidget in searchfox.

    manifest: {
      files: {"icon.png": imageBuffer},
      icons: {32: "icon.png"},
      // ...
    }

Comment 16

[Nicklas Boman [:smurfd]]

  let widget = getBrowserActionWidget(extension).forWindow(window);
  is(widget.node.getAttribute("disabled"), "true", "Button is disabled");

I guess i could use something like that, right. But what Attribute would i be looking for? (or how do i figure out what attribute to look for? :)

Comment 17

[Mark Striemer [:mstriemer]]

You can use the Browser Toolbox to inspect the browser chrome, hotkey should be something like (Ctrl|Command)+Alt+Shift+I, or through the Web Developer entry in the main app menu.

XUL images will treat list-style-image as the image source, so that's the attribute you'll want to check. It gets passed down from the parent and it look like you can check it on the toolbarbutton itself (widget.node). Checking if that attribute includes your image path should be enough.

Comment 18

[Nicklas Boman [:smurfd]]

So i did like this, after noticing via Inspecting the icon of the extension that its id would be random, and it is run after the  await extension.startup(); :
    let widget = getBrowserActionWidget(extension).forWindow(window);
    let icon = widget.node.ownerGlobal
        .getComputedStyle(document.getElementById(widget.node.getAttribute("widget-id")))
        .getPropertyValue("list-style-image");

    console.log("icon = " + icon + "\n");

That gave me This output : 
icon = url("chrome://browser/content/extension.svg")

Since i had "icon.png": imageBuffer, in the Manifest of the extension, i kindof expected the output to be "icon.png" ?
The icon it chose _was_ the green puzzlem so i guess in a way its correct?

Comment 19

[Mark Striemer [:mstriemer]]

I think my suggestion from comment 15 wasn't quite right. You'll want to update the extension to have (although it looks like having the file isn't strictly necessary):

    manifest: {
      icons: {32: "icon.png"}, // This is the important part.
      // ...
    },
    files: {
      "icon.png": imageBuffer,
      // ...
    },

You should be able to use getComputedStyle globally already like:

    let widget = getBrowserActionWidget(extension).forWindow(window);
    let image = getComputedStyle(widget.node).listStyleImage;
    ok(image.includes("/icon.png"), "The extension's icon is used");

Comment 20

[Nicklas Boman [:smurfd]]

Attachment: Bug1370224_181104_2010.patch

Right, i had 
  icons: {32: "icon.png"},
But in the wrong place.

Now it works :)

I thought we wanted to get the "icon.png" only, thats why i did try to get the Extension ID.

Comment 21

[Mark Striemer [:mstriemer]]

Comment on attachment 9022467 [details] [diff] [review]
Bug1370224_181104_2010.patch

This looks good, thanks!

Comment 22

[Mark Striemer [:mstriemer]]

Comment on attachment 9022467 [details] [diff] [review]
Bug1370224_181104_2010.patch

Have you setup arcanist [1]? We normally review code on phabricator and that makes it a bit easier to land things once they're ready. If you'd like to submit more patches in the future then getting your patch on phabricator would be nice.

Just going to confirm Shane is okay with this test.

[1] https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#setting-up-arcanist

Comment 23

[Nicklas Boman [:smurfd]]

Thanks Mark, will look into getting phabricator setup. Attempted it once and it was something that i ran into, cant remember right now, but didnt pursue it.

I guess asking in #developers if i run into some problems.
(also thanks Shane)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab318a0ae56
Fallback to extension icon for browser/page action r=mstriemer,mixedpuppy

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8ab318a0ae56

Comment 26

[Vlad Jiman]

It is my understanding that this bug was covered by automated tests, should manual validation also be performed? if not please set the qe-verify- flag. Thank you

Comment 27

[Andrew Swan [:aswan]]

qe-verify- per conversation with smurfd on irc.