Details
-
Bug
-
Status: Closed
-
Critical
-
Resolution: Fixed
-
None
-
None
-
ios
-
Important
Description
Regarding the code of WKWebView.h, WKWebViewConfiguration seems to be a readonly property.
@property (nonatomic, readonly, copy) WKWebViewConfiguration *configuration;
But in this plugin, we are trying to update the WKWebView configuration after initialising it and it looks like all this part is not considered :
https://github.com/apache/cordova-plugin-wkwebview-engine/blob/master/src/ios/CDVWKWebViewEngine.m#L146-L150
Attachments
Issue Links
Activity
Github user akofman commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-211276741
Any feedback ?
Github user shazron commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-211493781
It still needs to be able to update it dynamically. I would refactor it to not remove the existing code, but create a new `- (WKWebViewConfiguration*) createConfigurationFromSettings:(NSDictionary*)settings;`
That way it can be used in both updateSettings and initWithFrame
Github user ephemer commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-215453411
As I commented in the base repo, this PR doesn't do what it says it does yet.
@shazron is it normal to change the settings after the WebView has already been inited? When? It seems like this feature (of changing settings after init) is creating unnecessary work for us.
My preferred approach would be to init the CDVPlugin/WebViewEngine instance, but not the WKWebView immediately, because for that to work, we need the commandDelegate to be set. The problem with that approach is that this check in `CDVViewController.m` will fail, causing `self.webViewEngine` to fallback to UIWebView:
if (!self.webViewEngine || ![self.webViewEngine conformsToProtocol:@protocol(CDVWebViewEngineProtocol)] || ![self.webViewEngine canLoadRequest:[NSURLRequest requestWithURL:self.appUrl]])
{ self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds]; }The only thing I can image that would work in the current `CDVViewController.m` structure is that we init a WKWebView with default options on `init`, then re-init the WKWebView in `pluginInitialize` with the user's actual settings, because it's only then that `commandDelegate.settings` is finally available.
GitHub user ephemer opened a pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8
CB-11074: Ensure settings from config.xml are taken into consideration
#7 also addresses this issue but in my tests didn't work.
This pull request presents a workaround that requires two WKWebView instances to be inited (only one is kept and used for the app though). This isn't supposed to be merged as is, it's more a proof of concept and designed to provide exposure to the workaround if others users need this fix urgently.
@shazron the logic presented in this PR would have been made more complicated via the updateWithInfo: method, which as far as I could see is unused, so I removed it for now. Other than that, what do you think about this as general direction.
As I commented in #7, the best approach for this would be to change `CDVViewController.m` to pass in the settings directly on init, or to rearrange the check logic there. That is a longer-term solution. In the meantime, maybe we can get by with the approach presented here.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ephemer/cordova-plugin-wkwebview-engine master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #8
commit b87ad79a12af05c9a14559dd3f1d8e658493c27c
Author: Geordie J <geojay@gmail.com>
Date: 2016-04-28T16:39:17Z
CB-11074: Ensure settings from config.xml are taken into consideration
Github user shazron commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-215584737
@ephemer not just settings (which as you said will probably not a frequent scenario), but setting the other delegates. updateWithInfo will be used for certain apps that need to set their own delegates on the webViewEngines – a minor case, but I encountered this just last week.
I don't think self.webViewEngine will fail in the case of your 'preferred approach' you mentioned since that is just the CDVPlugin object wrapper that will always be instantiated unless WKWebView is not available: https://github.com/apache/cordova-plugin-wkwebview-engine/blob/33a75172a1450e8922788cd23382cc6ec33845c7/src/ios/CDVWKWebViewEngine.m#L47-L49
I'll comment on your #8 pull request.
Github user shazron commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8#issuecomment-215585938
I also commented on #7.
To solve this problem, instead of having to instantiate WKWebView twice here, I think we can safely move all initializing code from the initWithFrame, to pluginInitialize:
https://github.com/apache/cordova-plugin-wkwebview-engine/blob/33a75172a1450e8922788cd23382cc6ec33845c7/src/ios/CDVWKWebViewEngine.m#L50-L64 – all we need to do is save the CGRect for the frame for later. At this point, with this approach, the WKWebView will have been instantiated for this return call: https://github.com/apache/cordova-ios/blob/e51eda0f3b1e68d210182c3bbd3d9022c1ffe8ec/CordovaLib/Classes/Public/CDVViewController.m#L426
As I mentioned in #7, we still need updateWithInfo.
The rest of your approach is sound.
Github user ephemer commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8#issuecomment-215702371
@shazron in that case we'd have to remove the configuration code from updateWithInfo because we've already established that it doesn't do anything anyway. It'd be very frustrating for a new developer to see that code and wonder why it does nothing at all. As far as I can see it will never work, wkWebView.configuration is listed as `read-only` in Apple's docs. I'd also rename `updateWithInfo` to `updateDelegates` or similar unless it breaks something else.
If we just inited the WKWebView once in `pluginInitialize`, I'm pretty certain the `CDVViewController.m` would fail at the `canLoadRequest:` check and then proceed to replace the `WKWebViewEngine` with `defaultWebViewEngineClass`, as commented in #7:
if ( (...) || ![self.webViewEngine canLoadRequest:[NSURLRequest requestWithURL:self.appUrl]])
{ self.webViewEngine = [[NSClassFromString(defaultWebViewEngineClass) alloc] initWithFrame:bounds]; }Specifically this is due to the following code in `CDVWKWebViewEngine`: https://github.com/apache/cordova-plugin-wkwebview-engine/blob/33a75172a1450e8922788cd23382cc6ec33845c7/src/ios/CDVWKWebViewEngine.m#L136
`_engineWebView` would still be nil here (because `pluginInitialize` wouldn't have been called), which would as far as I understand (I'm Swift-first, ObjC when necessary) be coerced to `NO`, making the above check fail.
As far as I can see, the only clean solution to this would be to pass `settings` into the WebViewEngine's `init` method. That is probably also the most logical and explicit way about getting the settings from `config.xml` into the WebView.
Github user kenichi-fukushima commented on the pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7#issuecomment-216118441
A drive-by comment:
In our app, we ended up subclassing CDVWKWebViewEngine and creating another instance of WKWebView in -pluginInizialize so that we can use configuration values that we want. I'd suggest factoring out the code to create WKWebViewConfiguration to another method so that Cordova users can customize the logic by code rather than static configurations in a file. This means you can't create a web view instance in the initializer any more. This is OK because CDVWKWebViewEngine doesn't have to conform to the usual plugin semantics and you can add a factory method like -webView.
Github user macrozone commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
It seems that this bug prevents autoplay in <video> and <audio> elements on IOS, because the config "MediaPlaybackRequiresUserAction" is never passed to the WkWebview instance and therefore always true (default in the code).
I needed to switch back to UiWebview in my meteor application because of this.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8
I'm creating failing unit tests for this first, I think we have passed the point where we can argue about the implementation, but we need objective verification: https://issues.apache.org/jira/browse/CB-11496
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
I'm creating failing unit tests for this first, I think we have passed the point where we can argue about the implementation, but we need objective verification: https://issues.apache.org/jira/browse/CB-11496
Github user macrozone commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Any update on this? In my opinion, this is a very severe issue with WKWebview.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
I have failing unit tests that do objectively verify that as others have shown, yes, we can't update the configuration dynamically (I've tried all sorts of clever runtime tricks, to no avail).
The WKWebViewConfiguration.preferences (WKPreferences) can however be updated dynamically. I will update https://issues.apache.org/jira/browse/CB-11496 with the unit tests to reflect this, as well as document this. After doing so, I will re-review this pull request with that in mind.
Github user macrozone commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
@shazron : cool, thanks. Is there any short-time workaround known?
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8
I commented on #7, updateWithInfo needs to be changed to not allow updating the WKWebViewConfiguration, because you can't update it at runtime – I objectively verified this in the updated unit tests.
GitHub user shazron opened a pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/13
CB-11074 - Ensure settings from config.xml are taken into consideration
Take note of the disabled "testConfigurationWithMediaPlaybackAllowsAirPlay" unit test which demonstrates that the allowsAirPlayForMediaPlayback is not sticky (apparent Apple bug).
Also, "mediaPlaybackRequiresUserAction" configuration default differs on iOS 10 versus earlier versions (on iOS 10 it defaults to YES, when it was NO, previously). Probably due to: https://webkit.org/blog/6784/new-video-policies-for-ios/
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shazron/cordova-plugin-wkwebview-engine CB-11074
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/13.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #13
commit ec4350ab9b45b4e45413e30337d7a6e9e5bd5e22
Author: Shazron Abdullah <shazron@apache.org>
Date: 2016-08-04T22:47:59Z
CB-11074 - Ensure settings from config.xml are taken into consideration
Note that there is a skipped unit test "testConfigurationWithMediaPlaybackAllowsAirPlay", which deals with an Apple bug in setting the WKWebViewConfiguration.allowsAirPlayForMediaPlayback value.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8
Please see #13, and let's continue discussion there.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Please see #13, and let's continue discussion there.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Please provide feedback on #13, I will be pulling that in soon.
Github user shazron commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8
Please provide feedback on #13, I will be pulling that in soon.
Github user ephemer commented on the issue:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/13
Without having tested this, it seems reasonable to me
Commit 42c847b2680a80e52ffd3617119ddd6634751453 in cordova-plugin-wkwebview-engine's branch refs/heads/master from shazron
[ https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-wkwebview-engine.git;h=42c847b ]
CB-11074 - Ensure settings from config.xml are taken into consideration
Note that there is a skipped unit test "testConfigurationWithMediaPlaybackAllowsAirPlay", which deals with an Apple bug in setting the WKWebViewConfiguration.allowsAirPlayForMediaPlayback value.
This closes #13, closes #7, closes #8
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/13
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/8
GitHub user akofman opened a pull request:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7
Fixes
CB-11074: WKWebView configuration is not consideredI updated the configuration before the WKWebView initialisation in order to consider it.
It implies that the updateWithInfo method doesn't update it anymore. I'm not sure it is really a problem but I'd prefer @shazron to confirm.
Thanks for the reviews !
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/akofman/cordova-plugin-wkwebview-engine master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-wkwebview-engine/pull/7.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #7
commit 18219a0fe66208cc3ff1a794908510194cf86668
Author: Alexis Kofman <alexis.kofman@gmail.com>
Date: 2016-04-13T09:46:45Z
Fixes
CB-11074: WKWebView configuration is not considered