Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Review: Web Notification #3186

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix sample code
  • Loading branch information
peiche-jessica committed Jan 10, 2023
commit 4908aaf631af2392e48f6fe59ad7e18a8c851422
74 changes: 43 additions & 31 deletions specs/WebNotification.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,11 @@ Runtime.
## Handle PermissionRequested event

This can be achieved with the existing `PermissionRequested` events.
`PermissionRequested` event used to not be raised for `PermissionKind.Notification`.
Now, `PermissionRequested` events are raised for `PermissionKind.Notification`, and `PermissionState` needs to be set `Allow` explicitly to allow such permission requests. `PermissionState.Default` for `PermissionKind.Notification` is considered denied.
`PermissionRequested` event used to not be raised for
`PermissionKind.Notification`. Now, `PermissionRequested` events are raised for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curiously, the documentation suggests that it is indeed raised:

CoreWebView2PermissionKind.Notifications (4)

Indicates permission to send web notifications. Apps that would like to show notifications should handle PermissionRequested and/or PermissionRequested events and no browser permission prompt will be shown for notification requests. Note that push notifications are currently unavailable in WebView2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update docs to note which runtime version the notification permission event started being raised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that adding NotificationReceived event is a breaking change for some apps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes technically, but it falls into the same category as adding a new enum value to an existing enum and we're not worried about that case - documenting that for the PermissionRequested event we'll be adding new enum values in the future and to have an appropriate value in the switch default.

`PermissionKind.Notification`, and `PermissionState` needs to be set `Allow`
explicitly to allow such permission requests. `PermissionState.Default` for
`PermissionKind.Notification` is considered denied.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this affect apps that predate the new behavior? They presumably ignore the PermissionKind.Notification request. I assume the system default behavior is to display a prompt and let the user decide whether to allow; i.e., same as before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.


### C# Sample
```csharp
Expand Down Expand Up @@ -66,9 +69,11 @@ void WebView_PermissionRequested(object sender, CoreWebView2PermissionRequestedE
{
case MessageBoxResult.Yes:
args.State = CoreWebView2PermissionState.Allow;
_cachedPermissions[cachedKey] = true;
break;
case MessageBoxResult.No:
args.State = CoreWebView2PermissionState.Deny;
_cachedPermissions[cachedKey] = false;
break;
case MessageBoxResult.Cancel:
args.State = CoreWebView2PermissionState.Default;
Expand Down Expand Up @@ -169,11 +174,15 @@ using Microsoft.Toolkit.Uwp.Notifications;
void WebView_NotificationReceived(object sender, CoreWebView2NotificationReceivedEventArgs args)
{
CoreWebView2Deferral deferral = args.GetDeferral();
args.Handled = true;
var notification = args.Notification;

// Show notification with local MessageBox
string message = "Received notification from " + args.Uri + " with body " + notification.Body;
MessageBox.Show(message);
System.Threading.SynchronizationContext.Current.Post((_) =>
{
string message = "Received notification from " + args.Uri + " with body " + notification.Body;
MessageBox.Show(message);
}

// Requires Microsoft.Toolkit.Uwp.Notifications NuGet package version 7.0 or greater
var toastContent = new ToastContentBuilder()
Expand All @@ -183,15 +192,25 @@ void WebView_NotificationReceived(object sender, CoreWebView2NotificationReceive
toastContent.Show();
// See
// https://learn.microsoft.com/windows/apps/design/shell/tiles-and-notifications/send-local-toast?tabs=uwp
// for how to handle toast activation
// for how to handle toast activation.

// Call ReportShown after showing the toast notification to raise
// the DOM notification.show event.
// args.Handled has been set to true before we call Report... methods.
notification.ReportShown();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? The notification may not be shown immediately, and it might never be shown at all. I don't know what the DOM requirements are for the notification.show event.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please investigate.

peiche-jessica marked this conversation as resolved.
Show resolved Hide resolved

// During the toast notification activation handling, we may call
// ReportClicked and/or ReportClose accordingly.
notification.ReportClosed();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we validate that the ToastArguments correspond to the notification we are handling? Otherwise, if the app intercepts two notifications and shows two toasts, the app will take the user's answer to the first toast and report it as the response to both notifications.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment about m_id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Named ReportClosedAsync/ReportClickedAsync in the IDL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer async after changes above. Please ensure correct after all other changes are complete =)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it an error to Report redundant or conflicting actions?

  • Calling ReportShown / ReportClicked / ReportClosed twice
  • Calling both ReportClicked and ReportClosed
  • Calling ReportClicked(n) and ReportClicked(m) with n ≠ m.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update docs and implementation to match what Raymond says above. Should match the error case for not setting Handled and then calling a Report method.

args.Handled = true;
}
```

