881331 - Popup notifications added in a background window don't show the anchor icon until the window is brought to the front

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Jason Smith [:jsmith]]

Build: 6/10/2013 Nightly

STR

1. In window #1, go to http://mozilla.github.io/webrtc-landing/gum_test.html
2. Request video
3. Accept permissions
4. In window #2, go to http://mozilla.github.io/webrtc-landing/gum_test.html
5. Request video
6. Accept permissions

Expected

The visible indicator should be present on both windows.

Actual

The visible indicator disappears on the non-focused window. Additionally, Firefox begins to flash in the toolbar.

Comment 1

[Jason Smith [:jsmith]]

Definitely a recent regression. Easy to recover from though.

Comment 2

[Dão Gottwald [:dao]]

Not sure whether this a UI or core bug. A regression window would be useful.

Comment 3

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/993d7aff3109
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130228 Firefox/22.0 ID:20130228192921
Bad:
http://hg.mozilla.org/mozilla-central/rev/3362afba690e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130301 Firefox/22.0 ID:20130301154349
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=993d7aff3109&tochange=3362afba690e


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4e7646d14797
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130228 Firefox/22.0 ID:20130301003322
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/7df619463b43
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130228 Firefox/22.0 ID:20130301031827
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4e7646d14797&tochange=7df619463b43

regressed by:
da75867527dd	Dão Gottwald — Bug 843971 - Update browser-specific indicators when content gains or releases access to devices. r=dolske

Comment 4

[Dão Gottwald [:dao]]

