1345716 - Dev Edition (compact dark/light) theme: solid grayscale circle appears instead of blue (i) icon in location bar for icons used by add-ons

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[684sigma]

I have a problem with Firefox Beta 52. It doesn't happen in Firefox ESR 45.
Sometimes solid grayscale circle appears in location bar, while it's expected that blue (i) icon should appear.
It happens unpredictably and I don't remember all cases, however, I noticed one specific scenario when it happens

1. Install https://addons.mozilla.org/en-US/firefox/addon/devedition-theme-enabler/ , enable developer theme
2. Install https://addons.mozilla.org/en-US/firefox/addon/greasemonkey , restart the browser
3. Open http://example.org , click dropmarker in greasemonkey button, click "New user script", create a script


Result: solid grayscale circle appears in location bar
Expected: blue (i) icon should appear

DevEdition theme is my default install ever since curved tabs of Australis theme were implemented.

Comment 1

[:Gijs (he/him)]

Can you show a screenshot of what you're seeing? Does this happen in Firefox 53 (https://beta.mozilla.org/ ), where you no longer need an add-on to have the 'devedition' theme (now called 'compact light' and 'compact dark') ?

Comment 2

[684sigma]

Attachment: bug 1345716 detailed, by reporter.png

Screenshot attached.
Yes, it does happen in Firefox Beta 53.

Comment 3

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression in behavior of prominent add-ons

Range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bc9d199d895c33f1b4984afe61ec464a43a2a61a&tochange=c55bcb7c777ea09431b4d16903ed079ae5632648

This looks like bug 1304708 to me. Dão, any idea what's going on here?

Comment 4

[:Gijs (he/him)]

Final window from mozregression: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0ca8b0d966ee28bc6542a762f8a592cf76054d8&tochange=a481cba452933d5e7571fce7733b1ad7d41f493b

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #3)
> This looks like bug 1304708 to me. Dão, any idea what's going on here?

It's this change: https://hg.mozilla.org/mozilla-central/rev/a481cba45293#l4.12

I hadn't considered that add-ons might have multicolored icons.

Comment 6

[Johann Hofmann [:johannh]]

For simpler STR, you can reproduce this in compact theme by entering 

PopupNotifications.show(gBrowser.selectedBrowser, "test", "hello", null, {callback: () => {}, label: "ok", accessKey: "o"}, [], {})

in the browser toolbox console.

Comment 7

[Johann Hofmann [:johannh]]

(We probably also want to get rid of that old icon)

Comment 8

[Dão Gottwald [:dao]]

Ah, so this isn't actually an add-on icon. In that case, since there were no other complaints, maybe we can leave the code as is and just replace this icon.

Comment 9

[Johann Hofmann [:johannh]]

Yeah, I'm not even sure if it would be easy for add-ons to add an icon in the identity block. 

Realistically with the old extension API going away access to PopupNotifications would no longer be possible anyway. Bug 1331557 is tracking access for WebExtensions but I already mentioned that these should probably be anchored at the browser action icon of the extension instead.

So this icon, in whatever form, will likely not be publicly visible anymore after 57 if the deprecation timeline still stands. It might still be useful to have a fallback icon for internal usage I guess.

Comment 10

[Dão Gottwald [:dao]]

Attachment: Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

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

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

https://reviewboard.mozilla.org/r/119146/#review121496

Hm, the icon looks a bit different than the identity icon for me which is a bit irritating. I'll attach a screenshot. Probably need to fiddle with the size values.

I think I'd be fine with landing this if the small differences to the identity icon are resolved but I'm secretly hoping someone has a 16x16 SVG dialog icon lying around to replace this instead. Maybe there's a different icon we could re-use instead?

The proper way to solve this would probably be to get someone from UX to chime in, at the very least to say "this is almost invisible to users, so do whatever you like".

Philipp, do you have any feedback and/or ideas on how to solve this?

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Slightly different

Comment 13

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #11)
> I think I'd be fine with landing this if the small differences to the
> identity icon are resolved but I'm secretly hoping someone has a 16x16 SVG
> dialog icon lying around to replace this instead. Maybe there's a different
> icon we could re-use instead?

We have a few other info icons but they all come with their own differences.

> The proper way to solve this would probably be to get someone from UX to
> chime in, at the very least to say "this is almost invisible to users, so do
> whatever you like".

It think we've already established that this invisible to most users :)

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

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

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

https://reviewboard.mozilla.org/r/119146/#review121666

Comparing them again I think the new flat style isn't that far off the original. The old logo was also an (i) and yeah, this is only a fallback.

r=me

Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22d5bcbc967a
Make default notification anchor icon be part of notification-icons.svg. r=johannh

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22d5bcbc967a

Comment 18

[Gerry Chang [:gchang]]

Track 53+ as regression.

Hi ::dao,
Since this also affects 53/53, do you think the patch is worth uplifting to 53/54?

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1304708
[User impact if declined]: see comment 0
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: STR in comment 6
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: it just replaces an old icon with a new one. pretty straightforward.
[String changes made/needed]: /

Comment 20

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 21

[Hani Yacoub, Desktop QA]

Build ID: 20170320030209
User AgentMozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

Fix an UI regression that the icon is shown incorrectly in the location bar and was verified. Beta53+ & Aurora54+.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb8eade361cb

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/60a2d0da9096

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

See comment 19. Grafts cleanly to ESR52.

Comment 26

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

I can confirm this issue is fixed, I verified using Firefox 53.0b6, 54.0a2 on Win 8.1 x64 and Mac OS X 10.11 and Ubuntu 14.04 x64.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8846060 [details]
Bug 1345716 - Make default notification anchor icon be part of notification-icons.svg.

I guess this can't hurt.  UI regression fix, esr52+.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/2569af645a98

Comment 29

[Andrei Vaida [:avaida]]

Timea, could you please take a look at this on 53b10 as well?

Comment 30

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

I can confirm this issue is fixed, I verified using Firefox 53.0b10 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.
I will re-verify this issue using Fx 52.1esr once the build is available.

Comment 31

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

I can confirm this issue is fixed, I verified using Firefox 52.1esr on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.