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

feat: Add option for custom error page #5723

Merged
merged 25 commits into from
Jul 14, 2022
Merged

Conversation

theproducer
Copy link
Contributor

@theproducer theproducer commented Jun 27, 2022

This PR adds an option for displaying a local, custom error page in the case of an outdated web view (on Android), or in cases of web view errors.

closes: #4884

@theproducer theproducer changed the base branch from main to capacitor-4 June 27, 2022 16:58
@theproducer theproducer marked this pull request as ready for review June 27, 2022 17:52
@IT-MikeS IT-MikeS changed the base branch from capacitor-4 to main June 27, 2022 20:20
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Usually when we have a config option that can be configured for ios as ios.something and for android as android.something we also have a common something config option on the root so it can be configured for both platforms on a single place.

Also, not sure if it would make more sense to have the config option inside the server object since the error path is related to the WebView navigation (I mean, instead of having three errorPath, ios.errorPath and android.errorPath, to have only server.errorPath.

@theproducer
Copy link
Contributor Author

Usually when we have a config option that can be configured for ios as ios.something and for android as android.something we also have a common something config option on the root so it can be configured for both platforms on a single place.

Also, not sure if it would make more sense to have the config option inside the server object since the error path is related to the WebView navigation (I mean, instead of having three errorPath, ios.errorPath and android.errorPath, to have only server.errorPath.

Do you think there would be any benefit to allowing the error path to be set independently per platform? If not, I agree with having one option in server.errorPath.

@jcesarmobile
Copy link
Member

Not sure, but now that we allow .ts config files people can still have different values if they need to, so not sure if it's worth having both options.

@theproducer theproducer requested a review from jcesarmobile June 30, 2022 15:22
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

How should the errorPath look like?
On Android I had to use it like "errorPath": "/error.html", while on iOS I had to use it like "errorPath": "error.html" (without the /).
I think it should be like on iOS (without the /).

If using a server.url and there is no internet connection, it will try reload form the server.url, but appending the error page. i.e. if server.url is https://nonexistent.com and errorPath is error.html, it will try to reload https://nonexistent.com/error.html, which will continue to fail because there is no internet. It should try to load from the local url, in example capacitor://localhost/error.html on iOS or http://localhost/error.html on Android. (there seem to be a problem on iOS that prevents the capacitor://localhost/error.html url at the moment, we should probably fix that first.

Also, Capacitor's javascript should not be injected on the error page, since the injected javascript is not compatible with WebView in version 55 or older. Should be handled on the WebViewLocalServer file to exclude the error path.

@theproducer theproducer requested a review from jcesarmobile July 5, 2022 17:17
@theproducer theproducer requested a review from jcesarmobile July 11, 2022 18:37
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Line 175 of WebViewLocalServer.java should also be changed to check if it is the error url, otherwise when using server.url, if the url fails to load it won't be able to load the local assets. Should be something like:
if (isLocalFile(loadingUrl) || isMainUrl(loadingUrl) || !isAllowedUrl(loadingUrl) || isErrorUrl(loadingUrl)) {

Also, error page redirect code is also needed in onReceivedHttpError for Android in case it's using server.url and the url trying to load returns 404 or similar error, in that case it goes to onReceivedHttpError instead of onReceivedError

if (appUrl.charAt(appUrl.length() - 1) != '/') {
appUrl += "/";
}
if (errorPath != null && request.getUrl().toString().equals(appUrl)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this compare if the url to load is different from the appUrl? on iOS it's not comparing that.
If it's because when loading the error page the app enters into a loop of failures because of favicon.ico not loading, I think it's best to check if the request was to the main frame
if (errorPath != null && request.isForMainFrame()) {.

Copy link
Contributor Author

@theproducer theproducer Jul 13, 2022

Choose a reason for hiding this comment

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

If it's because when loading the error page the app enters into a loop of failures..

Yes, this was exactly why. Thanks!

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

looks good

@justinpathrose
Copy link

justinpathrose commented Jan 18, 2024

@theproducer We are trying to implement this in our app. I get that error page is shown based on minWebViewVersion in android, but can't figure out how this works on iOS. Is there a config we can use to say the minimum supported iOS version?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Minimum WebView version on Android
5 participants