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

Jump to bottom

fix using local asset files on windows #16930

Merged
merged 9 commits into from Dec 6, 2023
Merged

fix using local asset files on windows #16930

merged 9 commits into from Dec 6, 2023

Conversation

vividos
Copy link
Contributor

@vividos vividos commented Aug 22, 2023

Description of Change

This PR fixes both issues in #16646 on the windows platform.
The first commit fixes loading WebView content using UrlWebViewSource when the LocalScheme is used, which is https://appdir/.
The second commit fixes the case when using HtmlWebViewSource and the specified BaseUrl has a LocalScheme, but the virtual host name to folder mapping is first set and then cleared again.

Issues Fixed

Fixes #16646

@vividos vividos requested a review from a team as a code owner August 22, 2023 18:12
@ghost ghost added the community ✨ Community Contribution label Aug 22, 2023
@ghost
Copy link

ghost commented Aug 22, 2023

Hey there @vividos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Eilon
Eilon requested changes Aug 24, 2023
View reviewed changes
src/Core/src/Platform/Windows/MauiWebView.cs Outdated Show resolved Hide resolved
src/Core/src/Platform/Windows/MauiWebView.cs Outdated Show resolved Hide resolved
@samhouts samhouts added this to the Under Consideration milestone Aug 29, 2023
@jfversluis
Copy link
Member

/azp run MAUI-DeviceTests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Eilon
Eilon previously requested changes Sep 5, 2023
View reviewed changes
src/Core/src/Platform/Windows/MauiWebView.cs Outdated Show resolved Hide resolved
src/Core/src/Platform/Windows/MauiWebView.cs Outdated Show resolved Hide resolved
@samhouts samhouts requested a review from Eilon September 11, 2023 23:24
@vividos
Copy link
Contributor Author

vividos commented Sep 12, 2023

@Eilon @jfversluis What's the status on this? Would it be possible to get this into .NET 8 GA? Thanks!

@Eilon
Copy link
Member

Eilon commented Sep 12, 2023

I'm not very familiar with using WebView to load local app content so I'm not sure exactly how to evaluate this. I understand the general concept and the code looks to be following good practices.

vividos added a commit to vividos/WhereToFly that referenced this pull request Sep 16, 2023
@vividos
working around some bugs in the MAUI WebView implementation, failing …
1c068e9 
…to correctly setting the BaseUrl for an HtmlWebViewSource

see also:
dotnet/maui#16646
dotnet/maui#16930
@samhouts samhouts added stale Indicates a stale issue/pr and will be closed soon and removed stale Indicates a stale issue/pr and will be closed soon labels Sep 19, 2023
@vividos
Copy link
Contributor Author

vividos commented Sep 23, 2023

@jfversluis @Eilon so what are the next steps here?
I just added two page buttons to the sample project so that the bug and its fix can be tested.

@jfversluis
Copy link
Member

/rebase

@jfversluis

This comment was marked as off-topic.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
@jfversluis
Copy link
Member

We're now running all kinds of new (Windows) tests as part of our pipeline so just rebased it and triggered all the things.

Seems like this change isn't that risky, but the bar at this point is pretty high as we don't want to risk any regressions as we're closer to .NET 8 GA. Let's await the results here and I will bring it up.

Thanks for your patience here!

@vividos
Copy link
Contributor Author

vividos commented Oct 4, 2023

@jfversluis hey Gerald, any news if this PR can get in? Or will it go into a first patch release? Thanks!

@jfversluis
Copy link
Member

I think we'll have to defer this to after GA at this point, sorry, but it's coming!

@vividos
Copy link
Contributor Author

vividos commented Oct 24, 2023

I think this also fixes the issue #6165

@vividos
Copy link
Contributor Author

vividos commented Nov 20, 2023

OK so just to be sure, this works when people will set their own virtual hostname mapping folder? Is that what is captured in the LocalScheme field?

No, not when they set their own virtual hostname folder mapping, but when they use BaseUrl containing the LocalScheme prefix, or when a relative url is specified.

I can't seem to find any documentation on that.

It was developed in PR #7672 to fix #5523, but I guess it was never documented afterwards.

Regardless, this is not something that can be done through .NET MAUI at this time, but people still might with custom mappers so I want to make sure.

The virtual folder mapping is automatically used when using HtmlWebViewSource, so it can be used in MAUI at this time.

