-
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
Improved non-HTTP(S) link handling behavior #5313
Conversation
Also fixed crashing issues when a scheme has no handler. This is consistent with other platforms.
Before this gets merged, we will probably want to consider whether the behavior I've described in my previous comment is desirable. The |
I realize this wasn't covered in the original issue, but what happens if you use links like these on other platforms? Specifically, iOS with custom URL Schemes? It's also possible on Windows (ex. "do you want to allow example.com to open Just understanding if we get crashes on those platforms, similar to what we saw on Android, would be good. As long as it's not a crash, this would probably fall pretty low on our priority list to address though. |
@TanayParikh I've tested links with various schemes on other platforms, and they don't seem to have any issues. If the scheme is known, the link is handled correctly, and if the scheme is unknown, nothing happens when the link is clicked. This is why I changed the Android implementation to no-op when the scheme doesn't have a handler. |
if (callbackArgs.ExternalLinkNavigationPolicy == ExternalLinkNavigationPolicy.OpenInExternalBrowser) | ||
{ |
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 name OpenInExternalBrowser
really could mean OpenInExternalApp
. Should we consider renaming this?
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.
Good point. What do you think - OpenInExternalApp
? OpenExternal
? OpenExternally
? I'm fine with any. cc @TanayParikh in case he has opinions since he did the earlier version of this.
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 name
OpenInExternalBrowser
really could meanOpenInExternalApp
.
Would this be specific to android schemes, or would it apply to iOS custom schemes as well? Or is it handled differently for iOS?
Should we even give an option to open external/internal for intent? What would happen if the user requested OpenInWebview for an intent?
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.
Ideally the same APIs would have as close-to-equivalent-as-we-can behaviors across all platforms. So if we can do it for iOS too, yes.
Should we even give an option to open external/internal for intent? What would happen if the user requested OpenInWebview for an intent?
My guess is that developers would only want to use OpenInExternalBrowser
or CancelNavigation
for intents. But if they explicitly tell us to InsecureOpenInWebView
, would it be OK to just let them? I know no good will likely come of it, so they probably shouldn't do it, but if they really want to then I'm not sure it's our responsibility to stop them.
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.
Ideally the same APIs would have as close-to-equivalent-as-we-can behaviors across all platforms. So if we can do it for iOS too, yes.
Assuming we're handling iOS/Windows external schemes in the same relative fashion, I think OpenExternally
or OpenExternal
makes sense.
I know no good will likely come of it, so they probably shouldn't do it, but if they really want to then I'm not sure it's our responsibility to stop them.
Fair enough. Can we just make sure if they do do OpenInWebView
with an intent it just no-ops instead of crashing (not sure what the current code path will do with intents directly). Once we're in a better state with logging, we may also wish to add some logging in place for this to alert developers of a potentially un-intended code path being executed.
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.
Would this be specific to android schemes, or would it apply to iOS custom schemes as well? Or is it handled differently for iOS?
iOS will also open links with non-browser apps if a URL scheme is registered with another app (this was something I tested). So even currently, OpenInExternalBrowser
doesn't always guarantee that the link will be opened in a browser app.
What would happen if the user requested OpenInWebview for an intent?
IIRC, assuming the scheme is valid, the webview will just display a "Webpage not available" page (but I can double-check this on Monday). Assuming that's the case, I agree that it's probably fine to allow that page to display without enforcing anything ourselves.
request is not null && | ||
request.IsRedirect && | ||
request.IsForMainFrame) | ||
if (AppOriginUri.IsBaseOfPage(uri)) |
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'm not sure why we're using IsBaseOfPage
here instead of IsBaseOf
. Does it matter whether or not it's a page?
I thought that distinction was only relevant when deciding whether to do fallback routing.
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 guess my reasoning was that if we're trying to load a file, it seems like we should give the developer the option to open it in an external app/browser (i.e., we only load URLs in the view without invoking the ExternalNavigationStarting
callback if they refer to other pages in the app). I could be making a wrong assumption here so I'm certainly happy to change it.
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 personally would have regarded "external" navigation to relate to content or actions that are outside the application, and would intuitively consider static content in wwwroot
to be internal (or perhaps routing patterns like /users/{fullname}
matching /users/bob.gif
- Bob Gif is a possible real name).
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.
would intuitively consider static content in wwwroot to be internal
Would that work across all content types? Do we have webview internal PDF support for instance across all the platforms?
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.
(or perhaps routing patterns like /users/{fullname} matching /users/bob.gif - Bob Gif is a possible real name).
The developer could still support this if they wanted by adding their own handler for ExternalNavigationStarting
and indicating that they want the URL to be opened in the webview (but this does seem like a bit of a hack IMO). Whereas, if we always loaded internal resources in the webview, I don't know that we have a way for the developer to override that behavior to support external navigation.
But there's something else: I think the existing fallback routing logic could still cause problems anyway for URLs ending in .<something>
. I wonder if we should do something like re-processing the request as a navigation event if we fail to load an internal resource from the request URL (basically falling back to the host page if our initial guess was wrong). That way, users hopefully won't ever get the weird "There was no content at _____" page.
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.
Instead, use a button or similar with an @OnClick handler that does something useful for the target file (for example, opening it in an external app using MAUI APIs)
Ah I wasn't aware we had MAUI APIs to force the loading into an external (PDF Reader) app. In that case I agree this is all fine given developers at least have the optionality to open externally should it be something they want to do.
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.
Sorry I may have missed something here. Isn't that already our current behavior? Are you suggesting we don't go the route of UrlLoadingStrategy and just keep things as is?
I may not have been clear, my bad. I was suggesting that the current default behavior for loading file-like URLs internal to the base href is fine, but we would give developers the option to override it via UrlLoadingStrategy
.
Normally I'd agree, we want to minimize the complexity however possible.
I guess my concerns with this were that:
- If we special-case PDF handling, what should we do about other image formats (or other file types that browsers provide a preview for)? Are we going to evaluate their support on each platform and decide what best the default behavior is?
- Would we ever change the default behavior if a webview on some platform adopted a sufficient built-in PDF viewer? For example, the Android webview doesn't have support for previewing PDFs (so presumably we would open in an external app), but iOS does support it. So should we open within the webview on iOS but not Android? But then what if Android someday gets its own PDF viewer, will we change the default behavior again? Maybe we decide that for now we always open PDFs externally on both platforms, but what if the webview PDF viewers on both platforms improve to the extent where it's unnecessary to do so?
That said, maybe I'm overthinking this and it's fine to only consider the PDF format for now, and always open it externally on mobile platforms. I'm fine taking that route, just wanted to share some potential concerns 🙂
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.
Oops, I didn't refresh and missed a bunch of comments before posting, sorry!
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.
@MackinnonBuck This is a long thread and I'm no longer quite sure whether this is a clear plan from your perspective, or whether you're still blocked on getting anyone's input. If you're unblocked, great! But if you need anything please let us know.
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 should be unblocked for now, thanks @SteveSandersonMS! I think most of the work is done, the primary remaining TODOs are:
- Completing/verifying the new link handling functionality on iOS/Mac Catalyst.
- Determining what should be done about pages with URLs that look like file names. I've implemented a solution for WinUI, but I'm not sure if we want to include it in this PR (we would probably need something for the other platforms as well). My current approach is:
- If the URL looks like it has a file extension, attempt to load that file.
- If the URL does not have a file extension, or the file could not be found in the previous step, and WebView2 has indicated that this could be an internal navigation request, fall back on the host page.
Hoping to at least get 1. finished soon so this PR can get reviewed/merged by EOD tomorrow.
This is a great improvement - thanks @MackinnonBuck! I think we'll easily be able to understand this logic years from now. |
I think the big remaining question here is how we want to handle URLs that look like they point to internal files, but actually refer to pages. Do you have any thoughts on this @SteveSandersonMS @TanayParikh? My previous comment on this: #5313 (comment) |
iOS is still TODO.
Page URLs with patterns that make them look like files are now supported. Still need to decide how to approach this on the other platforms.
Okay, I think this PR is ready to be reviewed again @SteveSandersonMS @TanayParikh! I'm going to add some thoughts here to provide some context on the changes I made. I changed the This new rule does introduce a subtle change in behavior from an earlier version of this PR. Rather than doing something like The reason I bring this up is that the default behavior now makes it so if you click a link like Okay, on to the next thing: I've renamed And finally, I've also changed the MAUI WinUI fallback routing behavior to support pages with URLs that appear to have file extensions. How this works was outlined in a previous comment. This makes the behavior between all WebView2-based Blazor Hybrid platforms consistent (since WPF and WinForms already behave correctly for pages with "extensions"). I realize this change could be considered slightly off-topic for this PR, so I'm happy to undo the change if we need more time to think over the approach. I think the main downside with this change is that now WinUI is the only MAUI platform that supports this behavior. I haven't been able to come up with a good strategy for iOS/Mac Catalyst and Android yet (e.g. I don't think overzealously falling back on the host page when we get a 404 on an internal resource is desirable). |
Thanks @MackinnonBuck! This has ended up as a fairly significant change but I think it's good we got to this before GA. Letting developers control this behavior for all navigations is a valuable feature.
I have to admit I don't fully understand the design intent with this. TBH I don't know why anyone would be using the If we could conclude that people aren't actually going to link to Again, if I'm missing some known use case for same-host-different-scheme (given the host is always
Sounds good.
While I agree the behavior you've set up for WinUI is good in isolation, I do wonder if having an inconsistency across WinUI and Android/iOS might end up becoming a problem in itself. Developer might only test on Windows, then be confused about why a link to I would probably err on the cautious side by going with a least-common-denominator behavior across all MAUI platforms and filing an issue to consider enhancing this in the future. Is the following viable across all?
Then routing to |
TBH you're probably right that there isn't a practical purpose to this - I thought there might be some reason to use an alternate scheme that I wasn't aware of, but now that you point this out I think it's fine to not handle this case gracefully.
Sure, that sounds good. FWIW, the code before this PR also classified "external navigation" as navigating to a link with a different host name (scheme was ignored, for example, so
That seems valid to me. I initially thought it might be undesirable to provide a non-404 response for non-navigation requests that also don't match a static asset, but it does seem like that would actually work. Thanks so much for the feedback, @SteveSandersonMS! |
Coming back to my comment here:
I'm starting to think this might actually be problematic in some cases. While implementing this, I noticed that if, for example, an internal JavaScript file could not be located, you get these strange syntax errors in the console basically complaining that HTML tags are not valid JavaScript, etc. But also, if it's something like @SteveSandersonMS Do you think these problems are enough to hold off on the navigation fallback behavior changes? |
cc/ @guardrex we'll have to update https://docs.microsoft.com/en-us/aspnet/core/blazor/hybrid/routing?view=aspnetcore-6.0 based on this new API (after it's finalized/merged). @MackinnonBuck gave a great summary of the changes in this comment. |
On the web, by default we don't do fallback for file-like URLs, so that However the limitation with doing the same for MAUI is that, for MAUI, there's no way to customize the routing AFAIK, so there would be no way to have the fallback (and hence deep linking) for |
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.
Approving assuming this now reflects the final design that you worked out with @javiercn. Thanks for working through all the details on this one!
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.
LGTM, just a doc change needed 😄
Oh no! Sorry about that @TanayParikh! |
Description of Change
In Blazor MAUI for Android, clicking a link with a scheme other than HTTP or HTTPS would previously sometimes crash the app or display a "Webpage not available" page. This PR resolves the undesirable behavior by making the following changes:
intent
scheme, etc.).If a link uses theintent
scheme, but the intent could not be resolved (or the app could not be started) and abrowser_fallback_url
is specified, attempt to start an activity from an intent created by the fallback URL.Issues Fixed
Fixes #4897