New issue
Advanced search Search tips

Issue 781792 link

Starred by 5 users
Status: Fixed
Owner:
finnur@chromium.org
Closed: Sep 12
Cc:
chengx@chromium.org
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 734095



Sign in to add a comment

Windows Native Notifications require 1+ button for Reminders to work

Project Member Reported by finnur@chromium.org, Nov 6 2017 
A notification that has requireInteraction=true will silently be converted to requireInteraction=false if no actions are specified. This breaks a few use-cases for Notifications.

In other words, scenario="reminder" is not added to the <toast> tag unless <actions> contains buttons.

Comment 1 by finnur@chromium.org, Nov 6 2017

Cc: -bever...@google.com peter@chromium.org

Comment 2 by dtapu...@chromium.org, Nov 8 2017

Components: UI>Notifications

Comment 3 by awdf@chromium.org, Jan 8 2018

Labels: -Pri-1 Pri-2
Owner: finnur@chromium.org
Status: Assigned (was: Untriaged)
Lowering priority because this only affects Windows native notifications which aren't yet shipped and assigning to Finnur because he is working on Windows native notifications.

Comment 4 by peter@chromium.org, Sep 3

Labels: -Pri-2 Pri-1

Comment 5 by phistuck@chromium.org, Sep 3 

 Issue 879144  has been merged into this issue.

Comment 6 by chengx@chromium.org, Sep 3

Cc: chengx@chromium.org

Comment 10 by finnur@chromium.org, Sep 10 

This fix is just a work-around. There's an investigation going into whether there is a better fix available. If not, we'll close this.

Comment 11 by finnur@chromium.org, Sep 11

Labels: Merge-Request-70
We've not been able to identify a better work-around, so we need to merge this. It is important to fix this because this affects a lot of notifications, such as calendar reminders, causing people to miss them (thereby missing their meetings).

Comment 12 by abdulsyed@chromium.org, Sep 11 

How safe is this merge overall? Which CL specifically needs to be merged?

Comment 13 by finnur@chromium.org, Sep 12 

I think it should be quite safe. We're just adding a Close button to a (subset) of notifications. The CL in question is:

commit e2ad594c97b67e29fa190590dccfc5a4bfe54a89
Reviewed-on: https://chromium-review.googlesource.com/1202663
Project Member

Comment 14 by sheriffbot@chromium.org, Sep 12

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 12

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5

commit 8901a4d2d151022dfd5e58690ea9fb5b6421f2c5
Author: Finnur Thorarinsson <finnur@chromium.org>
Date: Wed Sep 12 13:10:35 2018

Windows Native Notifications: Add a Close button for reminders if no buttons are present.

Bug:  781792 
Change-Id: Ieb5c92cf3c926e508576cb812b16ca6dda12dae9
Reviewed-on: https://chromium-review.googlesource.com/1202663
Commit-Queue: Finnur Thorarinsson <finnur@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588530}(cherry picked from commit e2ad594c97b67e29fa190590dccfc5a4bfe54a89)
Reviewed-on: https://chromium-review.googlesource.com/1221265
Cr-Commit-Position: refs/branch-heads/3538@{#324}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5/chrome/browser/notifications/notification_platform_bridge_win.cc
[modify] https://crrev.com/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5/chrome/browser/notifications/win/notification_launch_id.cc
[modify] https://crrev.com/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5/chrome/browser/notifications/win/notification_launch_id.h
[modify] https://crrev.com/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5/chrome/browser/notifications/win/notification_launch_id_unittest.cc
[modify] https://crrev.com/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5/chrome/browser/notifications/win/notification_template_builder.cc
[modify] https://crrev.com/8901a4d2d151022dfd5e58690ea9fb5b6421f2c5/chrome/browser/notifications/win/notification_template_builder.h

Comment 16 by finnur@chromium.org, Sep 12

Status: Fixed (was: Assigned)
►

Sign in to add a comment