Closed Bug 1345716 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Theme, defect)

52 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- verified
firefox53 + verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: 684sigma, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
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)
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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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)
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.
Assignee: nobody → dao+bmo
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)
(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 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+
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
Flags: needinfo?(philipp)
https://hg.mozilla.org/mozilla-central/rev/22d5bcbc967a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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?
Flags: needinfo?(dao+bmo)
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)
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
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 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?
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 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+
Timea, could you please take a look at this on 53b10 as well?
Flags: needinfo?(timea.zsoldos)
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)
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.
You need to log in before you can comment on or make changes to this bug.