-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
HybridWebView control for integrating JS/HTML/CSS easily into a .NET MAUI app #22880
Conversation
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.
Require changes in the publicAPI files:
D:\a\_work\1\s\src\Core\src\Handlers\HybridWebView\HybridWebViewHandler.Windows.cs(68,22): error RS0016: Symbol 'MapSendRawMessage' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md) [D:\a\_work\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-windows10.0.19041.0]
D:\a\_work\1\s\src\Core\src\Handlers\HybridWebView\HybridWebViewHandler.Windows.cs(173,16): error RS0016: Symbol 'SendRawMessage' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md) [D:\a\_work\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-windows10.0.19041.0]
D:\a\_work\1\s\src\Core\src\Handlers\HybridWebView\HybridWebViewHandler.Windows.cs(167,11): error RS0016: Symbol 'HybridPlatformWebView' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md) [D:\a\_work\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-windows10.0.19041.0]
30 Warning(s)
290 Error(s)
Just let me know if can help with anything :)
src/Controls/samples/Controls.Sample/Resources/Raw/HybridSamplePage/index.html
Outdated
Show resolved
Hide resolved
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.
Just scanning the code and got some nit comments
1dd1bcf
to
e4ce427
Compare
e12fdd9
to
59907af
Compare
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.
Just doing a quick review of things since they have changed a fair bit. Mostly comments of names and places. I did not get to review everything yet as I have to go sleep, but for now I just got some thoughts and questions.
Was not sure if this was ready for a review since it is in draft, but we are close to net9 RC so...
/// </summary> | ||
public event EventHandler<HybridWebViewRawMessageReceivedEventArgs>? RawMessageReceived; | ||
|
||
public void SendRawMessage(string rawMessage) |
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.
Is there any way that platforms support a "blocking" request where we could have a SendRawMessageAsync
so that people can await the call and potentially have code that wait for the JS to finish executing? Or is this all a fire-and-forget messaging system?
My main thought is that if you want to invoke multiple messages, but they need to run in series and not just span and maybe a small delay somewhere will mix the order.
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.
There are several variations of these methods that I'll be adding in subsequent PRs, including async versions, plus method invocation. Some of this is tracked in #22303 and #22304.
You can technically implement this right now manually by sending messages back and forth like a state machine, but it's of course not ideal. I described this PR to another person as M-MVP (Minimum Minimal Viable Product 😁 ).
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Android.cs
Outdated
Show resolved
Hide resolved
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Android.cs
Outdated
Show resolved
Hide resolved
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Android.cs
Outdated
Show resolved
Hide resolved
|
||
public void SendRawMessage(string rawMessage) | ||
{ | ||
Handler?.Invoke(nameof(IHybridWebView.SendRawMessage), rawMessage); |
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.
For future proofing, this should not pass the string directly but instead pass a new record type:
// in src/Core/src/Primitives/SendRawMessageRequest.cs
namespace Microsoft.Maui
{
public record SendRawMessageRequest(string rawMessage);
}
If there is any need to return a value - such as a result of the request or some response, then inherit from RetrievePlatformValueRequest<T>
:
// in src/Core/src/Primitives/SendRawMessageRequest.cs
namespace Microsoft.Maui
{
public class SendRawMessageRequest(string rawMessage) : RetrievePlatformValueRequest<string>
{
// base members:
// public string Result { get; }
// public void SetResult(string result);
// public bool TrySetResult(string result);
}
}
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.
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.
In the original prototype the pattern was that the messages were encoded as JSON and there was a message type ID that said whether it was raw, method invoke, etc. Not sure what the new design will be but it will be capable of supporting multiple message types.
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 mean that for the maui codebase, we want to be consistent and use a wrapper for commands so people can cast instead of each command being its parameter data type.
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.
Discussed in the mtg; something like this will definitely happen but I'd rather do it once I add another message type (such as to invoke methods). That way I'll have a clearer sense of the design.
private Stream? PlatformOpenAppPackageFile(string filename) | ||
{ | ||
filename = PathUtils.NormalizePath(filename); | ||
|
||
try | ||
{ | ||
return _handler.Context.Assets?.Open(filename); | ||
} | ||
catch (Java.IO.FileNotFoundException) | ||
{ | ||
return null; | ||
} | ||
} | ||
|
||
internal static class PathUtils | ||
{ | ||
public static string NormalizePath(string filename) => | ||
filename | ||
.Replace('\\', Path.DirectorySeparatorChar) | ||
.Replace('/', Path.DirectorySeparatorChar); | ||
} |
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 this rather use FileSystemPlatformOpenAppPackageFile
instead of duplicating code?
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.
The MAUI FileSystem APIs are unfortunately async-only (normally a good thing), but the Android API being implemented here is sync-only. For that reason I did the call directly because ultimately the low-level file system API on Android is sync anyway (just regular file system call). I have been unable to find a pattern to do async interception of Android WebView requests.
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 create an issue to track this, we already have FileSystemUtils.NormalizePath
so we probably should create a helper for this and use it everywhere vs copying in all places.
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.
Filed #23668.
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Tizen.cs
Outdated
Show resolved
Hide resolved
7fe7393
to
2929e41
Compare
2929e41
to
c4d65c5
Compare
@mattleibow - thank you for the feedback, I think I've addressed or responded to each item. The PR should be ready for final review. I just rebased to net9 so that could bring up some new issues, but hopefully nothing too shocking. |
@mattleibow / @Redth / @tj-devel709 - this PR is good to go from my perspective as an initial step. Future PRs should theoretically be smaller because they'll just be individual sub-features (invoking C#/JS methods, debug support, etc.). It's important we get this into the next preview train, so if you have any feedback, please let me know (especially if it's urgent for this PR to go in). |
@@ -205,7 +205,7 @@ Task("Test") | |||
.IsDependentOn("SetupTestPaths") | |||
.Does(() => | |||
{ | |||
var waitForResultTimeoutInSeconds = 120; | |||
var waitForResultTimeoutInSeconds = 240; |
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.
On CI the tests were failing due to timeout, so I doubled it.
@mattleibow / @Redth / @tj-devel709 - how can I help make this code review easiest so that it can go in before the next preview locks down? There are only a few 'moving' parts to this:
|
else | ||
{ | ||
eventArgs.Response = sender.Environment!.CreateWebResourceResponse( | ||
Content: await CopyContentToRandomAccessStreamAsync(contentStream), |
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 try catch any of this? Or use our FireAndForget and move this to a separate method?
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 essentially copied from BlazorWebView so it's fairly tried-and-true. There's a fair bit of 'error' checking at each level, and there have been no reports of problems with the code (except now that I've jinxed it!).
@mattleibow / @PureWeen / @Redth / @tj-devel709 - alright the CI is green and I believe I've addressed all feedback. Thank you for the review the other day; I think the discussion put HybridWebView in a better place! |
string? DefaultFile { get; set; } | ||
string? HybridRoot { get; set; } |
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.
The properties should be removed since this can be obtained from the VirtualView directly.
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 based on the original experimental code at https://github.com/Eilon/MauiHybridWebView
Description of Change
A new HybridWebView control that hosts a local JS/HTML/CSS application in a cross-platform WebView. The C# (.NET) code in the .NET MAUI app and the JS code in the HybridWebView can communicate with each other via messages. The entire app is self-contained; that is, there is no network traffic or HTTP calls used to host the app (unless the app chooses to request remote content, such as calling a web API).
Issues Fixed
Fixes #22302