[webview_flutter] Implement zoom enabled for ios and android #4417

NickalasB · 2021-10-06T13:32:01Z

This PR adds the iOS and Android implementations of the soon-to-be added zoomEnabled param on the WebView widget after the platform_interface method was merged in #4404.

If approved and merged, this PR will be followed up with #3325, adding zoomEnabled to the main flutter_webview package as outlined in the instructions on changing Federated Plugins.

List which issues are fixed by this PR. You must list at least one issue.
Implements flutter/flutter#70731
Implements flutter/flutter#48283
Relates to flutter/flutter#48245
Relates to flutter/flutter#60921

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

No this is not a breaking change

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

* master: [webview_flutter] Update webview platform interface with new methods for running JavaScript. (flutter#4401)

NickalasB · 2021-10-06T13:34:09Z

@stuartmorgan @bparrishMines here's the 2nd PR for this plugin update. Thank you for taking a look!

bparrishMines · 2021-10-06T23:08:45Z

packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FLTWKNavigationDelegate.m

@@ -63,6 +63,17 @@ - (void)webView:(WKWebView *)webView
 }

 - (void)webView:(WKWebView *)webView didFinishNavigation:(WKNavigation *)navigation {
+  if (!self.shouldEnableZoom) {


What would happen if shouldEnableZoom was changed without the page reloading? Shouldn't this also be called for the current webpage along with every call to didFinishNavigation.

That makes sense but I'm not totally sure where you are suggesting I should also make this check? Inside decidePolicyForNavigationAction?

This workaround actually makes me question if this feature should even be supported by iOS directly. I'm leaning towards that a platform shouldn't support a feature from the platform interface unless it is directly supported by the API or there is a reasonable workaround. And I wouldn't consider running java script as a reasonable workaround.

@stuartmorgan Do we have a policy for this already?

cc @mvanbeusekom

Yeah, the WKWebView API doesn't provide a built-in way of toggling whether or not zoom is enabled. This workaround was intended to at least allow Flutter devs to toggle that on both Android and iOS. The original intention of this work was strictly to ENABLE zoom for Android as that functionality is currently missing. My background is in Android but in my research, I couldn't find a cleaner way of DISABLING zoom for iOS.

One other proposal, although it would require reverting #4404, is to just enable zooming for Android, and not make it togglable in the Flutter WebView. That would basically only require code-changes to webview_flutter/webview_flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewBuilder.java. Could just be me but it doesn't seem like there are many valid use-cases for disabling zoom... which is probably why the WKWebView API doesn't provide a way to do it. Again, appreciate the team helping move this forward so we can get off our fork.

And I wouldn't consider running java script as a reasonable workaround.

I can see this either way. I spent years working on a project that attempted to make UIWebView usable in a browser context though, and basically everything had to be done with JS in that case, so my judgement on this may well be skewed.

In this case, my take was:

The code being run here seems pretty benign. I.e., I would not expect it to have much likelihood of causing problems for pages.

It's only run if the plugin client explicitly turns off zoom. I would not expect that to be common, so problems would be relegated to edge cases of edge cases.

Given that my feeling is that it's reasonable to provide a best-effort implementation rather than nothing, as if someone is explicitly trying to turn on this behavior they presumably would want it to work on iOS too.

@stuartmorgan Do we have a policy for this already?

We don't have an explicit policy. There are obvious cases where we would say no, like use of private API on iOS, but this isn't doing anything like that, and using JS as a polyfill for OS web view limitations is something that definitely has a history of being relatively common on iOS, so I don't think this is a clear-cut case of "unreasonable".

That makes sense. Then I'm fine with the current implementation of the feature. I would just make it clear how the feature works on iOS in the followup PR for webview_flutter.

Thank you both!

bparrishMines

LGTM

stuartmorgan

Mostly looks good, but I'm not seeing any iOS tests here. The iOS part of this needs test coverage as well.

NickalasB · 2021-10-13T04:26:56Z

@stuartmorgan I noticed there are no existing iOS tests in the flutter_webview_wkwebview package so I wasn't totally sure where to start with that.

bparrishMines · 2021-10-14T01:50:03Z

@NickalasB Ios tests are found in plugins/packages/webview_flutter/webview_flutter_wkwebview/example/ios/RunnerTests/. Since you said you were more familiar with Android, let me know if you need help with writing the tests or you would like me to write them.

NickalasB · 2021-10-14T02:11:54Z

@NickalasB Ios tests are found in plugins/packages/webview_flutter/webview_flutter_wkwebview/example/ios/RunnerTests/. Since you said you were more familiar with Android, let me know if you need help with writing the tests or you would like me to write them.

@bparrishMines Lol. Knowing where the tests are definitely helps! I'd like to take a stab at them myself. If I get stuck I will definitely take you up on your offer to help. Thank you!

stuartmorgan · 2021-10-14T03:07:28Z

@bparrishMines Lol. Knowing where the tests are definitely helps!

This is documented at https://github.com/flutter/flutter/wiki/Plugin-Tests#types-of-tests; suggestions for how to make that easier to find are welcome (although it may just be that since the original PR predates that page, you just never saw the newer version of the contribution guide that links there).

NickalasB · 2021-10-14T14:25:38Z

@bparrishMines alright- after looking at the test classes, I do think I could use some help constructing the tests. I think this is basically along the lines of the correct assertion below in pseudocode but I also realize it's way off...

- (void)testWebViewWebEvaluateJavaScriptSourceIsCorrectWhenShouldEnableZoomIsFalse {
	given self.navigationDelegate.shouldEnableZoom == false;
	when webview.didFinishNavigation;
	then XCTAssertEqual(webView.javaScriptString,
      					@"var meta = document.createElement('meta');"
        				@"meta.name = 'viewport';"
        				@"meta.content = 'width=device-width, initial-scale=1.0, maximum-scale=1.0,"
        				@"user-scalable=no';"
       					@"var head = document.getElementsByTagName('head')[0];head.appendChild(meta);");
}

Again- thank you for the help here

bparrishMines · 2021-10-19T20:03:48Z

@NickalasB That solution looks fine. You could also pass the test NavigationDelegate a mock WkWebView and use verify to check that evaluateJavaScript was called when didFinishNavigation was called.

OCMClassMock(WKWebView.class);
OCMVerify([_mockWebView evaluateJavaScript:/*check parameters*/]);

NickalasB · 2021-10-21T19:19:57Z

@NickalasB That solution looks fine. You could also pass the test NavigationDelegate a mock WkWebView and use verify to check that evaluateJavaScript was called when didFinishNavigation was called.
OCMClassMock(WKWebView.class);
OCMVerify([_mockWebView evaluateJavaScript:/*check parameters*/]);

@bparrishMines @stuartmorgan Finally figured out a way to write the iOS tests. Apologize for the churn. My lack of experience with OC didn't help.

iOS tests added in 74addca

(cherry picked from commit 5238f60)

* master: [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) [ci] Replace Firebase Test Lab deprecated Pixel 4 device with Pixel 5 (flutter#4436) [image_picker_for_web] Added support for maxWidth, maxHeight and imageQuality (flutter#4389) Bump compileSdkVersion to 31 (flutter#4432) [camera] Update [CameraOrientationTests testOrientationNotifications] unit test to work on Xcode 13 (flutter#4426) Update integration_test README (flutter#3824) [webview_flutter] Adjust test URLs again (flutter#4407) [google_sign_in] Add serverAuthCode attribute to google_sign_in_platform_interface user data (flutter#4179) [camera] Add filter for unsupported cameras on Android (flutter#4418)

stuartmorgan

Minor nits, but otherwise LGTM

..._flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterWebView.java

...iew_flutter/webview_flutter_wkwebview/example/ios/RunnerTests/FLTWKNavigationDelegateTests.m

* Formatting and minor test cleanup

* master: Implement Android WebView api with pigeon (Java portion) (flutter#4441) [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434) Implement Android WebView api with pigeon (Dart portion) (flutter#4435) upgraded usage of BinaryMessenger (flutter#4451) [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442)

NickalasB · 2021-10-27T17:38:25Z

Comments addressed @stuartmorgan. Thank you again

stuartmorgan

LGTM!

stuartmorgan · 2021-10-27T18:22:39Z

The iOS failure is a Cirrus bug that we're waiting for them to resolve; once that's green we can get this landed!

@bparrishMines This is one of the PRs I was mentioning that I want to go ahead and land as it's already done; it should be easy to fold into your PR though.

* master: [ci.yaml] Main branch support (flutter#4440) [video_player] Initialize player when size and duration become available (flutter#4438) [webview_flutter] Implement zoom enabled for ios and android (flutter#4417) Partial revert of "upgraded usage of BinaryMessenger (flutter#4451)" (flutter#4453)

…lutter/plugins#4417)

…ideo_src_on_same_controller * commit '76e84c0679dbab7bfaaaa553b17bb0dbdb9a3c33': (537 commits) [video_player] Initialize player when size and duration become available (flutter#4438) [webview_flutter] Implement zoom enabled for ios and android (flutter#4417) Partial revert of "upgraded usage of BinaryMessenger (flutter#4451)" (flutter#4453) Implement Android WebView api with pigeon (Java portion) (flutter#4441) [in_app_purchase] Update to the latest pkg:json_serializable (flutter#4434) Implement Android WebView api with pigeon (Dart portion) (flutter#4435) upgraded usage of BinaryMessenger (flutter#4451) [flutter_plugin_tools] Fix pubspec-check on Windows (flutter#4428) Use OpenJDK 11 in CI jobs (flutter#4419) [google_sign_in] remove the commented out code in tests (flutter#4442) [webview] Fix typos in the README (flutter#4249) [google_sign_in] add serverAuthCode to GoogleSignInAccount (flutter#4180) [ci] Update macOS Cirrus image to Xcode 13 (flutter#4429) [shared_preferences] Switch to new analysis options (flutter#4384) [flutter_plugin_android_lifecycle] remove placeholder dart file (flutter#4413) [camera] Run iOS methods on UI thread by default (flutter#4140) [ci] Always run all `format` steps (flutter#4427) [flutter_plugin_tools] Fix license-check on Windows (flutter#4425) [google_maps_flutter] Clean Java test, consolidate Marker example. (flutter#4400) [image_picker][android] suppress unchecked warning (flutter#4408) ... # Conflicts: # packages/video_player/video_player/pubspec.yaml # packages/video_player/video_player_web/lib/video_player_web.dart # packages/video_player/video_player_web/pubspec.yaml

…#4417)

NickalasB added 10 commits October 5, 2021 11:41

Updating android AUTHORS/CHANGELOG/pubspec.yaml

5c72815

Adding Java implementation for zoomEnabled

1172d73

Updating Android WebView with zoomEnabled param

68283a0

Updating android pubspec version of platform_interface

3d5467c

Updating iOS pubspec/AUTHORS/CHANGELOG

b0f92d6

Adding iOS implementation for zoomEnabled

a342931

Updating iOS pubspec platform_interface version and WebView

b6b4c30

Removing extra line in Android pubspec.yaml

ef139b6

Merge branch 'master' into implement_zoomEnabled_for_ios_and_android

8b7bcb0

* master: [webview_flutter] Update webview platform interface with new methods for running JavaScript. (flutter#4401)

Update platform_interface_version

64db0db

google-cla bot added the cla: yes label Oct 6, 2021

github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-android platform-ios labels Oct 6, 2021

bparrishMines reviewed Oct 6, 2021

View reviewed changes

stuartmorgan self-requested a review October 7, 2021 20:05

bparrishMines approved these changes Oct 12, 2021

View reviewed changes

stuartmorgan suggested changes Oct 13, 2021

View reviewed changes

Adding test coverage for iOS

74addca

NickalasB added 2 commits October 21, 2021 12:44

Updating formatting

f2dd5ad

(cherry picked from commit 5238f60)

stuartmorgan suggested changes Oct 27, 2021

View reviewed changes

NickalasB added 2 commits October 27, 2021 10:13

Addressing PR feedback.

024397a

* Formatting and minor test cleanup

stuartmorgan approved these changes Oct 27, 2021

View reviewed changes

stuartmorgan added last mile waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. labels Oct 27, 2021

fluttergithubbot merged commit bb62aaa into flutter:master Oct 27, 2021

NickalasB deleted the implement_zoomEnabled_for_ios_and_android branch October 27, 2021 23:16

engine-flutter-autoroll mentioned this pull request Oct 28, 2021

Roll Plugins from 984015dffebb to 93f04832c622 (5 revisions) flutter/flutter#92618

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2021

bb62aaa [webview_flutter] Implement zoom enabled for ios and android (f…

d434643

…lutter/plugins#4417)

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021

[webview_flutter] Implement zoom enabled for ios and android (flutter…

206d88a

…#4417)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[webview_flutter] Implement zoom enabled for ios and android (flutter…

7a06ce3

…#4417)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter] Implement zoom enabled for ios and android #4417

[webview_flutter] Implement zoom enabled for ios and android #4417

NickalasB commented Oct 6, 2021

NickalasB commented Oct 6, 2021

bparrishMines Oct 6, 2021

NickalasB Oct 7, 2021 •

edited

bparrishMines Oct 7, 2021

NickalasB Oct 7, 2021

NickalasB Oct 8, 2021

stuartmorgan Oct 12, 2021

bparrishMines Oct 12, 2021 •

edited

NickalasB Oct 12, 2021

bparrishMines left a comment

stuartmorgan left a comment

NickalasB commented Oct 13, 2021

bparrishMines commented Oct 14, 2021

NickalasB commented Oct 14, 2021 •

edited

stuartmorgan commented Oct 14, 2021

NickalasB commented Oct 14, 2021 •

edited

bparrishMines commented Oct 19, 2021

NickalasB commented Oct 21, 2021

stuartmorgan left a comment

NickalasB commented Oct 27, 2021

stuartmorgan left a comment

stuartmorgan commented Oct 27, 2021

[webview_flutter] Implement zoom enabled for ios and android #4417

[webview_flutter] Implement zoom enabled for ios and android #4417

Conversation

NickalasB commented Oct 6, 2021

Pre-launch Checklist

NickalasB commented Oct 6, 2021

bparrishMines Oct 6, 2021

Choose a reason for hiding this comment

NickalasB Oct 7, 2021 • edited

Choose a reason for hiding this comment

bparrishMines Oct 7, 2021

Choose a reason for hiding this comment

NickalasB Oct 7, 2021

Choose a reason for hiding this comment

NickalasB Oct 8, 2021

Choose a reason for hiding this comment

stuartmorgan Oct 12, 2021

Choose a reason for hiding this comment

bparrishMines Oct 12, 2021 • edited

Choose a reason for hiding this comment

NickalasB Oct 12, 2021

Choose a reason for hiding this comment

bparrishMines left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

NickalasB commented Oct 13, 2021

bparrishMines commented Oct 14, 2021

NickalasB commented Oct 14, 2021 • edited

stuartmorgan commented Oct 14, 2021

NickalasB commented Oct 14, 2021 • edited

bparrishMines commented Oct 19, 2021

NickalasB commented Oct 21, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

NickalasB commented Oct 27, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented Oct 27, 2021

NickalasB Oct 7, 2021 •

edited

bparrishMines Oct 12, 2021 •

edited

NickalasB commented Oct 14, 2021 •

edited

NickalasB commented Oct 14, 2021 •

edited