-
Notifications
You must be signed in to change notification settings - Fork 362
Fixes CB-11074: WKWebView configuration is not considered #7
Conversation
Any feedback ? |
It still needs to be able to update it dynamically. I would refactor it to not remove the existing code, but create a new That way it can be used in both updateSettings and initWithFrame |
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
So that approach wouldn't work as is. The only thing I can image that would work in the current |
@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: cordova-plugin-wkwebview-engine/src/ios/CDVWKWebViewEngine.m Lines 47 to 49 in 33a7517
I'll comment on your #8 pull request. |
A drive-by comment: |
It seems that this bug prevents autoplay in I needed to switch back to UiWebview in my meteor application because of this. See also meteor/meteor#7173 |
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 |
Any update on this? In my opinion, this is a very severe issue with WKWebview. |
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. |
@shazron : cool, thanks. Is there any short-time workaround known? |
Please see #13, and let's continue discussion there. |
Please provide feedback on #13, I will be pulling that in soon. |
I 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 !