1395284 - Wrong color for the Screenshots icon when added to the URL Bar.

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[bbell]

Attachment: example

When the Screenshots unitilty is added to the URL Bar it appears black. It should be the gray.

Comment 1

[John Gruen [:jgruen]]

Not sure why this is happening, we use context-fill within the SVG (which is signed by mozilla and therefore whitelisted). Seems like it should inherit values.

Comment 2

[Drew Willcoxon :adw]

This is probably why:

"Note: This feature is available since Firefox 55, but is only currently supported with SVG images loaded via  chrome:// or resource:// URLs. To experiment with the feature in SVG on the Web it is necessary to set the svg.context-properties.content.enabled pref to true."

https://developer.mozilla.org/en-US/docs/Web/CSS/-moz-context-properties

For screenshots, the icon URL ends up being a file:// URI, like this:

> file:///Users/adw/mc/obj-artifact/dist/Nightly.app/Contents/Resources/browser/features/[e-mail]/webextension/icons/icon-32-v2.svg

I'm not sure how to fix this.  I guess it's the WebExtension machinery that's deciding to use a file URI?  Maybe we could set up a resource URI just for this, within bootstrap.js.  Yet one more thing to reconcile between WebExtensions page actions and Photon page actions.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1395284 - Wrong color for the Screenshots icon when added to the URL Bar.

Review commit: https://reviewboard.mozilla.org/r/174650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174650/

Comment 4

[Drew Willcoxon :adw]

Jared, what do you think about this?  There are a couple of problems.  The main one I described in comment 2.  The second problem is that the screenshots svg doesn't specify fill-opacity="context-fill-opacity".  I'll have to fix that in the GitHub repo I guess, and then it'll have to be merged back to m-c.

(I tested this with the svg fixed and it works BTW.)

Comment 5

[Drew Willcoxon :adw]

Comment on attachment 8902925 [details]
Bug 1395284 - Wrong color for the Screenshots icon when added to the URL Bar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174650/diff/1-2/

Comment 6

[Drew Willcoxon :adw]

Slight tweak.

Comment 7

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8902925 [details]
Bug 1395284 - Wrong color for the Screenshots icon when added to the URL Bar.

https://reviewboard.mozilla.org/r/174650/#review179840

How does Screenshots provide this path? Their icon was working with context-fill when it was a standalone toolbar button.

I'm OK with this patch, but maybe this patch isn't necessary if Screenshots uses the same URI it was using when it existed as a toolbar button?

Comment 8

[Drew Willcoxon :adw]

Interesting, are you sure about that?  That it didn't just work or look right by accident?  I can go back and look at an older build tomorrow to double check.  Because according to that MDN page, it seems like it shouldn't have worked, and that's what I seem to be seeing in practice too.  But page actions could certainly be missing something or not doing something that it should be to get this to work.

It's setting its icon here, when it creates the action: https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/browser/extensions/screenshots/bootstrap.js#254

That iconURL for me ends up as the URL in comment 2.

And then page actions sets style.listStyleImage on the urlbar button here: https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/browser/base/content/browser-pageActions.js#346

Oh, I just realized that it was going through the WebExtension machinery previously, when it was a toolbar button.  Tomorrow I'll take a look at that to see what it was doing.

Comment 9

[Drew Willcoxon :adw]

(Maybe it was making a resource URL too!)

Comment 10

[Drew Willcoxon :adw]

In fact I'm pretty sure it was IIRC

Comment 11

[Drew Willcoxon :adw]

The WebExtension toolbar gets a moz-extension URI for its icon:

> list-style-image: moz-extension://04c32d7f-9789-8e45-9910-9547602bf71f/icons/icon-32-v2.svg

There's a great code comment here explaining that moz-extension triggers SVG context painting, same as chrome and resource: https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/layout/svg/SVGContextPaint.cpp#39

So I guess the right fix for this bug is to use the same moz-extension URI that the toolbar icon was using.  My experience is that it's hard to reach into the WebExtension machinery from browser chrome, in this case to get the proper URI, so I'm guessing that might be tricky to do, but I'll look into it.

Comment 12

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #11)
> The WebExtension toolbar gets a moz-extension URI for its icon:

*toolbar button

Comment 13

[Drew Willcoxon :adw]

Yeah, using a moz-extension URI is the way to go.  That means there are no changes necessary to BrowserPageActions since the URI is generated in screenshots's bootstrap.js.  I'll make a GitHub pull request for the necessary screenshots changes and link to it here.

Comment 14

[Drew Willcoxon :adw]

Please see the PR: https://github.com/mozilla-services/screenshots/pull/3451

Comment 15

[Drew Willcoxon :adw]

Ian merged the PR (thanks!), so now this bug depends on merging the addon in the GitHub repo to m-c.

Comment 16

[Drew Willcoxon :adw]

Bug 1399615 has landed, so this is now fixed.

Comment 17

[u554753]

Verified on Windows 10 64bit, Mac OS X 10.11, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) as of today's date.

Comment 18

[Timea Zsoldos [:zstimi/tzsoldos], Desktop Release QA]

I have reproduced this issue using Firefox  57.0a1 (build ID:20170830220349) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 57.0b7 on Windows 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.11.