### C++ Sample

`Microsoft.Toolkit.Uwp.Notifications ` package does not work with non-UWP Win32
app. Hence in the sample code we use native MessageBox and handle notifications
accordingly.

```cpp
...
{
Expand All @@ -203,16 +222,21 @@ void WebView_NotificationReceived(object sender, CoreWebView2NotificationReceive
ICoreWebView2* sender,
ICoreWebView2NotificationReceivedEventArgs* args) -> HRESULT
{
// Block notifications from specific URIs and set Handled to
// false so the the default notification UI will not be
// shown by WebView2 either.
wil::unique_cotaskmem_string uri;
CHECK_FAILURE(args->get_Uri(&uri);
if (ShouldBlockUri(uri.get()))
// Setting Handled to TRUE so the the default notification UI will not be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Setting Handled to TRUE so the the default notification UI will not be
// Setting Handled to TRUE so the default notification UI will not be

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix wording.

// shown by WebView2 as we are handling the notification ourselves.
// Block notifications from specific origins and return directly without showing notifications.
CHECK_FAILURE(args->put_Handled(TRUE));
wil::unique_cotaskmem_string origin;
CHECK_FAILURE(args->get_SenderOrigin(&origin));
if (ShouldBlockOrigin(origin.get()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature does not exist in the C# sample. The C# and C++ samples should show equivalent functionality. If the C++ sample has a feature that is not in the C# sample, then C# developers will assume that the feature is not available in C#.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update to ensure samples match.

{
CHECK_FAILURE(args->put_Handled(FALSE));
return S_OK;
}
ShowNotification(args);

wil::com_ptr<ICoreWebView2Notification> notification;
CHECK_FAILURE(args->get_Notification(&notification));

ShowNotification(notification);
return S_OK;
})
.Get(),
Expand All @@ -222,40 +246,28 @@ void WebView_NotificationReceived(object sender, CoreWebView2NotificationReceive
...
//! [OnNotificationReceived]
void SettingsComponent::ShowNotification(
ICoreWebView2NotificationReceivedEventArgs* args)
ICoreWebView2Notification* notification)
{
AppWindow* appWindow = m_appWindow;

// Obtain a deferral for the event so that the CoreWebView2
// does not examine the properties we set on the event args and
// after we call the Complete method asynchronously later.
wil::com_ptr<ICoreWebView2Deferral> deferral;
CHECK_FAILURE(args->GetDeferral(&deferral));

wil::com_ptr<ICoreWebView2NotificationReceivedEventArgs> eventArgs = args;

appWindow->RunAsync(
[this, eventArgs, deferral]
[this, notification]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use-after-free bug. Need to extend lifetime of the notification.

Suggested change
[this, notification]
[this, wil::make_com_ptr(notification)]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix.

{
wil::com_ptr<ICoreWebView2Notification> notification;
CHECK_FAILURE(eventArgs->get_Notification(&notification));

wil::unique_cotaskmem_string uri;
CHECK_FAILURE(eventArgs->get_Uri(&uri));
wil::unique_cotaskmem_string origin;
CHECK_FAILURE(eventArgs->get_SenderOrigin(&origin));
wil::unique_cotaskmem_string title;
CHECK_FAILURE(notification->get_Title(&title));
wil::unique_cotaskmem_string body;
CHECK_FAILURE(notification->get_Body(&body));

std::wstring message =
L"The page at " + std::wstring(uri.get()) + L" sends you an
L"The page from " + std::wstring(origin.get()) + L" sends you an
notification:\n\n";
message += body.get();
notification->ReportShown();
int response = MessageBox(nullptr, message.c_str(), title.get(), MB_OKCANCEL);
(response == IDOK) ? notification->ReportClicked() : notification->ReportClosed();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completion callback parameters missing from ReportClosed and ReportClicked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure correct after all other changes are done.


deferral->Complete();
});
}
//! [OnNotificationReceived]
Expand Down