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

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Theme
VERIFIED FIXED
2 months ago
16 days ago

People

(Reporter: 684sigma, Assigned: dao)

Tracking

({regression})

52 Branch
Firefox 55
regression
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox53+ verified, firefox54 verified, firefox55 verified, firefox-esr52 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 months ago
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.

Updated

2 months ago
Component: Untriaged → Theme
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') ?
Flags: needinfo?(684sigma)
(Reporter)

Comment 2

2 months ago
Created attachment 8845563 [details]
bug 1345716 detailed, by reporter.png

Screenshot attached.
Yes, it does happen in Firefox Beta 53.
Flags: needinfo?(684sigma)
[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?
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox53: --- → ?
Flags: needinfo?(dao+bmo)
Keywords: regression
Summary: Regression DevEdition theme: solid grayscale circle appears instead of blue (i) icon in location bar sometimes → Dev Edition (compact dark/light) theme: solid grayscale circle appears instead of blue (i) icon in location bar for icons used by add-ons
Blocks: 1304708
Final window from mozregression: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0ca8b0d966ee28bc6542a762f8a592cf76054d8&tochange=a481cba452933d5e7571fce7733b1ad7d41f493b
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

2 months ago
(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.
Flags: needinfo?(dao+bmo)
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.
(We probably also want to get rid of that old icon)
(Assignee)

Comment 8

2 months ago
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.
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 hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Assignee: nobody → dao+bmo

Comment 11

2 months ago
mozreview-review
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?
Attachment #8846060 - Flags: review?(jhofmann)
Flags: needinfo?(philipp)
Created attachment 8846635 [details]
Slightly different
(Assignee)

Comment 13

2 months ago
(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 hidden (mozreview-request)

Comment 15

2 months ago
mozreview-review
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!
Attachment #8846060 - Flags: review?(jhofmann) → review+

Comment 16

2 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22d5bcbc967a
Make default notification anchor icon be part of notification-icons.svg. r=johannh
(Assignee)

Updated

2 months ago
Flags: needinfo?(philipp)

Comment 17

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22d5bcbc967a
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

a month ago
Flags: qe-verify+
Track 53+ as regression.

Hi ::dao,
Since this also affects 53/53, do you think the patch is worth uplifting to 53/54?
tracking-firefox53: ? → +
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 19

a month ago
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]: /
Flags: needinfo?(dao+bmo)
Attachment #8846060 - Flags: approval-mozilla-beta?
Attachment #8846060 - Flags: approval-mozilla-aurora?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)

Comment 21

a month ago
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(brindusa.tot)
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+.
Attachment #8846060 - Flags: approval-mozilla-beta?
Attachment #8846060 - Flags: approval-mozilla-beta+
Attachment #8846060 - Flags: approval-mozilla-aurora?
Attachment #8846060 - Flags: approval-mozilla-aurora+

Comment 23

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb8eade361cb
status-firefox54: affected → fixed

Comment 24

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/60a2d0da9096
status-firefox53: affected → fixed
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.
Attachment #8846060 - Flags: approval-mozilla-esr52?

Comment 26

29 days ago
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.
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: qe-verify+
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+.
Attachment #8846060 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 28

28 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/2569af645a98
status-firefox-esr52: affected → fixed
Timea, could you please take a look at this on 53b10 as well?
Flags: needinfo?(timea.zsoldos)

Comment 30

18 days ago
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.
Flags: needinfo?(timea.zsoldos)

Comment 31

16 days ago
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.
status-firefox-esr52: fixed → verified
You need to log in before you can comment on or make changes to this bug.