API Review: Web Notification #3186
Conversation
Co-authored-by: David Risney <dave@deletethis.net>
API Spec: Web Notification
| void WebView_PermissionRequested(object sender, CoreWebView2PermissionRequestedEventArgs args) | ||
| { | ||
| CoreWebView2Deferral deferral = args.GetDeferral(); | ||
| System.Threading.SynchronizationContext.Current.Post((_) => |
There was a problem hiding this comment.
Doesn't this still run on the UI thread?
There was a problem hiding this comment.
Add a comment explaining why we are posting here - to avoid reentrancy from running message loop during MessageBox
| CoreWebView2Deferral deferral = args.GetDeferral(); | ||
| System.Threading.SynchronizationContext.Current.Post((_) => | ||
| { | ||
| using (deferral) |
There was a problem hiding this comment.
Also need to set Handled to true?
There was a problem hiding this comment.
Follow up - check if this is necessary. If necessary also add a comment explaining.
| ```cpp | ||
| /// Specifies the text direction of the notification. | ||
| [v1_enum] | ||
| typedef enum COREWEBVIEW2_TEXT_DIRECTION_KINDS { |
There was a problem hiding this comment.
This isn't a flags enum so should be singular (...KIND)
| /// 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( |
There was a problem hiding this comment.
What's the difference between a profile notification and a webview notification?
There was a problem hiding this comment.
Update comment to explain a bit more and link to DOM documentation.
| /// If `Handled` is set to TRUE then WebView will not display the notification | ||
| /// with the default UI, and the host will be responsible for handling the | ||
| /// notification and for letting the web content know that the notification | ||
| /// has been displayed, clicked, or closed. You should set `Handled` to `TRUE` |
There was a problem hiding this comment.
should or must?
We should make this requirement more evident in the code examples
|
|
||
| /// A collection of unsigned long integers. | ||
| [uuid(974A91F8-A309-4376-BC70-7537D686533B), object, pointer_default(unique)] | ||
| interface ICoreWebView2UnsignedLongCollection : IUnknown { |
There was a problem hiding this comment.
Why this rather than an array?
There was a problem hiding this comment.
This can be an array instead.
| /// The host may run this to report the notification has been displayed and it | ||
| /// will cause the [show](https://developer.mozilla.org/docs/Web/API/Notification/show_event) | ||
| /// event to be raised for non-persistent notifications. | ||
| /// You should only run this if you are handling the `NotificationReceived` |
There was a problem hiding this comment.
Similar to above, use 'must'
| /// [Notification.renotify](https://developer.mozilla.org/docs/Web/API/Notification/renotify) | ||
| /// DOM API. | ||
| /// The default value is `FALSE`. | ||
| [propget] HRESULT ShouldRenotify([out, retval] BOOL* value); |
There was a problem hiding this comment.
The app is supposed to watch for new notifications and see if it gets matches?
There was a problem hiding this comment.
Update ref doc to say 'See more information'. And consider rewording 'specifies'.
| ICoreWebView2* sender, | ||
| ICoreWebView2NotificationReceivedEventArgs* args) -> HRESULT | ||
| { | ||
| // Setting Handled to TRUE so the the default notification UI will not be |
There was a problem hiding this comment.
| // 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 |
| { | ||
| // ICoreWebView2Notification members | ||
| String Body { get; }; | ||
| CoreWebView2NotificationDirectionKinds Direction { get; }; |
There was a problem hiding this comment.
Was this meant to be the text-reading direction? (CoreWebView2TextDirectionKinds)
There was a problem hiding this comment.
Yes. Please ensure this is updated after rerunning tool to generate MIDL3.
| ```csharp | ||
| namespace Microsoft.Web.WebView2.Core | ||
| { | ||
| enum CoreWebView2TextDirectionKinds |
There was a problem hiding this comment.
"Default" means the webview has a reasonable (probably correct) direction to apply for most cases, right?
Windows.UI.Core CoreWindowFlowDirection and Windows.UI.Xaml FlowDirection don't have "default", just the rtl+ltr values, but I think including Default makes sense here since most users of the enum won't care to be specifying this.
There was a problem hiding this comment.
Make sure ref docs for this (above) correctly document this.
Also rename 'Default' to 'Auto'.
| Boolean RequireInteraction { get; }; | ||
| Boolean Silent { get; }; | ||
| Double Timestamp { get; }; | ||
| IVectorView<UInt64> Vibrate { get; }; |
There was a problem hiding this comment.
"Vibrate" sounds like bool(s). Looks like this was update to VibrationPattern in the C++ version above?
There was a problem hiding this comment.
Yes, thanks. Please also update similar to above.
|
|
||
| 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 |
There was a problem hiding this comment.
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.
|
|
||
| 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 |
There was a problem hiding this comment.
Does this mean that adding NotificationReceived event is a breaking change for some apps?
There was a problem hiding this comment.
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.
|
|
||
| // Handle toast activation for C# unpackaged app. | ||
| // Listen to notification activation | ||
| ToastNotificationManagerCompat.OnActivated += toastArgs => |
There was a problem hiding this comment.
nit, indentation seems busted in this section. (Extra indentation declaring the .OnActivated, and missing some in the ReportClosed scope
| /// is `FALSE` or `ReportShown` has not been run when this is called. Returns | ||
| /// `E_INVALIDARG` if an invalid action index is provided. Use `ReportClicked` | ||
| /// to activate an non-persistent notification. | ||
| HRESULT ReportClickedWithAction([in] UINT actionIndex, [in] ICoreWebView2NotificationReportClickedCompletedHandler* handler); |
There was a problem hiding this comment.
This is an arity-overload in the C# but different method name here. Intentional discrepancy?
There was a problem hiding this comment.
Yes due to COM IDL limitations.
| HRESULT ReportClicked([in] ICoreWebView2NotificationReportClickedCompletedHandler* handler); | ||
|
|
||
| /// The host may run this to report the persistent notification has been | ||
| /// activated with a given action, and it will cause the |
There was a problem hiding this comment.
There is no sample showing "with an action" vs ReportClicked. Does no-action mean customer clicked the notification body?
There was a problem hiding this comment.
Add another sample that does persistent notifications and uses ReportClickedWithAction
|
|
||
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Please update docs to note which runtime version the notification permission event started being raised.
| `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. |
There was a problem hiding this comment.
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.
| can also use such information to decide to show or not show a particular | ||
| notification. The host can `GetDeferral` or set the `Handled` property on the | ||
| `NotificationReceivedEventArgs` to handle the event at a later time or let | ||
| WebView2 know if the notification has been handled. By default, if the |
There was a problem hiding this comment.
I believe the Handled property applies only to permission requests from frames. (Handling the event prevents the event from propagating to the top-level document.) "Handling" the event from the top-level document has no effect.
This is sort of hinted at in the documentation but not explicitly stated.
The host may set this flag to TRUE to prevent the PermissionRequested event from firing on the CoreWebView2 as well.
So it says that this prevents the event from propagating to the root document. But it doesn't say that it has no effect if fired on the CoreWebView2 itself.
There was a problem hiding this comment.
Please update wording.
| CHECK_FAILURE(args->GetDeferral(&deferral)); | ||
|
|
||
| // Do the rest asynchronously, to avoid calling MessageBox in an event handler. | ||
| m_appWindow->RunAsync([this, deferral, args] { |
There was a problem hiding this comment.
Lifetime bug here. We need to extend the lifetime of the args.
| m_appWindow->RunAsync([this, deferral, args] { | |
| m_appWindow->RunAsync([this, deferral, args = wil::make_com_ptr(args)] { |
|
|
||
| ``` | ||
|
|
||
| ## Filter Notifications from a specific doamin and send local toast |
There was a problem hiding this comment.
| ## Filter Notifications from a specific doamin and send local toast | |
| ## Filter Notifications from a specific domain and send local toast |
| /// will cause the [show](https://developer.mozilla.org/docs/Web/API/Notification/show_event) | ||
| /// event to be raised for non-persistent notifications. | ||
| /// You should only run this if you are handling the `NotificationReceived` | ||
| /// event. Returns `E_ABORT` if `Handled` is `FALSE` when this is called. |
There was a problem hiding this comment.
What if an app does
args.Handled = true;
args.Notification.ReportShown();
args.Handled = false; // haha, I changed my mindIs the rule that once you call a Report method, you cannot un-handle the event?
| /// notification actions are specified. Note that actions are only applicable | ||
| /// for persistent notifications according to the web standard, and an empty | ||
| /// NotificationActionCollectionView will always be returned for | ||
| /// non-persistent notifications. |
There was a problem hiding this comment.
Whoa, this restriction is buried in the Notifications spec deep in section 2 (Notifications)
Actions are only currently supported for persistent notifications.
| /// | ||
| /// The caller must free the returned string with `CoTaskMemFree`. See | ||
| /// [API Conventions](/microsoft-edge/webview2/concepts/win32-api-conventions#strings). | ||
| [propget] HRESULT BodyImageUri([out, retval] LPWSTR* value); |
There was a problem hiding this comment.
Should at least mention Direction, Language, RequiresInteraction, IsSilent, and these other properties in the sample (say, in a comment of "Other properties you can use to style your replacement notification".)
There was a problem hiding this comment.
Please update to at least note in comment if not use in sample.
| [propget] HRESULT ShouldRenotify([out, retval] BOOL* value); | ||
|
|
||
| /// A boolean value indicating that a notification should remain active until | ||
| /// the user clicks or dismisses it, rather than closing automatically. |
There was a problem hiding this comment.
I don't think OS notifications support this. Is this a requirement of the DOM API, or can hosts fudge it?
There was a problem hiding this comment.
Update docs to note that you may not be able to necessarily implement this.
| /// This corresponds to the | ||
| /// [action](https://developer.mozilla.org/docs/Web/API/Notification/actions) | ||
| /// member of a notification action object. | ||
| [propget] HRESULT Action([out, retval] LPWSTR* value); |
There was a problem hiding this comment.
It's weird that Action has a property called Action, but that's what the DOM API calls it, so I'm inclined to match it.
It's not clear what the difference between Action and Title is.
|
|
||
| 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 |
|
|
||
| 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 |
There was a problem hiding this comment.
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.
| void WebView_PermissionRequested(object sender, CoreWebView2PermissionRequestedEventArgs args) | ||
| { | ||
| CoreWebView2Deferral deferral = args.GetDeferral(); | ||
| System.Threading.SynchronizationContext.Current.Post((_) => |
There was a problem hiding this comment.
Add a comment explaining why we are posting here - to avoid reentrancy from running message loop during MessageBox
| CoreWebView2Deferral deferral = args.GetDeferral(); | ||
| System.Threading.SynchronizationContext.Current.Post((_) => | ||
| { | ||
| using (deferral) |
There was a problem hiding this comment.
Follow up - check if this is necessary. If necessary also add a comment explaining.
| /// notification actions are specified. Note that actions are only applicable | ||
| /// for persistent notifications according to the web standard, and an empty | ||
| /// NotificationActionCollectionView will always be returned for | ||
| /// non-persistent notifications. |
| /// | ||
| /// The caller must free the returned string with `CoTaskMemFree`. See | ||
| /// [API Conventions](/microsoft-edge/webview2/concepts/win32-api-conventions#strings). | ||
| [propget] HRESULT BodyImageUri([out, retval] LPWSTR* value); |
There was a problem hiding this comment.
Please update to at least note in comment if not use in sample.
| [propget] HRESULT ShouldRenotify([out, retval] BOOL* value); | ||
|
|
||
| /// A boolean value indicating that a notification should remain active until | ||
| /// the user clicks or dismisses it, rather than closing automatically. |
There was a problem hiding this comment.
Update docs to note that you may not be able to necessarily implement this.
| String BadgeUri { get; }; | ||
| String ImageUri { get; }; | ||
| Boolean Renotify { get; }; | ||
| Boolean RequireInteraction { get; }; |
There was a problem hiding this comment.
| Boolean RequireInteraction { get; }; | |
| Boolean RequiresInteraction { get; }; |
| IVectorView<CoreWebView2NotificationAction> Actions { get; }; | ||
| String BadgeUri { get; }; | ||
| String ImageUri { get; }; | ||
| Boolean Renotify { get; }; |
There was a problem hiding this comment.
Make sure all the MIDL3 matches the COM IDL after running gen tool.
david-risney
added
the
review completed
No description provided.