Thanks for reviewing this PR! I'll follow up with the requested changes.

@jfversluis
Copy link
Member

No, not when they set their own virtual hostname folder mapping, but when they use BaseUrl containing the LocalScheme prefix, or when a relative url is specified.

OK But it seems that when I use SetVirtualHostNameToFolderMapping I can set a sort of custom local scheme, correct?

If I would set this through a custom handler for my WebView2, will this code in this PR still work? Or will it look for https://appdir/ and just try continue loading as if it were a remote URL?

Or am I understanding this thing completely wrong? 😄

@vividos
Copy link
Contributor Author

vividos commented Nov 20, 2023

Your understanding is correct, this call on WebView2 establishes that local scheme. In UWP this could be done with the ms-appx-web:// URL scheme.

Good question on what would happen when a custom handler interferes with this, e.g. because someone already added a workaround. I would have to try that out...

@jfversluis
Copy link
Member

OK! If we can just check that and somehow take into account that scenario that would be great and we'd have a complete solution I think 😄 Thank you!

@jfversluis jfversluis added the s/pr-needs-author-input PR needs an update from the author label Nov 21, 2023
@ghost
Copy link

ghost commented Nov 21, 2023

Hi @vividos. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

@jfversluis jfversluis removed the request for review from rachelkang November 21, 2023 08:02
@vividos
Copy link
Contributor Author

vividos commented Dec 2, 2023

@jfversluis @MartyIX The commit I just made should solve the last remaining issue, so the PR should be ready.

@jfversluis
Copy link
Member

/azp run MAUI-UITests-public

@azure-pipelines Azure Pipelines

This comment was marked as off-topic.

Sign in to view
@jfversluis

This comment was marked as off-topic.

Sign in to view
vividos and others added 9 commits December 4, 2023 16:09
@vividos @jfversluis
fix using UrlWebViewSource with a URL containing a LocalScheme URL on…
57e0d2a 
… windows that should trigger a call to CoreWebView2.SetVirtualHostNameToFolderMapping()
@vividos @jfversluis
fix using HtmlWebViewSource with a BaseUrl containing a LocalScheme U…
bf374c4 
…RL on windows that should set up virtual host name to folder mapping correctly
@vividos @jfversluis
moved checking uri string with local scheme to own static method
9c2291c
@vividos @jfversluis
moved checking data uri string to own static method
dda39dc
@vividos @jfversluis
replaced ToLowerInvariant() call with StartsWith() parameter StringCo…
ef25a1d 
…mparison.OrdinalIgnoreCase
@vividos @jfversluis
added test buttons to platform specifics samples for Windows WebView …
207e0ca 
…about loading local assets
@vividos @MartyIX @jfversluis
changed sample HTML text to use raw string literals
b51f2c8 
Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
@vividos @MartyIX @jfversluis
put StartsWith() check on one line
9e27557 
Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
@vividos @jfversluis
removed flag _navigatingToHtmlWithBaseUrl and directly recognize WebV…
79ec531 
…iew2 data URI with inserted base tag
@jfversluis
Copy link
Member

/azp run MAUI-UITests-public

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jfversluis jfversluis dismissed Eilon’s stale review December 6, 2023 09:49

Requested changes have been made

@jfversluis jfversluis merged commit aeabd1a into dotnet:main Dec 6, 2023
47 checks passed
@jfversluis
Copy link
Member

Thank you so much for your contribution!

@vividos
Copy link
Contributor Author

vividos commented Dec 6, 2023

@jfversluis thanks Gerald for merging this! Do you have an estimation in which SR this will be going in? Thanks!

@vividos vividos deleted the fix-webview2-local-assets branch December 7, 2023 06:40
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartyIX MartyIX MartyIX left review comments

@jfversluis jfversluis jfversluis approved these changes

@StephaneDelcroix StephaneDelcroix Awaiting requested review from StephaneDelcroix StephaneDelcroix is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@Eilon Eilon Awaiting requested review from Eilon

Assignees
No one assigned
Labels
area/controls 🎮 Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor community ✨ Community Contribution control-webview platform/windows 🪟 s/pr-needs-author-input PR needs an update from the author
Projects
None yet
Milestone
Under Consideration
Development

Successfully merging this pull request may close these issues.

Can't use WebView with local files stored as MauiAssets on Windows
5 participants
@vividos @jfversluis @Eilon @MartyIX @samhouts