Closed Bug 1007344 Opened 8 years ago Closed 2 years ago

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

Categories

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

29 Branch
x86_64
Windows 8.1
defect

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
thunderbird_esr60 --- wontfix
thunderbird_esr68 --- affected
firefox-esr68 --- fixed
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- fixed
firefox76 --- fixed

People

(Reporter: tyleregeto, Assigned: perceptron8)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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
Sorry, the timeout in the above code should be 500 * i, so that the notifications are spread out by 500 ms.
Component: Untriaged → DOM
Product: Firefox → Core
Jonas, do we throttle these somehow?
Flags: needinfo?(jonas)
No idea. Maybe William knows since I think this is in his implementation.
Flags: needinfo?(jonas) → needinfo?(wchen)
Duplicate of this bug: 1083558
Duplicate of this bug: 1124096
Hi, tyleregeto@gmail.com 
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.
This is a serious bug, no one work on it?
I think Edwin is corralling some UX work here.
Flags: needinfo?(wchen) → needinfo?(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?
Flags: needinfo?(edwong) → needinfo?(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.
Flags: needinfo?(wchen)
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 .
Dupe of bug 1263155.
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?
Blocks: 1263155
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(overholt)
William, did you ever get a Windows development machine?
Flags: needinfo?(overholt) → needinfo?(wchen)
Priority: -- → P3
Component: DOM → DOM: Core & HTML

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.

Assignee: nobody → perceptron8
Status: NEW → ASSIGNED
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a0a22903ead2
Ignore inactive windows when moving alerts. r=MattN
Flags: needinfo?(william)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76

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.

Flags: needinfo?(MattN+bmo)
Depends on: 1338831

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.

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

Keywords: regression
Component: DOM: Core & HTML → Notifications and Alerts
Product: Core → Toolkit

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
Flags: needinfo?(MattN+bmo)
Attachment #9132928 - Flags: approval-mozilla-beta?

Is this something we can add automated tests for?

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

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

Attachment #9132928 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(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 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
Attachment #9132928 - Flags: approval-mozilla-esr68?

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.

Attachment #9132928 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.