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

API Review: Web Notification #3186

wants to merge 18 commits into from

Conversation

peiche-jessica
Copy link
Collaborator

No description provided.

@peiche-jessica peiche-jessica added the API Proposal Review WebView2 API Proposal for review. label Feb 3, 2023
void WebView_PermissionRequested(object sender, CoreWebView2PermissionRequestedEventArgs args)
{
CoreWebView2Deferral deferral = args.GetDeferral();
System.Threading.SynchronizationContext.Current.Post((_) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this still run on the UI thread?

Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to set Handled to true?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a flags enum so should be singular (...KIND)

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.

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

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.

/// 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`
Copy link
Contributor

Choose a reason for hiding this comment

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

should or must?

We should make this requirement more evident in the code examples

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 'must'


/// A collection of unsigned long integers.
[uuid(974A91F8-A309-4376-BC70-7537D686533B), object, pointer_default(unique)]
interface ICoreWebView2UnsignedLongCollection : IUnknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this rather than an array?

Copy link
Contributor

Choose a reason for hiding this comment

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

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`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just should?

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

The app is supposed to watch for new notifications and see if it gets matches?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
// ICoreWebView2Notification members
String Body { get; };
CoreWebView2NotificationDirectionKinds Direction { get; };
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this meant to be the text-reading direction? (CoreWebView2TextDirectionKinds)

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 ensure this is updated after rerunning tool to generate MIDL3.

```csharp
namespace Microsoft.Web.WebView2.Core
{
enum CoreWebView2TextDirectionKinds
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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; };
Copy link
Contributor

Choose a reason for hiding this comment

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

"Vibrate" sounds like bool(s). Looks like this was update to VibrationPattern in the C++ version above?

Copy link
Contributor

Choose a reason for hiding this comment

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

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


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


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

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.

specs/WebNotification.md Show resolved Hide resolved
/// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an arity-overload in the C# but different method name here. Intentional discrepancy?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no sample showing "with an action" vs ReportClicked. Does no-action mean customer clicked the notification body?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

CHECK_FAILURE(args->GetDeferral(&deferral));

// Do the rest asynchronously, to avoid calling MessageBox in an event handler.
m_appWindow->RunAsync([this, deferral, args] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lifetime bug here. We need to extend the lifetime of the args.

Suggested change
m_appWindow->RunAsync([this, deferral, args] {
m_appWindow->RunAsync([this, deferral, args = wil::make_com_ptr(args)] {

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.


```

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

/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

What if an app does

args.Handled = true;
args.Notification.ReportShown();
args.Handled = false; // haha, I changed my mind

Is the rule that once you call a Report method, you cannot un-handle the event?

Copy link
Contributor

Choose a reason for hiding this comment

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

See elsewhere.

/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa, this restriction is buried in the Notifications spec deep in section 2 (Notifications)

Actions are only currently supported for persistent notifications.

Copy link
Contributor

Choose a reason for hiding this comment

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

No change necessary.

///
/// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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".)

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 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think OS notifications support this. Is this a requirement of the DOM API, or can hosts fudge it?

Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

No change required.


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.

Please fix.


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

void WebView_PermissionRequested(object sender, CoreWebView2PermissionRequestedEventArgs args)
{
CoreWebView2Deferral deferral = args.GetDeferral();
System.Threading.SynchronizationContext.Current.Post((_) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow up - check if this is necessary. If necessary also add a comment explaining.

specs/WebNotification.md Show resolved Hide resolved
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

No change necessary.

///
/// 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);
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 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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; };
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
Boolean RequireInteraction { get; };
Boolean RequiresInteraction { get; };

IVectorView<CoreWebView2NotificationAction> Actions { get; };
String BadgeUri { get; };
String ImageUri { get; };
Boolean Renotify { get; };
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure all the MIDL3 matches the COM IDL after running gen tool.

@david-risney david-risney added the review completed WebView2 API Proposal that's been reviewed and now needs final update and push label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review. review completed WebView2 API Proposal that's been reviewed and now needs final update and push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants