1007344 - Web Notifications can't create more than one at a time

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P3)

Description

[tyleregeto]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140421221237

Steps to reproduce:

Using the web notifications API, I create multiple notifications at one time in a for loop like as demo'd in to following sudo code:

for(i=0; i<num_notifications;i++) {
    new Notification("title...", {body:"body...", icon:"img..."})
}

No notifications appear if the value of num_notifications is greater than 1. Showing each in a timeout fixes the issue, like so:

for(i=0; i<num_notifications;i++) {
    setTimeout(function(){new Notification("title...", {body:"body...", icon:"img..."})}, 500);
}

Timeout needs to be a large enough value for the fix to work, a timeout of 10 doesn't help.


Actual results:

No notifications showed


Expected results:

All notifications are shown

Comment 1

[tyleregeto]

Sorry, the timeout in the above code should be 500 * i, so that the notifications are spread out by 500 ms.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jonas, do we throttle these somehow?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No idea. Maybe William knows since I think this is in his implementation.

Comment 6

[litianw]

Hi, [e-mail] 
No, the timeout in the code should be 500, not 500*i, because these notifications are supposed to be fired at the same time.I would expect them to be one stack on each other. But somehow none of them shown.

Comment 7

[litianw]

This is a serious bug, no one work on it?

Comment 8

[Andrew Overholt [:overholt]]

I think Edwin is corralling some UX work here.

Comment 9

[Edwin Wong [:edwong]]

I can repro this - windows only.  I doubt it's a regression so it's been around a long time.  I'm not sure what the use case is but that could factor in prioritization.  This seems like a platform issue, can ya'll investigate?

Comment 10

[William Chen [:wchen]]

I'm currently working on shadow DOM, I'll get a windows development machine and take a look at this afterwards if nobody else takes this in the meantime.

Comment 11

[XFox]

Happens with WebExtensions' implementation too. "If you call chrome.notifications.create() more than once in rapid succession, Firefox may end up not displaying any notification at all." - https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/notifications .

Comment 12

[janekptacijarabaci]

Dupe of bug 1263155.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Or rather that bug is a dupe of this one.  Or at least dependent on; nothing requires webextensions notifications to be subject to the same restrictions as DOM notifications.

Andrew, this does seem a bit odd; can we get someone to take a look?

Comment 14

[Andrew Overholt [:overholt]]

William, did you ever get a Windows development machine?

Comment 15

[perceptron8]

Hi! I'd like to let you know about another use case that this bug makes hard to implement.

We are developing some kind of panic-button within our web app. We have to use WebSockets, so users are notified instantaneously. Messages must be visible even if browser is minimized, that's why we use Notifications. However, users can and do open multiple tabs. To eliminate duplicates, we will rely on https://developer.mozilla.org/en-US/docs/Web/API/Notification/tag. This way, even if messages are delivered via web sockets to each tab separately and each tab constructs Notification on its own, we can be sure, that exactly one notification is going to be delivered.

Our solution works perfectly fine on Linux. Because of how it works on Windows, we are forced to add some ugly hacks, because not only nothing happens in case of rapid succession - all further notifications are being ignored. One have to reopen a tab to be notified again.

Comment 16

[perceptron8]

Attachment: Bug 1007344 - Ignore inactive windows when moving alerts.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0a22903ead2
Ignore inactive windows when moving alerts. r=MattN

Comment 18

[Razvan Maries]

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

Comment 19

[Matthew N. [:MattN]]

Thanks again for the patch! Does this also fix bug 1337490 (see my last comment)? Perhaps you're interested in bug 1263155 (also possibly fixed by your patch)?

We should consider uplifting this IMO since it totally breaks notifications and the fix is fairly straightforward. I will request that in a few days if there are no issues reported.

Comment 20

[perceptron8]

You're welcome!

I'm sure that bug 1337490 has been fixed by this patch - https://bugzilla.mozilla.org/show_bug.cgi?id=1337490#c34 contains code that is almost identical to the one we used to prove that we encountered a bug in Firefox.

I believe that this doesn't fix bug 1263155. https://bugzilla.mozilla.org/show_bug.cgi?id=1263155#c0 says that "All notification popups should be positioned on the visible screen, even if they are stacked on top of each other" and "None of the notification popups should be positioned off-screen". This is impossible to achieve in general case (how many alerts can you fit on the visible screen at once? 10? 100? 1000? how many can you read in 20 seconds?). In Chrome (without using native OS services) there are at most 3 notifications visible at once. The rest is queued. If bug 1263155 is in it's essence about making all notifications possible to read, then there's much more work to do - alert.js would need a complete rewrite. You said "we won't use this code much longer when we get native notifications finished on Windows" (https://phabricator.services.mozilla.com/D66605#2033000), so I'm not sure such rewrite would pay off. Obviously I'm not in a position to judge.

I knew about both bugs beforehand, that's how I found this one ;-) My goal was to come up with something straightforward and easy to review as soon as possible.

As you noticed, Notifications (at least on Windows) are totally broken. It's almost 6 years now. Uplifting would be great.

Comment 21

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

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

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 9132928 [details]
Bug 1007344 - Ignore inactive windows when moving alerts.

Beta/Release Uplift Approval Request

• User impact if declined: If a website tries to display more than on notification at a time (which would be common upon starting Firefox or in chat-like apps that use many notifications), instead no notification will be shown.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I think it's unlikely we made this feature worse since this bug has made it quite broken for many use cases. We are simply waiting for the notification window to be loaded before accessing the y coordinate.
• String changes made/needed: None

Comment 23

[Julien Cristau [:jcristau]]

Is this something we can add automated tests for?

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9132928 [details]
Bug 1007344 - Ignore inactive windows when moving alerts.

fix an issue with alerts/notifications, approved for 75.0b6

Comment 25

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc4c75fc6a3e

Comment 26

[Matthew N. [:MattN]]

(In reply to Julien Cristau [:jcristau] from comment #23)

Is this something we can add automated tests for?

Yes, but I didn't want to burden the contributor with that since IMO we should ship native notifications on Window ASAP (bug 1497425). Also nobody is actively working in this area so any time we spend on this is taking away from projects that management has prioritized. It's sad but true.

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 9132928 [details]
Bug 1007344 - Ignore inactive windows when moving alerts.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bad bug (notifications appear fully off-screen) that makes notifications appear totally off-screen on Windows and some Linux setups (wherever the XUL notifications are used). This also affects Thunderbird mail notifications.
• User impact if declined: If a website tries to display more than on notification at a time (which would be common upon starting Firefox or in chat-like apps that use many notifications), instead no notification will be shown.
• Fix Landed on Version: 75.0
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Shipped in 75 without fallout so far and without this fix the feature is quite broken so it's unlikely we would make the feature worse with this fix.
• String or UUID changes made by this patch: None

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9132928 [details]
Bug 1007344 - Ignore inactive windows when moving alerts.

Fixes a pretty ugly papercut bug. Pretty simple fix with no known regressions since it shipped in Fx75. Approved for 68.8esr.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/4fe4bbbd5969