[webview_flutter] - Added WebViewSetting option for iOS #4339
Conversation
github-actions
bot
added
p: webview_flutter
|
Thanks for the submission! As explained in the contributing guide, all behavioral changes need tests. Please let us know when you’ve updated the PR with tests and we can take a look. |
|
@GauteHaugen Are you planning on updating this PR to add tests? |
|
@stuartmorgan, sorry about that. It has been a lot of work lately, so I forgot about it. Will add tests over the weekend |
|
@stuartmorgan I have now pushed some tests. I noticed that there had also been some changes to the folder structure as well as the package being split into 4 since the initial PR. I have pulled down the latest version, and have added all changes again with tests. I also updated the version for each package. so at the moment, some workflows fail due to the packages not being released. Should I move this PR into 4 different PRs, one for each package? |
There was a problem hiding this comment.
It'll need to be split into three PRs; see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins
But there's current a large rewrite happening in the Android implementation that will eliminate the need for the Android change here, so I would suggest starting with splitting out just the platform interface PR; once that's reviewed and landed, hopefully the Android change will have landed, and then you can split out just the iOS change.
(I left a few initial comments; it wasn't a full review, but generally the overall change looks good to proceed with splitting it.)
| ``` | ||
| * `Array` of App-Bound Domains is limited to 10 domains | ||
| * Content supplied by the app through local files, data URLs and HTML String are always treated | ||
| as App-Bound Domains, and do not need to be listed. |
There was a problem hiding this comment.
Please link to authoritative documentation of the OS feature rather than duplicating it here (where it could easily become outdated).
| /// ``` | ||
| /// * `Array` of App-Bound Domains is limited to 10 domains | ||
| /// * Content supplied by the app through local files, data URLs and HTML String are always treated | ||
| /// as App-Bound Domains, and do not need to be listed. |
There was a problem hiding this comment.
Same here; just link to the documentation.
| /// ``` | ||
| /// * `Array` of App-Bound Domains is limited to 10 domains | ||
| /// * Content supplied by the app through local files, data URLs and HTML String are always treated | ||
| /// as App-Bound Domains, and do not need to be listed. |
There was a problem hiding this comment.
You don't need to re-document the feature in the example. If people want to know what the feature does, they should look at the API documentation.
| @@ -1,3 +1,7 @@ | |||
| ## 2.1.3 | |||
|
|
|||
| * Added `limitsNavigationsToAppBoundDomains` to `WebSettings` ([flutter/issues/89530](https://github.com/flutter/flutter/issues/89530)) | |||
There was a problem hiding this comment.
We don't generally reference issues in CHANGELOGs
| NSNumber* limitsNavigationsToAppBoundDomains = settings[key]; | ||
| _webView.configuration.limitsNavigationsToAppBoundDomains = | ||
| [limitsNavigationsToAppBoundDomains boolValue]; | ||
| } |
There was a problem hiding this comment.
There's no test coverage of this part of the change; if this were accidentally removed in the future, the feature would stop working but no tests would fail. The best option here would be a native unit test that sends a message with this enabled, and ensures (probably via an exposed-for-testing accessor to give you the _webView) that the configuration is set correctly.
There was a problem hiding this comment.
I understand why this needs a unit test, but I'm not sure how to implement testing for it. I tried looking at the other properties, and to me, it looks like none of the properties would be caught in a test if native code were altered. So I'm not entirely sure how to proceed with the unit test.
Some of the questions I have regarding the native unit test.
- Should it be in the flutter_webview_wkwebview package or the flutter_webview package?
- Should it be all swift code, or is there some way to test from flutter with flutter code sending the message over the message channel?
- Also if the test is added to the ios folder, is there anything special I need to do to make it work with the GitHub workflows/checks?
There was a problem hiding this comment.
it looks like none of the properties would be caught in a test if native code were altered.
That's true, but: https://github.com/flutter/flutter/wiki/Plugin-Tests#faq-do-i-need-to-add-tests-even-if-the-part-of-the-code-im-changing-isnt-already-tested
- Should it be in the flutter_webview_wkwebview package or the flutter_webview package?
The former. Unit tests should live in the same package as the code they are testing.
- Should it be all swift code, or is there some way to test from flutter with flutter code sending the message over the message channel?
It should be Objective-C; testing from Dart would require an integration test, which would be much more involved for this case.
- Also if the test is added to the ios folder, is there anything special I need to do to make it work with the GitHub workflows/checks?
Native unit tests are already configured for the package, you just need to add a test to those existing test files (or if you make a new file, add it to the same target).
|
@stuartmorgan, I have now followed the steps in the "Changing federated packages" guide. The first PR #4464 is up with the changes for flutter_webview_platform_interface. I have also as the guide explained changed this PR to include dependency_overrides until the other PRs have been merged |
limitsNavigationsToAppBoundDomains will now be set when the WebView is initialized
…ns is sent correctly
|
@stuartmorgan, while writing the iOS native unit test. I found out that |
| @@ -94,6 +96,7 @@ class WebView extends StatefulWidget { | |||
| this.initialMediaPlaybackPolicy = | |||
| AutoMediaPlaybackPolicy.require_user_action_for_all_media_types, | |||
| this.allowsInlineMediaPlayback = false, | |||
| this.limitsNavigationsToAppBoundDomains = false, | |||
There was a problem hiding this comment.
Adding platform-specific arguments to this list is not a direction I want to take if we can possibly avoid it; it could get out of hand very quickly.
@bparrishMines @mvanbeusekom You're both more familiar than I am with the overall API of this plugin; do either of you have thoughts on how to introduce platform-specific extensions here?
I'm wondering if it would make sense to add a creation parameters class that's created by a factory, implemented by the implementing packages, so that they could return subclasses of the settings object that have extensions. Then clients could do platform-specific casting to access settings like this one, similar to the IAP extensions.
There was a problem hiding this comment.
In the long term, I believe that yes we could use casting to add cleaner support for very specific platform features like this. For example, the API would look something like:
class MyWidget extends StatelessElement {
MyWidget(StatelessWidget widget) : super(widget);
Future<void> additionalIosSetup(IosWebViewController controller) {
controller.configuration.setLimitsNavigationsToAppBoundDomains(false);
}
Future<void> additionalAndroidSetup(AndroidWebViewController controller) {
...
}
@override
Widget build() {
return WebView(
onWebViewCreated: (WebViewController controller) {
if (Platform.isIOS) {
additionalIosSetup(controller as IosWebViewController);
} else if (Platform.isAndroid) {
additionalAndroidSetup(controller as AndroidWebViewController);
}
},
);
}
}However, this can only be done after the iOS platform implementation is transitioned to pigeon and we update the platform interface. After #4503 lands, this should be possible for Android. So we may want to hold off on adding this feature until iOS is also transitioned.
There was a problem hiding this comment.
Not sure if I'm missing something with your example @bparrishMines, but limitsNavigationsToAppBoundDomains is not a setting that can be changed after the WebView has been initialized. It needs to be configured during the initialize process of the FLTWebViewController object. And if I understand it correctly onWebViewCreated is called after both the WebView Configuration and the WebView is done initializing
There was a problem hiding this comment.
Ah, I hadn't realized it was part of initialization. Then, I agree with @stuartmorgan that we could expose a CreationParams class that each platform can add additional fields too. A solution that could look something like this in the platform interface:
class CreationParams {
factory CreationParams() {
return WebView.platform.createCreationParams();
}
}
abstract class WebViewPlatform {
.
.
.
void createCreationParams() {
throw UnimplementedError();
}
}@stuartmorgan Is this what you were considering?
There was a problem hiding this comment.
That looks pretty much like I was thinking (except that we'd probably want the platform interface class to return the base class rather than throw unimplemented, so that only platforms that want extra parameters need to implement it).
There was a problem hiding this comment.
@stuartmorgan is this something I should implement, or should I wait for it to be implemented before adding my changes? Also, how would this look in an app where I might target multiple platforms and want toggle options for multiple platforms?
There was a problem hiding this comment.
It's not something there's an existing plan to implement, so you should go ahead and implement it. I would start with trying it as part of this PR; we could do it as a separate PR that lands first, but it's probably useful to build (and review) in conjunction with this use as a test case to make sure it's working as we want it to end to end. (If it turns out to be more complex than anticipated for some reason, it could be split out later.)
Also, how would this look in an app where I might target multiple platforms and want toggle options for multiple platforms?
Basically like the example shown above, but with the settings object being cast, not the controller.
There was a problem hiding this comment.
so you should go ahead and implement it
Oh, but as @bparrishMines mentioned, it should wait a couple of weeks since the restructuring that's happening in the webview implementation will probably interact with this.
There was a problem hiding this comment.
@stuartmorgan Sounds good, is there an active issue or PR for the iOS transition to pigeon that I can follow to know when this change is landed?
There was a problem hiding this comment.
There wasn't, but really should be. I filed flutter/flutter#93732 (Android is in progress now and nearing completion, iOS will follow after).
|
Status update from triage: The iOS conversion to Dart is still in progress, so this is still on hold. |
|
跳转其他url无法触发onPageFinished |
|
flutter/flutter#93732 is complete, so this is unblocked; you'll just need to update the implementation for the new Dart-based implementation. |
|
@GauteHaugen Are you still planning on updating this to the new implementation? |
|
@stuartmorgan I can take a look at it over the weekend. Is the plan to still implement it as discussed in the comments above? |
|
I believe so. @bparrishMines Is the thread above still the model for the v4 structure? |
|
I think this feature is still blocked because it requires the feature to set a platform specific |
That was going to be my next question. When I was looking at the code, I could not find an easy way to override CreationsParams. Then I will wait for flutter/flutter#94051 to land before implementing |
|
Sorry about that; I forgot that platform-specific creation params were not supported before, and conflated it with just changing |
|
Any update on this issue since September? Really would love this option as PWAs / Service Workers cannot work inside of a flutter WebView it seems. Looking at v4 of flutter WebView seems like I should be able to set it via WebKitWebViewControllerCreationParams and seems to be what is being referred to in the last comment. Also the referenced issue that that was being waited on is closed. |
|
webview_flutter v4 has finally been completed, which included fixing the platform interface to that we can make additions to it without breaking changes. That means that this is finally unblocked! Thanks for your patience while we worked through the restructuring. webview_flutter v4 uses a Dart-first implementation, where the platform channel boundary is as close as possible to the underlying host APIs, and plugin logic is written primarily in Dart rather than the host language. That means that this PR will need to be updated to follow that model. Please let us know if you have questions about updating to v4 (and #6881 may be useful as an example of adding a feature in the v4 structure). |
|
I'm going to go ahead and mark this as a draft since it needs to be reworked for v4, just to get it off of our review queue. Please mark it as ready for review once that's been done. Thanks! |
|
We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages. Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord. Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state! |
Added limitsNavigationsToAppBoundDomains that is an optional setting for iOS
Added
limitsNavigationsToAppBoundDomainsproperty toWebViewSettingsthat can be used on iOS. This will give the developer access to some JavaScript APIs that are disabled by default.List which issues are fixed by this PR. You must list at least one issue.
(flutter/issues/89530)
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.