-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
CB-7179 (iOS): Add WKWebView support for iOS #271
Conversation
Travis CI build is failing because, as noted above, the |
@infil00p @stevengill @agrieve @shazron @purplecabbage Could you guys review this when you get a chance? |
Sorry for the late review. Could you resolve the conflicts before review? Thanks! |
8e02526
to
a3cc42d
Compare
@shazron I've resolved the merge conflicts and rebased to a single commit. |
Yes, it uses the CDVWKProcessPoolFactory class from cordova-plugin-wkwebview-engine in order to share cookies between the Cordova app Webview and the IAB Webview so setting a cookie in one makes it available in the other. This is currently enabled by default, but could be made optional.
Since the master branch is being continually updated, this PR branch will undoubtedly get out of sync with it. But there's no point in me continually resolving merge conflicts if this PR is never going to be reviewed/merged. @shazron If This would eliminate the need for the |
Just an aside: |
Hi, any news when this PR will be merged and released into a new version? |
As soon as 1) @dpa99c had the time to fix the conflicts, 2) someone or multiple ones from Apache Cordova (or the community, like you @rafaelberrocalj!) had time to test and review (hopefully with Approval), 3) then someone from Apache Cordova who takes the responsibility of merging the PR and finally 4) someone from Apache Cordova creates a new release of the plugin. |
I have done my part and resolved the existing merge conflicts. |
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.
Non-Obj-C parts of the PR look good. Left 2 comments.
README.md
Outdated
@@ -136,9 +136,10 @@ instance, or the system browser. | |||
|
|||
iOS supports these additional options: | |||
|
|||
- __usewkwebview__: set to `yes` to use WKWebView engine for the InappBrowser. Omit or set to `no` (default) to use UIWebView. Note: this requires that a WKWebView engine plugin be installed in the Cordova project (e.g. [cordova-plugin-wkwebview-engine](https://github.com/apache/cordova-plugin-wkwebview-engine) or [cordova-plugin-ionic-webview](https://github.com/ionic-team/cordova-plugin-ionic-webview)). |
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.
What will happen if no WKWebView plugin is installed?
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.
If usewkwebview=yes
and no WKWebView plugin is installed, the loaderror
callback will be invoked with the error message "usewkwebview option specified but cordova-plugin-wkwebview-engine not present"
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.
Ah, got it.
2 suggestions:
- Change "this" in the note to "Using
usewkwebview=yes
requires ..." - Change the
loaderror
callback to not refer to the specific plugin, but just mention "... but no plugin that supplies a WKWebView engine present" (or similar)
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.
OK, I've made tweaks based on those suggestions
@@ -76,11 +76,31 @@ | |||
<config-file target="config.xml" parent="/*"> | |||
<feature name="InAppBrowser"> | |||
<param name="ios-package" value="CDVInAppBrowser" /> | |||
<param name="onload" value="true" /> |
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.
What does this onload
param do? Couldn't find it anywhere else.
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.
onload
specifies whether to initialize the plugin when the app controller initializes. There's some docs about it here.
It ensures that pluginInitialize()
is called on app startup, rather than the first plugin invocation.
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 learned something, thanks ;)
Why this change? Could this have any side effects?
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.
It just ensures that the default values defined by pluginInitialize()
are set right away at app startup.
I don't remember the exact reason it was needed (it's been a few months since I created this PR) but I'm pretty sure it was necessary.
The change is minimal and will not cause side effects.
Travis is failing with:
|
(Oh great, I think that is from another PR.) |
@dpa99c Unfortunately the build issue remains, do you have the ability to fix it? It might be a merge issue that removed something, not sure what. I tested this locally by installing the plugin from your branch and the error is still there. |
@shazron you were right - I missed a line from that PR when resolving the merge conflict - I've updated this PR branch to correct this and Travis CI build is no longer failing because of this. However, the Travis job is still failing because the tests are building/running against the iOS 9.3 SDK, however the new WKWebView implementation uses some properties of the WKWebViewConfiguration class which were only introduced in the iOS 10 SDK, such as ignoresViewportScaleLimits. Is building against iOS 9 SDK still a mandatory requirement for |
We should update it to test only against the iOS 10 SDK, and make a note of it in the release notes, and also bump a minor version at least. |
Hi, can i test this pull request somehow in config.xml? |
You can install the plugin from the branch which this pull request is based on via:
|
With this changes i still need to use https://github.com/apache/cordova-plugin-wkwebview-engine ? |
@rafaelberrocalj yes, if you want to use the WKWebView implementation, you'll need the WKWebView engine plugin |
Ok thanks, so the iOS 9.3 test I readded should probably fail. But currently both are failing:
*sigh (Investigating) |
And as it came it's gone - now the tests are just failing because they don't get their results from Saucelabs :/ |
Confirmed that the switch to Xcode8.3 is not the cause for the test failures: |
OK, there must be something preventing the tests running via |
@janpio I think I've figured out why the tests are failing in this PR, even in Xcode 8.3/iOS 10: This PR requires that I confirmed this by running Then with As you can see, in this second case the tests timed out while trying to connect to the local server, leading to the same error observed in the Travis CI logs. However, as you can see from the Simulator screenshot, all the tests passed. I then ran the As you can see, a similar result: all tests passed in the Simulator, but tests failed due to a timeout connecting to the local server. The upshot is that it seems currently I'm guessing since UIWebView is now deprecated in iOS 12, we'll be moving at some point to make WKWebView the default Cordova Webview (in |
… the WKWebView implementation
Thanks to @knight9999 for helping to resolve apache/cordova-paramedic#52, Travis tests are now passing for iOS. But failing on other platforms for some reason. |
@shazron @janpio What's now blocking this PR? All merge conflicts with master are currently resolved and the Travis CI tests are now passing for iOS. The Travis tests for "browser-edge" and "android-6.0" failed on the last run due to timeout/connectivity issues, but then the tests are currently failing on |
<platform name="ios"> | ||
<!-- The WKWebView implementation for inappbrowser requires the presence of this plugin --> | ||
<dependency id="cordova-plugin-wkwebview-engine" /> | ||
<dependency id="cordova-plugin-wkwebview-engine-allowfileaccess" url="https://github.com/knight9999/cordova-plugin-wkwebview-engine-allowfileaccess.git" /> |
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.
We can’t have this dependency, even in a test ... what if knight9999 makes a commit to master? It could break our tests ...
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.
Without the code in that plugin, cordova-paramedic
tests fail with WKWebView.
What do you suggest? Bundle the plugin code inside tests/
within this plugin?
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 plugin itself is only 2 lines. There are a couple things we could do.
- Ask @knight9999 to publish the allowfileaccess plugin to npm, and have our test depend on that published version.
- make the wk-allowfileaccess a cordova supported plugin, and also do suggestion 1, but replace Ken with 'we'
- revisit the decision to not allow file access directly in the wkwebview plugin, could we just default to NO, but allow the developer to turn it on? ( This would require a firm warning about app store rejection possibilities, so it might make things too easy for developers and lead to rejections. )
- Include it in the paramedic repo as a spec plugin, as suggested by Ken [1]
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.
@knight9999 Ken, any preference in Jesse's above options?
This isn't something I can resolve myself to unblock this PR: all of the above require action on the part of Cordova maintainers and it has wider implications in terms of test support for WKWebView on iOS, i.e. for cordova-plugin-wkwebview-engine
and cordova-ios@5
.
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.
Now I have updated my plugin to change author 'Apache Cordovaand license
Apache-2.0`.
But I think it is better to change github repository before publishing to npm. Now I am asking this to cordova members.
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 have published my plugin https://www.npmjs.com/package/cordova-plugin-wkwebview-engine-allowfileaccess .
This PR is now available MaKleSoft/cordova-plugin-migrate-localstorage#4 |
Every other way of doing this is messy and involves crazy git rebasing, so I am gonna just merge this as is, and see if I can address my concern about the test dependency in another pr. |
@purplecabbage great, thanks for unblocking this. I will be on standby to help resolve any issues with the new implementation. |
Hi, I submitted an issue for this at The testing projects are shared at Any idea for this issue? Thanks |
I created a simple iOS UIWebView cordova app with just inappbrowser plugin added into the project, and it can repeat the same issue. The project (basictest.zip) is shared at the folder of The project (basictest.zip) only has a simple openInappbrowser button, once index.html is opened, then clicking toolbar's done button to close the inappbrowser. After the inappbrowser is closed. Clicking on the main UIWebView's openInappbrowser button again will do nothing, it seems the main UIWebView is disabled. This indicates the issue is in the new inappbrowser plugin's implementation. Thanks |
Platforms affected
iOS
What does this PR do?
This PR adds support for WKWebView to the InappBrowser plugin on iOS.
It:
Additional changes in this PR include:
usewkwebview
option.Any feedback/suggestions/bug reports/collaboration to improve the implementation in this PR would be most welcome.
What testing has been done on this change?
The automated and manual tests have been run as outlined here and all tests appear to pass for the iOS platform against both webview implementations.
Note: other platforms have not been tested because they have not been changed.
Notes
usewkwebview=yes
is set and the WKWebView engine classes don't exist in the project, the 'loaderror' callback will be invoked when attempting to callInappBrowser.open()
.usewkwebview=yes
will result in aloaderror
.cordova-plugin-inappbrowser
sousewkwebview=yes
will work with both.clearsessioncache
is yet to be implemented in the WKWebView implementation. This is because the httpCookieStore interface was only added in iOS 11 and so to support iOS 10 and below, a different mechanism must be used to find and remove session cookies. This is not trivial and may not even be possible.Checklist