So the underlying problem (that isn't a recent regression) is that popup notifications added in a background window don't show the icon until the window is brought to the front.

Comment 5

[Alex Keybl [:akeybl]]

Not blocking-gum and specific to gUM, so not tracking.

Comment 6

[Dão Gottwald [:dao]]

The "window starts flashing" part is bug 882188.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Hmm, I guess perhaps the activeWindow check in show() should be moved into _update, and only influence the popup-opening (and not the anchor-showing). That might throw off some tests that expect backgroundShow to fire in that case, though.

Comment 9

[Benjamin Smedberg]

Gavin, is comment 7 the approach we want? This would be nice to have for CtP plugins.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Yes, I think so.

Comment 11

[Matthew N. [:MattN]]

Sankha said he'll try get to it this week.

Comment 12

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

Comment 13

[Matthew N. [:MattN]]

(In reply to Sankha Narayan Guria [:sankha93] from comment #12)
> Created attachment 831987 [details] [diff] [review]
> patch v1

Hey Sankha, do the tests all pass without modification?

Comment 14

[Sankha Narayan Guria [:sankha]]

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #13)

> Hey Sankha, do the tests all pass without modification?

I just tested browser/base/content/test/general/browser_popupNotification.js, and all tests are passing as long as the browser window of the test is in focus. If the focus is taken to another window, the tests fail, is that normal behavior?

I could push to try to check it once more.

Comment 15

[Matthew N. [:MattN]]

(In reply to Sankha Narayan Guria [:sankha93] from comment #14)
> (In reply to Matthew N. [:MattN] (catching up on reviews) from comment #13)
> If the focus is taken to another window, the tests fail, is that normal
> behavior?

That's normal for some mochitests like this where panels are involved.
 
> I could push to try to check it once more.

That would be good since there are other tests which exercise PopupNotifications. I've pushed a run for you:
https://tbpl.mozilla.org/?tree=Try&rev=a125072533ae

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Patch looks good, but can you also write a test to cover this case?

If you find me or Matt on IRC we can give you some pointers.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 831987 [details] [diff] [review]
patch v1

Needs a test (per last comments).

Comment 18

[Matthew N. [:MattN]]

Sankha, any update on tests?

Comment 19

[Jason Smith [:jsmith]]

What do you mean specifically?

Comment 20

[Matthew N. [:MattN]]

Oops, sorry, wrong person. I meant to flag the assignee, not the reporter.

Sankha, any update on tests?

Comment 21

[Sankha Narayan Guria [:sankha]]

Yeah, I had some questions regarding the tests. I'll ping you on IRC for it.

Comment 22

[Matthew N. [:MattN]]

Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.

Comment 23

[Florian Quèze [:florian]]

Attachment: Updated patch with test

No recent activity here, so I updated the patch (it bitrotted mostly due to the refactoring in bug 1085691) and wrote a test.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33a89829ac0e

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8545282 [details] [diff] [review]
Updated patch with test

Review of attachment 8545282 [details] [diff] [review]:
-----------------------------------------------------------------

I get a failure while running the test directory (OS X Debug):
831 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/popupNotifications/browser_popupNotification_4.js | A promise chain failed to handle a rejection:  - at chrome://mochitests/content/browser/browser/base/content/test/popupNotifications/head.js:119 - TypeError: Assert is null

::: toolkit/modules/PopupNotifications.jsm
@@ -314,5 @@
>          if (!notification.dismissed) {
>            this.window.getAttention();
>          }
> -        if (notification.anchorElement.parentNode != this.iconBox) {
> -          this._updateAnchorIcon(notifications, notification.anchorElement);

Can you explain why the deletion of the `if` fixes this?

Comment 26

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (away until Jan. 7) from comment #25)
> I get a failure while running the test directory (OS X Debug):
> 831 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/popupNotifications/browser_popupNotification_4.js
> | A promise chain failed to handle a rejection:  - at
> chrome://mochitests/content/browser/browser/base/content/test/
> popupNotifications/head.js:119 - TypeError: Assert is null

Never mind this part since it happens without your patch too :(

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 8545282 [details] [diff] [review]
Updated patch with test

(In reply to Matthew N. [:MattN] from comment #25)

> ::: toolkit/modules/PopupNotifications.jsm
> @@ -314,5 @@
> >          if (!notification.dismissed) {
> >            this.window.getAttention();
> >          }
> > -        if (notification.anchorElement.parentNode != this.iconBox) {
> > -          this._updateAnchorIcon(notifications, notification.anchorElement);
> 
> Can you explain why the deletion of the `if` fixes this?

This test checks that the notification is not attached to the location bar. Another way to phrase this is: "if (SocialAPI) ...". This test was added by Shane in bug 1085691 when we mistakenly thought the issue was SocialAPI-only, and didn't want to change the non-SocialAPI behavior at all to reduce risk for a patch we wanted to uplift.

Comment 28

[Marco Mucci [:MarcoM]]

Hi Florian, can you provide a point value.

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8545282 [details] [diff] [review]
Updated patch with test

Review of attachment 8545282 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Florian Quèze [:florian] [:flo] from comment #27)
> Comment on attachment 8545282 [details] [diff] [review]
> Updated patch with test
> 
> (In reply to Matthew N. [:MattN] from comment #25)
> 
> > ::: toolkit/modules/PopupNotifications.jsm
> > @@ -314,5 @@
> > >          if (!notification.dismissed) {
> > >            this.window.getAttention();
> > >          }
> > > -        if (notification.anchorElement.parentNode != this.iconBox) {
> > > -          this._updateAnchorIcon(notifications, notification.anchorElement);
> > 
> > Can you explain why the deletion of the `if` fixes this?
> 
> This test checks that the notification is not attached to the location bar.
> Another way to phrase this is: "if (SocialAPI) ...". This test was added by
> Shane in bug 1085691 when we mistakenly thought the issue was
> SocialAPI-only, and didn't want to change the non-SocialAPI behavior at all
> to reduce risk for a patch we wanted to uplift.

Ah, ok, I'm not surprised that it didn't make sense to me as I wasn't thinking of SocialAPI

Comment 30

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/07be7d740021

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07be7d740021

Comment 32

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue on a nightly before the fix and verified on latest Nightly that the indicator is visible on all opened windows across all platforms, Windows 7 64bit, Mac OS X 10.9.5 and Ubuntu 14.04 32bit.