1598710 - New message icon on address bar is shown as the information icon if the notification appears when the browser is not focused/active

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P3)

Description

[Florencia Di Ciocco, NI to vbalducci]

Attachment: info icon.png

Steps to reproduce:
This bug is extremely random. I will mention the steps that led to us running into this, but they are not reliable to reproduce the bug.

1. Download the latest version of Nightly from the official page on Mac 10.14.
   Open Nightly with a new profile
   Load zdnet.com

2. Open up an old dated Firefox Nightly (from two days ago in this case)
   Load zdnet.com and verify that it's the correct icon
   Go to "Options - Help - About Nightly", wait for the "Download Updates" to finish and restart the browser
   On zdnet.com press the new notification button
   Refresh the browser
   Check that the icon has changed

Expected results:
New message icon to remain and be the only one visible

Actual results:
Sometimes the prompt is accompanied by an info icon instead of the new message icon.

Notes: We've always seen this bug on 72.0a1 (2019-11-22) (64-bit). Since I can't reproduce the bug I can't be sure that this hasn't happened on the older version.

Comment 1

[(no longer active)]

Gijs, Johann is out. I'm wondering if this bug makes you think of anything? Not sure if it's super important in itself but I'm worried if it's the sign of an underlying problem that could affect other things?

Comment 2

[:Gijs (he/him)]

It looks like the (i) icon is the fallback icon at https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/browser/themes/shared/notification-icons.inc.css#28-33 and the desktop notification one is defined a little below at https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/browser/themes/shared/notification-icons.inc.css#54 .

I don't know why we'd ever display the wrong icon. There are roughly 2 possibilities:

1. we somehow picked the wrong anchor element
2. we picked the right anchor element but somehow the wrong style is displaying.

I can't reproduce at all so I don't know what is going on.

Florencia, can you reproduce this like some of the time, or something? If so, can you try using the browser toolbox ( https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox ) and inspecting the icon (use the button in the top left of the browser toolbox to "pick an element" to select the icon), and taking a screenshot of the browser toolbox after inspecting the icon?

Comment 3

[:Gijs (he/him)]

If this is running with webrender, it's possible this is related to bug 1598819, I guess...

Comment 4

[Florencia Di Ciocco, NI to vbalducci]

