Desktop Notification open a new tab.
Reported by
olm...@gmail.com,
Oct 24
|
||||||||
Issue description
UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/69.0.3497.81 Chrome/69.0.3497.81 Safari/537.36
Steps to reproduce the problem:
1. Send first notification
2. Send second notification
3. Click on notification
What is the expected behavior?
The click on notification should return focus opened tab
What went wrong?
Click notification -> open a new tab
Did this work before? N/A
Chrome version: 69.0.3497.81 Channel: n/a
OS Version: Ubuntu 16.04 LTS
Flash Version:
This behavior is also in Chrome for Windows and Mac.
This behavior is only when using the parameter 'tag' in options:
new Notification("Test " + i, {
body: 'Notification ' + i,
tag: 'soManyNotification'
});
,
Oct 25
,
Oct 26
Hello I can confirm this bug. It is in Chrome for several weeks (I think more than one month and less than 2 months). It was also in v69 and latest v70. Before in about two months there was not that bug in Chrome. This bug probably impacts also Gmail: https://productforums.google.com/forum/#!topic/gmail/7rMrLrNuOIk It is very easy to reproduce this bug with one line of code: 1.) Go to any website which uses HTTPS (you can use current website) 2.) Open console (F12) 3.) Request notification and approve permission by entering this code to console: Notification.requestPermission() 4.) Run notification by entering this code to console: var notification = new Notification('My title', {requireInteraction: true, tag: "message", body: "My body"}); notification.onclick = function() { console.log("CLICK"); this.close(); }; When you run code from 4) only once it works well after you click at notification - "CLICK" is correctly logged to console. But when you run code two or more times and after click at notification new tab is opened with domain of the origin website (e.g. https://bugs.chromium.org/) and notification click event is never fired. When you test this in another browser e.g. Firefox it works as expected.
,
Oct 26
Confirm this issue Change logs provided by chrome bisect tool: https://chromium.googlesource.com/chromium/src/+log/5eedcc63cf1752ea5f475fb097df00c25b83af19..b50036163e2ca5222c4fefd1599539777933ae40 The suspect commit: https://chromium.googlesource.com/chromium/src/+/60036df9cee4317b4c271b3b3e43c536d45ba1dd
,
Oct 27
I've found the root cause of this issue and working on a fix for it
,
Oct 28
Waiting for review: https://chromium-review.googlesource.com/c/chromium/src/+/1303898
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e4c3c10ede11e9ce60c0be6b2b5614831d5a072 commit 4e4c3c10ede11e9ce60c0be6b2b5614831d5a072 Author: Sunny <ratsunny@gmail.com> Date: Wed Oct 31 10:51:52 2018 Postpone listener replacement after non-persistent notification closed When calling |RegisterNonPersistentNotificationListener| with an existed notification id, it will first dispatch a close event to old notification and set new listener into listener map. Currently the new listener is replaced immediately after calling |DispatchNonPersistentCloseEvent|, since it's an asynchronize call, it will remove the replaced listener unexpectedly, instead of removing the original listener. To fix this, we can postpone listener replacement after the close event finish it's cleaning job. For new created notification, the listener can be set immediately due to there is no close event need to be dispatched. Bug: 880266 , 898486 Change-Id: I370d77386cf1a6636f368f1785d92d362a3b4263 Reviewed-on: https://chromium-review.googlesource.com/c/1303898 Reviewed-by: Mike West <mkwst@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#604203} [modify] https://crrev.com/4e4c3c10ede11e9ce60c0be6b2b5614831d5a072/content/browser/notifications/notification_event_dispatcher_impl.cc [modify] https://crrev.com/4e4c3c10ede11e9ce60c0be6b2b5614831d5a072/content/browser/notifications/notification_event_dispatcher_impl.h [modify] https://crrev.com/4e4c3c10ede11e9ce60c0be6b2b5614831d5a072/content/browser/notifications/notification_event_dispatcher_impl_unittest.cc
,
Oct 31
I propose backport this pstch to dev/beta/release branch
,
Nov 15
***UI Mass Triage*** Since work is in progress, adding appropriate labels.
,
Nov 30
Assigning this to mkwst who approved the original submit, please look at merging this down to 71.
,
Nov 30
This bug requires manual review: We are only 3 days from stable. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 30
,
Nov 30
I'll take this given that Mike's OOO. jayhlee@, since it's really late in the M71 process could you add a brief justification of priority for the TPMs? Merging SGTM from an engineering PoV, Sunny's CL is isolated and safe.
,
Nov 30
Thanks Peter, justification is a major enterprise customer is now reporting the issue in case #17630888.
,
Nov 30
Is the change verified on M72?
,
Nov 30
Rejecting merge to M71 as this is regressed since M68 and per offline chat with peter@ and jayhlee@ . |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by swarnasree.mukkala@chromium.org
, Oct 25