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

Screen Capture Requested #3655

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Screen Capture Requested #3655

wants to merge 6 commits into from

Conversation

srwei
Copy link

@srwei srwei commented Jul 20, 2023

No description provided.

@srwei srwei added the API Proposal Review WebView2 API Proposal for review. label Jul 20, 2023
@srwei srwei requested a review from david-risney July 20, 2023 23:55
webView.CoreWebView2.ScreenCaptureRequested += (sender, screenCaptureArgs) =>
{
// Get Frame Info
wil::com_ptr<ICoreWebView2FrameInfo> frameInfo;

Choose a reason for hiding this comment

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

It is not a C#

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a similar one below

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

get a video stream of a user's tabs, windows, or desktop. This API is available in WebView2,
but the current default UI has some problems that we need to fix to make sure that hybrid apps
using WebView2 have a more seamless web/native experience. This includes removing the tab column
in the UI, replacing default strings and icons that do not match in WV2, and potentially having
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 talking about UI that is never shown in the spec, so it's not very helpful. But really, the second half of the paragraph sounds like inside baseball. Maybe shorten to

This API is available in WebView2 and displays a default picker which may not be appropriate for all apps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking my understanding ... This API isn't about enabling the host to make the experience more seamless (replacing default strings and such), it's just about the ability to cancel?

Copy link
Contributor

Choose a reason for hiding this comment

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

Next time we'll separate out back story and future plans from rest of background.

invoked.

In the case of a nested iframe requesting permission, we will raise the event
off of the top level iframe.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nested frames are weird in the WebView2 API surface.

  • Sometimes actions related to nested frames result in the event being raised on the nested frame. (DOMContentLoaded, navigation events, WebMessageReceived.)
  • Sometimes actions related to nested frames result in the event being raised on the top level frame + the root document. (PermissionRequested, ScreenCaptureRequested.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Dave: Follow up with how we want events to behave wrt frames in the future. Plus public document on event patterns in WebView2.

// CoreWebView2Frame event handler, then we will not raise the
// ScreenCaptureRequested event off the CoreWebView2.

CHECK_FAILURE(args->put_Handled(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

After we set Handled = true, what does it mean when Cancel is left as its default value of false?

Does it mean "Don't cancel, show default UI"?

Does it mean "Don't cancel, allow silent capture without any prompt"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean "Don't cancel, show default UI"?

This is what happens

// ScreenCaptureRequested event off the CoreWebView2.

CHECK_FAILURE(args->put_Handled(true));
return S_OK;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if I set Cancel = true but don't set Handled? Does the next handler observe Cancel = true? Or does each handler get a fresh Cancel = false?

What happens if nobody sets Handled = true, but somebody sets Cancel = true? Does that cancel? Or do you have to Handle the event in order to Cancel it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed above


We propose introducing the `ScreenCaptureRequested` event. This event will be raised whenever
the WebView2 and/or iframe corresponding to the CoreWebView2Frame or any of its descendant iframes
requests permission to use the Screen Capture API before the UI is shown.
Copy link
Contributor

Choose a reason for hiding this comment

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

From the name "Requested", I was expecting that this was asking the host to do something. E.g. BasicAuthenticationRequested is asking the host for a response. Really, I think this event is saying "I'm about to start the screen capture process, this is your chance to reject it"? That suggests a name more like ScreenCaptureStarting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we more align the name with getDisplayMedia?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this to align with the other ..Starting events

  • ScreenCaptureStarting

before the `CoreWebView2` event handlers. If `Handled` is set true as part of
the `CoreWebView2Frame` event handlers, then the `ScreenCaptureRequested` event
will not be raised on the `CoreWebView2`, and its event handlers will not be
invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there really 4 cases that need to be supported (the combinations of Handled and Cancel)? I think if you set Cancel you're always going to also set Handled. And if you don't set Cancel I think you're not going to set Handled.

I.e., I don't think Handled is necessary; if Cancel is set, then stop raising events.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, the double boolean is clearer that one of them controls propagation and the other controls the system behavior. It also aligns with other uses of Handled and Cancel.

You might set Handled=true and Cancel=false to mean "Trust me, it's okay. Don't need to ask others for permission to capture. I give you permission to capture."

And you might set Handled=false and Cancel=true to mean "I think you should cancel, but call the other handlers because one of them might have a stronger opinion."

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave as is: match existing xaml / dom patterns of bubbling

wil::com_ptr<ICoreWebView2> m_webviewEventSource;
EventRegistrationToken m_screenCaptureRequestedToken = {};

m_webviewEventSource->add_ScreenCaptureRequested(
Copy link

Choose a reason for hiding this comment

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

This should just be m_webView or something similar. It is not an EventSource.

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 rename. Also its named like a member but looks like a local. Probably needs to be updated to fix that as well.

```c#
private WebView2 webView;

m_webview.CoreWebView2.FrameCreated += (sender, frameCreatedArgs) =>
Copy link

Choose a reason for hiding this comment

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

Inconsistent variable name 'webView'/'m_webView'.

Copy link
Contributor

Choose a reason for hiding this comment

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

similar issue to above with member / local naming and logic. Please fix similar to above.

before the `CoreWebView2` event handlers. If `Handled` is set true as part of
the `CoreWebView2Frame` event handlers, then the `ScreenCaptureRequested` event
will not be raised on the `CoreWebView2`, and its event handlers will not be
invoked.
Copy link

Choose a reason for hiding this comment

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

Assuming nothing handles the event on the CoreWebView2Frame, we will raise the event on the CoreWebView2. The two events share the same args, so the CoreWebView2 event args has a Handled property. What does Handled do in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does nothing in this case. We should explicitly document that is doing nothing in that case.

Dave: Follow up with how we want Handled to behave in this sort of case in the future.

{
// Get Frame Info
wil::com_ptr<ICoreWebView2FrameInfo> frameInfo;
CHECK_FAILURE(args->get_FrameInfo(&frameInfo));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be OriginalSourceFrameInfo? + other places

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

`CoreWebView2Frame` event handlers will be invoked first,
before the `CoreWebView2` event handlers. If `Handled` is set true as part of
the `CoreWebView2Frame` event handlers, then the `ScreenCaptureRequested` event
will not be raised on the `CoreWebView2`, and its event handlers will not be
Copy link

Choose a reason for hiding this comment

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

I assume that if I set args.Cancel=True for CoreWebView2Frame.ScreenCaptureRequested but don't mark it as handled, when CoreWebView2.ScreenCaptureRequested is raised, the args.Cancel property will return true?

If so, it would be worth explicitly stating so in the doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explicitly note in docs: if you don't mark handled true then the same event args (with modifications) bubble up to the next event handler on the corewebview2

get a video stream of a user's tabs, windows, or desktop. This API is available in WebView2,
but the current default UI has some problems that we need to fix to make sure that hybrid apps
using WebView2 have a more seamless web/native experience. This includes removing the tab column
in the UI, replacing default strings and icons that do not match in WV2, and potentially having
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking my understanding ... This API isn't about enabling the host to make the experience more seamless (replacing default strings and such), it's just about the ability to cancel?

CHECK_FAILURE(frameInfo->get_Source(&frameSource));

// If the host app wants to cancel the request for a specific source
if (frameSource = "https://developer.microsoft.com/en-us/microsoft-edge/webview2/")
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
if (frameSource = "https://developer.microsoft.com/en-us/microsoft-edge/webview2/")
if (frameSource == "https://developer.microsoft.com/en-us/microsoft-edge/webview2/")

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

CHECK_FAILURE(frameInfo->get_Source(&frameSource));

// If the host app wants to cancel the request for a specific source
if (frameSource = "https://developer.microsoft.com/en-us/microsoft-edge/webview2/")
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 a safe comparison? The trailing "/" is guaranteed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please reuse other sample code that does URI comparison. Also compare just hostname.


We propose introducing the `ScreenCaptureRequested` event. This event will be raised whenever
the WebView2 and/or iframe corresponding to the CoreWebView2Frame or any of its descendant iframes
requests permission to use the Screen Capture API before the UI is shown.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we more align the name with getDisplayMedia?

/// `CoreWebView2Frame` event handlers invoked first. The host may
/// set this flag to `TRUE` within the `CoreWebView2Frame` event handlers
/// to prevent the remaining `CoreWebView2` event handlers from being
/// invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also the remaining frame handlers from being invoked?

Copy link
Contributor

Choose a reason for hiding this comment

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

The text is currently correct. It will need to be updated in the future when we support grand+child frames.

///
/// If a deferral is taken on the event args, then you must synchronously
/// set `Handled` to TRUE prior to taking your deferral to prevent the
/// `CoreWebView2`s event handlers from being invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have this pattern anywhere else? Generally when you take a deferral, you can set results later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave this as is. We do have this pattern elsewhere in WebView2.

Dave: Include this in how event bubbling should work

[propput] HRESULT Handled([in] BOOL handled);
/// The host may set this flag to cancel the screen capture. If canceled,
/// the screen capture UI is not displayed regardless of the
/// `Handled` property.
Copy link
Contributor

Choose a reason for hiding this comment

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

(Although if you don't set Handled, someone downstream might clear your Cancel)

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 this to the documentation.

Copy link
Contributor

@david-risney david-risney left a comment

Choose a reason for hiding this comment

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

Please update per feedback, thanks!

get a video stream of a user's tabs, windows, or desktop. This API is available in WebView2,
but the current default UI has some problems that we need to fix to make sure that hybrid apps
using WebView2 have a more seamless web/native experience. This includes removing the tab column
in the UI, replacing default strings and icons that do not match in WV2, and potentially having
Copy link
Contributor

Choose a reason for hiding this comment

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

Next time we'll separate out back story and future plans from rest of background.


We propose introducing the `ScreenCaptureRequested` event. This event will be raised whenever
the WebView2 and/or iframe corresponding to the CoreWebView2Frame or any of its descendant iframes
requests permission to use the Screen Capture API before the UI is shown.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this to align with the other ..Starting events

  • ScreenCaptureStarting

`CoreWebView2Frame` event handlers will be invoked first,
before the `CoreWebView2` event handlers. If `Handled` is set true as part of
the `CoreWebView2Frame` event handlers, then the `ScreenCaptureRequested` event
will not be raised on the `CoreWebView2`, and its event handlers 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.

Explicitly note in docs: if you don't mark handled true then the same event args (with modifications) bubble up to the next event handler on the corewebview2

before the `CoreWebView2` event handlers. If `Handled` is set true as part of
the `CoreWebView2Frame` event handlers, then the `ScreenCaptureRequested` event
will not be raised on the `CoreWebView2`, and its event handlers will not be
invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave as is: match existing xaml / dom patterns of bubbling

before the `CoreWebView2` event handlers. If `Handled` is set true as part of
the `CoreWebView2Frame` event handlers, then the `ScreenCaptureRequested` event
will not be raised on the `CoreWebView2`, and its event handlers will not be
invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

It does nothing in this case. We should explicitly document that is doing nothing in that case.

Dave: Follow up with how we want Handled to behave in this sort of case in the future.

webView.CoreWebView2.ScreenCaptureRequested += (sender, screenCaptureArgs) =>
{
// Get Frame Info
wil::com_ptr<ICoreWebView2FrameInfo> frameInfo;
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 fix.

```c#
private WebView2 webView;

m_webview.CoreWebView2.FrameCreated += (sender, frameCreatedArgs) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

similar issue to above with member / local naming and logic. Please fix similar to above.

/// `CoreWebView2Frame` event handlers invoked first. The host may
/// set this flag to `TRUE` within the `CoreWebView2Frame` event handlers
/// to prevent the remaining `CoreWebView2` event handlers from being
/// invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

The text is currently correct. It will need to be updated in the future when we support grand+child frames.

///
/// If a deferral is taken on the event args, then you must synchronously
/// set `Handled` to TRUE prior to taking your deferral to prevent the
/// `CoreWebView2`s event handlers from being invoked.
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave this as is. We do have this pattern elsewhere in WebView2.

Dave: Include this in how event bubbling should work

[propput] HRESULT Handled([in] BOOL handled);
/// The host may set this flag to cancel the screen capture. If canceled,
/// the screen capture UI is not displayed regardless of the
/// `Handled` property.
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 this to the documentation.

@david-risney david-risney added the review completed WebView2 API Proposal that's been reviewed and now needs final update and push label Aug 10, 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.

7 participants