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
Address feedback
peiche-jessica committed Feb 1, 2023
commit 320baf0418624c33d657db424509d68f8796bb11
69 changes: 50 additions & 19 deletions specs/WebNotification.md
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ service worker registration.
You should be able to handle notification permission requests, and
further listen to `NotificationReceived` events to optionally handle the
notifications themselves. The `NotificationReceived` events are raised on
Copy link
Contributor

Choose a reason for hiding this comment

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

Wordsmithing. This is correct but consider "The NotificationReceived events are raised on CorebWebView2 for non-persistent notifications and on CoreWebView2Profile object for persistent notifications." for clearer separation.

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.

`CorebWebView2` and `CoreWebView2Profile` object respectively for non-persistent
`CorebWebView2` and `CoreWebView2Profile` object for non-persistent
and persistent notifications respectively.

The `NotificationReceived` event on `CoreWebView2` and `CoreWebView2Profile` let
@@ -96,6 +96,9 @@ CHECK_FAILURE(m_webView->add_PermissionRequested(
&m_permissionRequestedToken));
}
...
std::map<std::tuple<std::wstring, COREWEBVIEW2_PERMISSION_KIND, BOOL>, bool>
m_cachedPermissions;

HRESULT SettingsComponent::OnPermissionRequested(
ICoreWebView2* sender, ICoreWebView2PermissionRequestedEventArgs* args)
{
@@ -116,12 +119,12 @@ HRESULT SettingsComponent::OnPermissionRequested(

COREWEBVIEW2_PERMISSION_STATE state;

auto cached_key = std::make_tuple(std::wstring(uri.get()), kind, userInitiated);
auto cached_permission = m_cached_permissions.find(cached_key);
if (cached_permission != m_cached_permissions.end())
auto cachedKey = std::make_tuple(std::wstring(uri.get()), kind, userInitiated);
auto cachedPermission = m_cachedPermissions.find(cachedKey);
if (cachedPermission != m_cachedPermissions.end())
{
state =
(cached_permission->second ? COREWEBVIEW2_PERMISSION_STATE_ALLOW
(cachedPermission->second ? COREWEBVIEW2_PERMISSION_STATE_ALLOW
: COREWEBVIEW2_PERMISSION_STATE_DENY);
}
else
@@ -142,11 +145,11 @@ HRESULT SettingsComponent::OnPermissionRequested(
switch (response)
{
case IDYES:
m_cached_permissions[cached_key] = true;
m_cachedPermissions[cachedKey] = true;
state = COREWEBVIEW2_PERMISSION_STATE_ALLOW;
break;
case IDNO:
m_cached_permissions[cached_key] = false;
m_cachedPermissions[cachedKey] = false;
state = COREWEBVIEW2_PERMISSION_STATE_DENY;
break;
default:
@@ -165,15 +168,28 @@ HRESULT SettingsComponent::OnPermissionRequested(

## Filter Notifications from a specific doamin and send local toast
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
## Filter Notifications from a specific doamin and send local toast
## Filter Notifications from a specific domain and send local toast

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 typo.


Learn more about sending a local toast [Send a local toast notification from a C# app - Windows apps | Microsoft Learn](https://learn.microsoft.com/windows/apps/design/shell/tiles-and-notifications/send-local-toast?tabs=uwp).
This sample uses custom app UI to display toast notifications. It does so using
`Microsoft.Toolkit.Uwp.Notifications` nuget package. For an unpackaged C# WinApp
SDK based app, you can handle the toast notification activation in your app's
startup code such as in `App.xaml.cs`, and then dispatch it to the WebView2
notification object. Note that the notification object will be gone along with
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing terminology here, because "activation" in the WinAppSDK sense is really talking about app activation to tell you that the user completed their Toast interaction. It's not activating an object in the WebView2.

Maybe "An unpackaged C# WinApp SDK based app learns of changes to the toast state from the ToastNotificationManagerCompat.OnActivated event and can forward those notifications to the 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 to avoid ambiguous use of 'activation' here.

WebView2. If your app is closed when the toast notification is activated, the
Comment on lines +168 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing phrasing "will be gone along with the WebView2". It sounds like a threat that the system will spontaneously destroy your notification and the WebView2 at some unspecified future point. I think what this is really saying is "The Notification is automatically (destroyed | removed | something) when its associated WebView2 is (destroyed | closed | something)."

(I wasn't sure what "gone" means here, so I offered some possible answers that might be wrong.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rephrase to be more precise and accurate.

notification object should be gone and you should not try to dispatch the
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a more precise term than "gone". Does it mean "Destroyed (and any attempt to access it - even just to call Release - will crash)", or "Permanently hidden" or "Disconnected (all method calls except for AddRef and Release fail with RPC_E_DISCONNECTED)" or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rephrase to be more precise and accurate.

activation.

For UWP and desktop packaged apps, you can learn more about via [Send a local
toast notification from a C# app - Windows apps | Microsoft
Learn](https://learn.microsoft.com/windows/apps/design/shell/tiles-and-notifications/send-local-toast?tabs=uwp).

### C# Sample

```csharp
using Microsoft.Toolkit.Uwp.Notifications;
...
void WebView_NotificationReceived(object sender, CoreWebView2NotificationReceivedEventArgs args)
{
CoreWebView2Deferral deferral = args.GetDeferral();
using (args.GetDeferral())
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add comment that a deferral is not required by the sample, but we obtain one to demonstrate how to obtain and consume a deferral properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment out the GetDeferral code with a comment explaining why you would want to use it.

{
args.Handled = true;
var notification = args.Notification;

@@ -190,24 +206,37 @@ void WebView_NotificationReceived(object sender, CoreWebView2NotificationReceive
.AddText("Notification sent from " + args.Uri +":\n")
.AddText(notification.Body);
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.

// 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.


// Handle toast activation for C# unpackaged app.
// Listen to notification activation
ToastNotificationManagerCompat.OnActivated += toastArgs =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Race condition: Should subscribe to this event before showing the notification, in case the user responds to the toast before we can subscribe to the event. (Can happen if the toast is being automated, say, because your connected FitBit is auto-responding to all toasts because you're napping.)

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 fixed when moving subscription to app startup from previous comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit, indentation seems busted in this section. (Extra indentation declaring the .OnActivated, and missing some in the ReportClosed scope

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.

{
// Obtain the arguments from the notification
ToastArguments args = ToastArguments.Parse(toastArgs.Argument);

// Obtain any user input (text boxes, menu selections) from the notification
ValueSet userInput = toastArgs.UserInput;

// Need to dispatch to UI thread if performing UI operations
Application.Current.Dispatcher.Invoke(delegate
{
// 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.

});
};
}
}
```

### C++ Sample

`Microsoft.Toolkit.Uwp.Notifications ` package does not work with non-UWP Win32
`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.

@@ -296,9 +325,10 @@ interface ICoreWebView2Profile3 : IUnknown {
/// Add an event handler for the `NotificationReceived` event for persistent
/// notifications.
///
/// If a deferral is not taken on the event args, the subsequent scripts are
/// blocked until the event handler returns. If a deferral is taken, the
/// scripts are blocked until the deferral is completed.
/// If a deferral is not taken on the event args, the subsequent scripts after
/// the DOM notification creation call (i.e. `Notification()`) are blocked
/// until the event handler returns. If a deferral is taken, the scripts are
/// blocked until the deferral is completed.
HRESULT add_NotificationReceived(
Copy link
Contributor

Choose a reason for hiding this comment

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

No sample for this or discussion about why no sample was needed (e.g. because it's identical to the CoreWebView2 version aside from where you register the handler).

Would an app want to treat persistent and non-persistent notifications differently?

Or is the reason that you subscribe to CoreWebView2.NotificationReceived to handle notifications raised by that specific WebView2, and you use CoreWebView2Profile.NotificationReceived for notifications not associated with any WebView2?

Do persistent notifications outlive the WebView2? (Why are they called "persistent"?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please add a third sample for this as we described elsewhere.

Please ensure we document the scope of persistent and non-persistent with respect to WebView2 (environment, profile, browser process, webview2)

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between a profile notification and a webview notification?

Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment to explain a bit more and link to DOM documentation.

[in] ICoreWebView2ProfileNotificationReceivedEventHandler* eventHandler,
[out] EventRegistrationToken* token);
@@ -314,9 +344,10 @@ interface ICoreWebView2_17 : ICoreWebView2_16 {
/// Add an event handler for the `NotificationReceived` event for
/// non-persistent notifications.
///
/// If a deferral is not taken on the event args, the subsequent scripts are
/// blocked until the event handler returns. If a deferral is taken, the
/// scripts are blocked until the deferral is completed.
/// If a deferral is not taken on the event args, the subsequent scripts after
/// the DOM notification creation call (i.e. `Notification()`) are blocked
/// until the event handler returns. If a deferral is taken, the scripts are
/// blocked until the deferral is completed.
HRESULT add_NotificationReceived(
[in] ICoreWebView2NotificationReceivedEventHandler* eventHandler,
[out] EventRegistrationToken* token);