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.

Sign up for GitHub

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

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1c1d3d0
Add Web Notification API Spec draft
peiche-jessica Dec 9, 2022
952a757
Apply suggestions from code review
peiche-jessica Dec 13, 2022
e03a214
Address initial feedback part 1
peiche-jessica Dec 13, 2022
cbd9d61
Add enum CoreWebView2TextDirectionKinds to c# API details
peiche-jessica Dec 13, 2022
5e22e5f
Report* and CloseRequested
peiche-jessica Jan 4, 2023
0b9a618
Persistent -> ProfileNotificationReceivedEventHandler
peiche-jessica Jan 6, 2023
61d7ae7
Address initial feedback part 2
peiche-jessica Jan 6, 2023
c61f653
Update ref doc
peiche-jessica Jan 6, 2023
b8395cb
Add DataAsJson and TryGetDataAsString
peiche-jessica Jan 9, 2023
88418d0
Add links to Notification.actions
peiche-jessica Jan 9, 2023
0b6a3aa
Apply API name suggestion
peiche-jessica Jan 10, 2023
25207b4
Clarify Handled and ReportXXX methods
peiche-jessica Jan 10, 2023
8ef88d3
Clarify deferral info
peiche-jessica Jan 10, 2023
4908aaf
Fix sample code
peiche-jessica Jan 10, 2023
993fe9f
Make ReportClicked and ReportClosed async
peiche-jessica Jan 10, 2023
320baf0
Address feedback
peiche-jessica Feb 1, 2023
16ba2bc
Update sample
peiche-jessica Feb 3, 2023
4493c00
Merge pull request #3049 from MicrosoftEdge/api-notification-draft
peiche-jessica Feb 3, 2023
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
Clarify Handled and ReportXXX methods
  • Loading branch information
@peiche-jessica
peiche-jessica committed Jan 10, 2023
commit 25207b430a22003d6693cde22afea29e2156cc49
60 changes: 39 additions & 21 deletions specs/WebNotification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,12 +346,14 @@ interface ICoreWebView2NotificationReceivedEventArgs : IUnknown {
///
/// 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. If after the event handler or deferral
/// completes `Handled` is set to FALSE then WebView will display the default
/// notification UI. Note that if `ReportShown` has been called on the `Notification`
/// object, WebView will not display the default notification regardless of
/// the Handled property. The default value is FALSE.
/// 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'

/// before you call `ReportShown`, `ReportClicked`, `ReportClickedWithAction`
/// and `ReportClosed`, otherwise they will fail with `E_ABORT`. If after the
Copy link
Contributor

Choose a reason for hiding this comment

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

The sample doesn't follow this rule. It takes a deferral (which postpones the effect of the Handled property), but calls ReportShown before completing the deferral. Is the sample in error?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of E_ABORT, please use HRESULT_FROM_WIN32(ERROR_INVALID_STATE). Also, please see if we have a way to write to the debugger/console more information.

/// event handler or deferral completes `Handled` is set to FALSE then WebView
/// will display the default notification UI. Note that you cannot un-handle
/// this event once you have set `Handled` to be `TRUE`. The default value is
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
/// this event once you have set `Handled` to be `TRUE`. The default value is
/// this event once you have set `Handled` to be `TRUE`. The initial value is

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.

/// FALSE.
[propput] HRESULT Handled([in] BOOL value);

/// Gets whether the `NotificationReceived` event is handled by host.
Expand Down Expand Up @@ -431,28 +433,44 @@ interface ICoreWebView2Notification : IUnknown {
HRESULT remove_CloseRequested(
[in] EventRegistrationToken token);

/// The host may run this to report the notification has been displayed.
/// The NotificationReceived event is considered handled regardless of the
/// Handled property of the NotificationReceivedEventArgs if the host has
/// run ReportShown().
/// 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'

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

HRESULT ReportShown();

/// The host may run this to report the notification has been clicked. Use
/// `ReportClickedWithAction` to specify an action to activate a persistent
/// notification. This will no-op if `ReportShown` is not run or `ReportClosed` has been
/// run.
/// The host may run this to report the notification has been clicked, and it
/// will cause the
/// [click](https://developer.mozilla.org/docs/Web/API/Notification/click_event)
/// event to be raised for non-persistent notifications and the
/// [notificationclick](https://developer.mozilla.org/docs/Web/API/ServiceWorkerGlobalScope/notificationclick_event)
/// event for persistent notifications. Use `ReportClickedWithAction` to specify an
/// action to activate a persistent notification.
/// You should only run this if you are handling the `NotificationReceived`
/// event. Returns `E_ABORT` if `Handled` is `FALSE` or `ReportShown` has not
/// been run when this is called.
HRESULT ReportClicked();

/// The host may run this to report the persistent notification has been
/// activated with a given action. The action index corresponds to the index
/// in NotificationActionCollectionView. This returns `E_INVALIDARG` if an invalid
/// action index is provided. Use `ReportClicked` to activate an non-persistent
/// notification. This will no-op if `ReportShown` is not run or `ReportClosed` has been
/// run.
/// 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

/// [notificationclick](https://developer.mozilla.org/docs/Web/API/ServiceWorkerGlobalScope/notificationclick_event)
/// event to be raised. The action index corresponds to the index in
/// NotificationActionCollectionView. You should only run this if you are
/// handling the `NotificationReceived` event. Returns `E_ABORT` if `Handled`
/// 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);

/// The host may run this to report the notification was dismissed.
/// This will no-op if `ReportShown` is not run or `ReportClicked` has been run.
/// The host may run this to report the notification was dismissed, and it
/// will cause the
/// [close](https://developer.mozilla.org/docs/Web/API/Notification/close_event)
/// event to be raised for non-persistent notifications and the
/// [notificationclose](https://developer.mozilla.org/docs/Web/API/ServiceWorkerGlobalScope/notificationclose_event)
/// event for persistent notifications. You should only run this if you are
/// handling the `NotificationReceived` event. Returns `E_ABORT` if `Handled`
/// is `FALSE` or `ReportShown` has not been run when this is called.
HRESULT ReportClosed();

/// A string representing the body text of the notification.
Expand Down