Closed Bug 1370224 Opened 3 years ago Closed Last year

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

Categories

(WebExtensions :: Frontend, defect, P5)

53 Branch
defect

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: blask, Assigned: smurfd, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: triaged)

Attachments

(1 file, 1 obsolete file)

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.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
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?
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".
Status: UNCONFIRMED → NEW
Component: WebExtensions: General → WebExtensions: Frontend
Ever confirmed: true
Summary: Browser action's default_icon not optional → Browser and page action should default to extension icon rather before puzzle piece
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: triaged
Mentor: mstriemer
I want to try my hands on this.
Hi, I'm a beginner and would like to work on this as my first bug. Can I take it up?
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).
(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?
Product: Toolkit → WebExtensions
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.
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.
Flags: needinfo?(mstriemer)
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
Assignee: nobody → smurfd
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
Flags: needinfo?(mstriemer)
Attached patch Bug1370224_181019_2301.patch (obsolete) — Splinter Review
Hey, not sure if i have over-simplified this.
But the attached patch gives the expected results. 

What kind of tests should i run?
Flags: needinfo?(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
Flags: needinfo?(mstriemer)
The test seems to have passed, right? \o/
Flags: needinfo?(mstriemer)
Comment on attachment 9018728 [details] [diff] [review]
Bug1370224_181019_2301.patch

I stumbled across this, this is fine.
Attachment #9018728 - Flags: review+
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"},
      // ...
    }
  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? :)
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.
Flags: needinfo?(mstriemer)
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?
Flags: needinfo?(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");
Flags: needinfo?(mstriemer)
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.
Attachment #9018728 - Attachment is obsolete: true
Flags: needinfo?(mstriemer)
Comment on attachment 9022467 [details] [diff] [review]
Bug1370224_181104_2010.patch

This looks good, thanks!
Flags: needinfo?(mstriemer)
Attachment #9022467 - Flags: review+
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
Attachment #9022467 - Flags: review?(mixedpuppy)
Attachment #9022467 - Flags: review?(mixedpuppy) → review+
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)
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab318a0ae56
Fallback to extension icon for browser/page action r=mstriemer,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/8ab318a0ae56
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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
Flags: needinfo?(smurfd)
qe-verify- per conversation with smurfd on irc.
Flags: needinfo?(smurfd) → qe-verify-
You need to log in before you can comment on or make changes to this bug.