(In reply to :Gijs (he/him) from comment #2)

Florencia, can you reproduce this like some of the time, or something? If so, can you try using the browser toolbox ( https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox ) and inspecting the icon (use the button in the top left of the browser toolbox to "pick an element" to select the icon), and taking a screenshot of the browser toolbox after inspecting the icon?

Hi Gijs, no, I wasn't able to reproduce it either but Rares has been able to reproduce it. Maybe he can help us.

Comment 5

[Rares Doghi, Desktop QA]

Hi , I tried to reproduce this issue in our latest Nightly build 72.0a1 (2019-11-26) with a fresh profile but I can't reproduce it anymore. In the older build this issue would reproduce 100% of the time. Maybe we can close this issue as works for me and reopen if it reoccurs ?

Comment 6

[:Gijs (he/him)]

(In reply to Rares Doghi from comment #5)

Hi , I tried to reproduce this issue in our latest Nightly build 72.0a1 (2019-11-26) with a fresh profile but I can't reproduce it anymore. In the older build this issue would reproduce 100% of the time. Maybe we can close this issue as works for me and reopen if it reoccurs ?

WFM!

Comment 7

[Sebastian Padilla]

Attachment: lebug.jpg

Hi guys! This is still happening so I'm reopening this bug.

This time it appeared once I switched back and forth between FF and Slack.

It is a very random bug but it does occur.

Comment 8

[:Gijs (he/him)]

(In reply to Sebastian Padilla from comment #7)

Created attachment 9111970 [details]
lebug.jpg

Hi guys! This is still happening so I'm reopening this bug.

Please provide the information requested in comment #2. Also, please clarify if webrender is enabled on the device where you're seeing this, and what nightly version you're seeing this on.

Comment 9

[Sebastian Padilla]

Attachment: lebug2.jpg

Sure!

I've done what you've asked but it seems that the button info is not appearing on the console (adding a screenshot where you can see that I've selected the "pick an element" option and that the mouse is hovering over the i button). Maybe I'm doing something wrong?

There's no webrender enabled and the FF Nightly Version I'm using is 72.0a1 (2019-11-26) (64-bit)

Hope this helps!

Comment 10

[:Gijs (he/him)]

(In reply to Sebastian Padilla from comment #9)
Please use the Browser toolbox. There was a link with more details in comment #2...

Comment 11

[Sebastian Padilla]

Attachment: lebug3.jpg

Here we go!!

Comment 12

[:Gijs (he/him)]

(In reply to Sebastian Padilla from comment #11)

Created attachment 9111981 [details]
lebug3.jpg

Here we go!!

Thanks. This is very strange; the icon is in the same DOM position as https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/browser/base/content/browser.xhtml#941 and has the same ID but different classes.

Digging into this by looking for writes to "class" in PermissionUI.jsm and PopupNotifications.jsm, it looks like this is caused by the code in https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/toolkit/modules/PopupNotifications.jsm#1447-1471, which scrubs the class names out of the notification icon. This is pretty old code (from bug 1130356). Looking for callsites, we find https://searchfox.org/mozilla-central/rev/cce8b90aece0f42e5025e45282de16066eeaa662/toolkit/modules/PopupNotifications.jsm#619 . This code runs if we're showing a notification for an active browser in an inactive window.

This allows me to reproduce reliably with the following steps:

1. open nightly
2. type zdnet.com in the address bar (or click on new tab page) and immediately switch to another app
3. wait a good 10-20 seconds (and/or ensure you can see when the page has fully loaded) so the icon shows up
4. switch back to nightly

Ehsan, can someone who works with this code more often take over from here? I suspect we need to see if we even still need this code, and/or if/why other icons aren't affected (perhaps the others use #id selectors to pick an icon?) or what. I would be a bit surprised if this was a recent regression, but I suppose we could check for that, too.

Comment 13

[(no longer active)]

Thanks a lot, Gijs and Sebastian.

Nihanth, is this something you could look into please? Thanks!

Comment 14

[Florencia Di Ciocco, NI to vbalducci]

(In reply to :Gijs (he/him) from comment #12)

I would be a bit surprised if this was a recent regression, but I suppose we could check for that, too.

You're right, I've gone as far back as Nightly 64.0a1 (2018-09-11) and the bug is still present. Didn't proceed with the regression after that.

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Thanks for the digging folks, to me this is an edge case that doesn't need to be fixed with immediacy so I'm marking this P3.

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Assigned myself by mistake. I might pick this up in 73 but don't want to block anyone else from taking it before I get a chance.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

:fdiciocco, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 19

[Florencia Di Ciocco, NI to vbalducci]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #18)

:fdiciocco, since this bug is a regression, could you fill (if possible) the regressed_by field?

I've gone as far back as Nightly 64.0a1 (2018-09-11) and the bug is still present. Didn't proceed with the regression after that.

Comment 20

[Johann Hofmann [:johannh]]

Attachment: Bug 1598710 - Remove code that overwrites anchor class names in PopupNotifications.jsm. r=MattN

Looking at
https://searchfox.org/mozilla-central/search?q=notification-anchor-icon&case=false&regexp=false&path=
this code doesn't seem to be needed and it was a little weird for any anchor to rely on this
sort of implicit class rewriting. It certainly breaks any anchor that uses custom class names.

Note that in addition to this, there doesn't seem to be any need for supporting multiple
anchors for one notification anymore, so I'm going to open a separate bug for removing that.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98b4029eeaf3
Remove code that overwrites anchor class names in PopupNotifications.jsm. r=MattN

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/98b4029eeaf3

Comment 23

[Florencia Di Ciocco, NI to vbalducci]

Hi,
I've tested this bug on Nightly 73.0a1 (2019-12-13) (64-bit) and it's fixed. Just a note: the notification icon does not appear until I've returned the focus to FF.

Regards, Flor.

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 9114909 [details]
Bug 1598710 - Remove code that overwrites anchor class names in PopupNotifications.jsm. r=MattN

Beta/Release Uplift Approval Request

• User impact if declined: Sometimes showing the wrong icon for notification permission requests
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: This should be covered by the QA team for the Notification Permission Prompt project.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch removes some code that we deemed safe to remove because it was unused (but was overwriting an attribute we needed). It's been baking on Nightly for a while and it seems like we were right.
• String changes made/needed: None

Comment 25

[Florencia Di Ciocco, NI to vbalducci]

Attachment: beta.png

Hi Johann,
Has this been pushed to Beta? Because I see the old Icon.
Regards, Flor.

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

No it has not. If it had, "status-firefox72" would show up as "fixed".

Comment 27

[Julien Cristau [:jcristau] (back April 22)]

Is this worse in 72 vs previous releases? I'd be tempted to let this ride the trains otherwise...

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

As we only have one beta build left, I'd prefer to let this fix ride with 73.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9114909 [details]
Bug 1598710 - Remove code that overwrites anchor class names in PopupNotifications.jsm. r=MattN

While this is verified in nightly, it also looks like an edge case and we're very late in the beta cycle.

Comment 30

[Johann Hofmann [:johannh]]

Sorry, I was out for holidays and didn't set all my flags...

While I would have slightly preferred a late uplift for this, I'm fine with it either way. It's a cosmetic issue. Thanks!