1493864 - Notifications no longer emit sound

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andrew Valencia]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180920135444

Steps to reproduce:

Push notification to browser (permission allowed)


Actual results:

Notification popup is placed, but not sound played


Expected results:

I should hear a sound for the new notification (there's a long-standing bug for the
notification light to start blinking, but that's a different bug(

Comment 1

[Jan Henning [:JanH]]

Can I assume from that that your phone runs Oreo or later? The notifications only did that because we accidentally set the default priority for all notifications on Android O and later to "High", which for most notifications is too much. But true, there's already bug 1236902 for the same basic issue, namely that those notifications don't have a separate priority.

Comment 2

[Andrew Valencia]

If you'll look at bug 1257373 comment 18 from Jovan Gerodetti, notification with sound started working, and I certainly used it in pre-O Android.  Like I said, no LED, but yes sound with the notification.  But now with the Beta builds it's back to not making a sound?  I just re-verified, Firefox 61.0 on Android 7.1.2--phone off, notification comes in, and sound.  63.0b7 on 8.1.0, back to no sound--stealth notifications.  However, 62.0.2 on 8.1.0 does indeed do sound with the notification.

So I stand by my bug report--there's a regression, sound *was* and *is* working in 61 and 62, but has stopped working in the beta builds.  Please let me know if you need some test case JS code, and I can extract something from my messaging server source code.

Comment 3

[Jan Henning [:JanH]]

You're right, sorry, I didn't read the code closely enough. While we don't set a standard notification priority, sound was indeed added in bug 1378445.

As a workaround, you can go into Android's notification settings and change the settings for our default notification channel (currently the app name, might change to "Browser" in the future) to something audible.

Comment 4

[Jan Henning [:JanH]]

Flod, creating a new notification channel needs a new string, so what's your opinion regarding fixing this on Beta?

Comment 5

[Jan Henning [:JanH]]

In addition to the string changes from bug 1488874, we also need a separate notification channel for web notifications, since those used to be audible and therefore need a separate channel in order to use different default notification settings (also so users can configure the separately).

What should that channel be called: "Site notifications", "Web notifications", or ...?
Also what would sensible default settings for that channel be? Just sound, or sound + notification light, or even vibration? Not sure whether a heads-up notification that appears floating on top of the screen would be warranted by default, though.

Comment 6

[Francesco Lodolo [:flod]]

(In reply to Jan Henning [:JanH] from comment #4)
> Flod, creating a new notification channel needs a new string, so what's your
> opinion regarding fixing this on Beta?

Redirecting to Delphine, she's in charge of l10n for Fennec.

We're quickly approaching the deadline to ship any localization update in Beta (Oct 9), and IMO it's too late to introduce string changes, but it's Delphine's final call.

Comment 7

[Delphine Lebédel [:delphine - Use needinfo]]

Thanks for flagging. Are we just talking about one new string here? When do you expect we'd get it?
As Flod says the deadline for l10n is right around the corner, so we need to evaluate if this is a must-have or not because we may end up having this string displayed in English if the turn around if not enough. 
If this string is not critical and can wait another cycle, I'd vote on that (and as I understand it there's a workaround in the meantime to get sound to work). LMK

Comment 8

[Jan Henning [:JanH]]

Hopefully this week, but I'm not sure. If it's too late for string uplifts then I think this could just ride the trains.

Comment 9

[Delphine Lebédel [:delphine - Use needinfo]]

Then I would say let's just have this ride the trains, IMHO

Comment 10

[Pascal Chevrel:pascalc]

Wontfix for 63 as per comment #9

Comment 12

[Bram Pitoyo [:bram]]

(In reply to Jan Henning [:JanH] from comment #5)
> In addition to the string changes from bug 1488874, we also need a separate
> notification channel for web notifications, since those used to be audible
> and therefore need a separate channel in order to use different default
> notification settings (also so users can configure the separately).
> 
> What should that channel be called: "Site notifications", "Web
> notifications", or ...?

We can either call them “Site notifications” or “Notifications from sites”.


> Also what would sensible default settings for that channel be? Just sound,
> or sound + notification light, or even vibration? Not sure whether a
> heads-up notification that appears floating on top of the screen would be
> warranted by default, though.

When prompted with the dialogue “Do you want to receive notifications from twitter.com?”, most users who tap “Yes” expects to receive the same notification style that other apps have sent them, that they’re used to seeing. This means that we should use sound and visual, but not light. Better yet, we can default to whatever the system’s default value is.

Comment 13

[Jan Henning [:JanH]]

Attachment: Bug 1493864 - Assign site notifications to a dedicated notification channel. r?jchen

Bug 1378445 made those audible and we want to preserve the behaviour.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b57be7d7343c
Assign site notifications to a dedicated notification channel. r=jchen

Comment 15

[Cristian Brindusan [:cbrindusan]]

Backed out changeset b57be7d7343c (Bug 1493864) for build bustages on android-findbugs.

Backout: https://hg.mozilla.org/integration/autoland/rev/16aba5eca40ab337711f931f3b3140eba02c4c6f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending,running,success,testfailed,busted,exception&revision=b57be7d7343c78e4484b03d98c4a1a21e03ea941

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204914862&repo=autoland&lineNumber=4267

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b57be7d7343c

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1adf0333d63
Assign site notifications to a dedicated notification channel. r=jchen

Comment 18

[Jan Henning [:JanH]]

Forgot the "break;" in the switch statement, sorry.

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d1adf0333d63