-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support WebViewControl functionality for 17692 #2219
Support WebViewControl functionality for 17692 #2219
Conversation
Minimum version updated to April 2018 Update, the first generally available OS version with metadata contract 6 and the WebViewControl APIs necessary. The target version has been updated to latest Insider version 17677, the minimum version required to support new WebView features, such as Partition.
The reference now prefers the latest version of the SDK if it exists, then moves to the minimum version. If neither SDK are available, the reference is unresolved.
The property was introduced in a later version of the SDK. The value is gated on a check on `ApiInformation.IsPropertyPresent` to verify the property is available on the type. If the property is not present, the wrapper type holds on to the value and it is simply dropped when creating the WinRT type with the process options.
Master changed folder structure. Move file for new structure
This reverts commit 740fc50.
Underlying events from WebViewControl for GotFocus and LostFocus raise the respective events in WinForms and WPF.
namespace Microsoft.Toolkit.Win32.UI.Controls | ||
{ | ||
/// <inheritdoc /> | ||
public interface IWebView2 : IWebView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just break the original? I doubt anyone would be implementing their own
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be released with 3.1, which according to semver would not permit such a breaking change. On 4.0 it could be merged to create that single interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it's a breaking change, but I can't imagine anyone is implementing it. I'm ok with merging them in 4.0. Should be added to the breaking changes issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it to #2178
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since our next release is 4.0, does it make sense now to introduce the breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -678,8 +682,19 @@ private void OnFrameNavigationStarting(WebViewControlNavigationStartingEventArgs | |||
} | |||
} | |||
|
|||
private void OnGotFocus(object args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this not protected virtual
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. Class is sealed
@@ -691,6 +706,17 @@ private void OnLongRunningScriptDetected(WebViewControlLongRunningScriptDetected | |||
|
|||
private void OnLongRunningScriptDetected(IWebViewControl sender, Windows.Web.UI.WebViewControlLongRunningScriptDetectedEventArgs args) => OnLongRunningScriptDetected(args); | |||
|
|||
private void OnLostFocus(object args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this not protected virtual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. Class is sealed
Note: this requires #2220 or similar to be able to install the required SDK update |
Add UserAgent introduced in 17686
Since WebViewControlProcessOptions is used directly, use check prior to invocation to ensure property on type exists to avoid runtime error on downlevel clients. Downlevel clients will receive an empty string if property not present.
The types should have been sealed with initial release.
ce0a0a6
to
771e70c
Compare
- Need documentation for how to use AddPreLoadedScript. - API appears to be non-functional at this point in time.
58fbc76
to
4d4bbeb
Compare
Should we review and merge this on a separate branch for now and setup up a custom VM for building? /cc @onovotny |
No opinion about a separate branch, but we'll hopefully have progress on getting the hosted agents working shortly. |
Updating to 17704 as installed by CommunityToolkit#2220
Includes information about the PR/CI YAML files as well as installing on own development machine by manually invoking the PowerShell file.
Since the SDK will undergo a major version, combine interfaces
@@ -60,12 +61,50 @@ private WebViewControlProcess(Windows.Web.UI.Interop.WebViewControlProcess proce | |||
/// <value><c>true</c> if this instance can access the private network; otherwise, <c>false</c>.</value> | |||
public bool IsPrivateNetworkClientServerCapabilityEnabled => _process.IsPrivateNetworkClientServerCapabilityEnabled; | |||
|
|||
/// <summary> | |||
/// Gets a value indicating the partition of the web view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention the version here that this is valid on? (same below?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's tricky to know what version it is valid with--the API is versioned at a different cadence than the OS, it just so happens we "snap" and that's what's out there. We handle support by asking the universal contract if a type, property, method, event (etc.) is available, so we don't care what version. Right now the API is visible in insiders fast, insiders slow(ish), and other RS5 builds.
retval.EnterpriseId = EnterpriseId; | ||
if (!string.IsNullOrEmpty(options?.Partition)) | ||
{ | ||
retval.Partition = options?.Partition; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is safe now as it's checked so '?' is redundant? (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0091a1f
Per code review feedback, the result of the expression is already checked for null, so the check in the assignment is redundant.
# Conflicts: # Microsoft.Toolkit.Win32/Tests/UnitTests.WebView.WinForms/UnitTests.WebView.WinForms.csproj
Issue: #2180
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Missing new functionality introduced in Windows SDK update. See #2180
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Other information
Note: this requires #2220 or similar to be able to install the required SDK update
Resolves #2180