New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 898486 link

Starred by 6 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

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'
});
 
testWrong.mp4
452 KB View Download
testNotification.html
1.9 KB View Download
Labels: Needs-Triage-M69
Components: -Blink UI>Notifications
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.

I've found the root cause of this issue and working on a fix for it
Project Member

Comment 7 by bugdroid1@chromium.org, 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

I propose backport this pstch to dev/beta/release branch
Labels: Hotlist-DesktopUIValid Hotlist-DesktopUIChecked
***UI Mass Triage***

Since work is in progress, adding appropriate labels.
Cc: jayhlee@chromium.org
Labels: Hotlist-Enterprise Merge-Request-71 OS-Mac OS-Windows
Owner: mkwst@chromium.org
Status: Started (was: Unconfirmed)
Assigning this to mkwst who approved the original submit, please look at merging this down to 71.
Project Member

Comment 11 by sheriffbot@chromium.org, Nov 30

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
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
Labels: M-71
Owner: peter@chromium.org
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.

Thanks Peter, justification is a major enterprise customer is now reporting the issue in case #17630888.
Is the change verified on M72?
Labels: -Merge-Review-71 Merge-Rejected-71
Rejecting merge to M71 as this is regressed since M68 and per offline chat with peter@ and jayhlee@ .

Sign in to add a comment