[webview_flutter_wkwebview] Add WKWebView method stubs

[bparrishMines]

No version change:
Part of flutter/flutter#93732 and doesn't make any changes to the current implementation.

No CHANGELOG change: Incremental unused code doesn't need to be noted in the CHANGELOG.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[stuartmorgan-g]

This is one of those things that makes me suspect going all the way to the native boundary isn't going to be a great experience (at least not without automatic language projection); having to plumb in new ancillary objects for minor usage could get ugly.

(No change needed here, just flagging as something we'll want to watch for over time as we evaluate this approach and think about whether we want to flex the boundary back a little.)

[stuartmorgan-g]

You're going to need this handling in the Dart code, otherwise this method will throw errors all the time.

[bparrishMines]

Done. I added a try catch and added a TODO to ensure the NSError is passed back with the PlatformException.

[stuartmorgan-g]

This method is only supposed to be called when a result is expected, so the ?? '' seems wrong. We should probably throw a clear error message if it's null instead (e.g., telling them to use runJavascript instead if they aren't returning anything).

[bparrishMines]

done. Changed to throw ArgumentError on null values.

[stuartmorgan-g]

We should consider flexing the boundary here and putting scroll-by functionality on the webview; this method will do weird things if there's other scrolling happening at the same time due to the async gap between the getter and setter.

E.g., imagine that someone calls (scrollBy(0, 10), and the JS on the page does its own scroll-by 50y which happens to fire at ~the same time. With the current (native) implementation, the result is that the page scrolls 60. With this implementation, the result will either be that it scrolls by 60, or that it scrolls by 10 (possibly with a visible flicker to 50), depending on where the JS fires relative to the async calls.

[bparrishMines]

I think this is caused by the fact that scrollBy is asynchronous and not that there are multiple calls. Even after I added a scrollBy method to UIScrollView, an app user will still be able to scroll while the scrollBy method call is being passed over. Leading to the same problem, right? I would also suspect that scrollBy is often used after calling getScrollX and getScrollY anyways.

I think async message channels just makes this method inherently flawed compared to just using scrollTo.

[bparrishMines]

That also makes me wonder if we should change getScrollX and getScrollY to getScrollPosition in the new interface.

[stuartmorgan-g]

Even after I added a scrollBy method to UIScrollView, an app user will still be able to scroll while the scrollBy method call is being passed over. Leading to the same problem, right?

If it's a single call over the method channel, no scrolling can be lost even when it's async. Regardless of what the user is doing around when it arrives, it will still scroll by the requested amount.

In my example scenario, here are the possible outcomes with the multi-async-call implementation:

other scroll happens
* plugin gets position
* plugin sets position
=> Total scroll is the sum of both scrolls

* plugins gets position
other scroll happens
* plugin sets position
=> Total scroll is 10; the other scroll is destroyed

* plugin gets position
* plugin sets position
other scroll happens
= Total scroll is the sum of both scrolls

(If there is a stream of other scroll events, the chances of some of them being in the middle and lost go up significantly.)

With the single async method, the other scroll can only be entirely before or entirely after, so the lost scroll can't happen.

I would also suspect that scrollBy is often used after calling getScrollX and getScrollY anyways.

Why wouldn't people use scrollTo for that use case? I would expect scrollBy to be for something like a "page down" button/key, where the delta is all that matters.

But yes, if people are trying to use getScroll + scrollBy as a way of getting scrollTo behavior, they may get unexpected results due to async. That's already true though.

[stuartmorgan-g]

That also makes me wonder if we should change getScrollX and getScrollY to getScrollPosition in the new interface.

I think that makes a lot of sense.

[bparrishMines]

@stuartmorgan This is ready for a second round review.

[stuartmorgan-g]

This passthrough doesn't look like it will have the right behavior on older versions of iOS. Prior to 14, when we had to split out runJavascript/runJavascriptReturningResult, evaluateJavascript was the method to call regardless of whether there was a result. You're calling a method that throws on null return.

[stuartmorgan-g]

To cover the issue mentioned in my other comment: we should have two tests for evaulateJavascript, one where the mock webview returns something and one where it returns null.

[bparrishMines]

Done. From testing it looks like a nil return value for evaluateJavaScript would return the String (null). This implementation returns null. This is because toString is handled differently with Objective-C vs Dart.

In this implementation, WKWebView.evaluateJavaScript returns a Future<Object?>. But the current implementation turns the return value to a String before it is returned: https://github.com/flutter/plugins/blame/main/packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m#L384.

Should we make these align? I was considering making WKWebView.evaluateJavaScript return Future<String>. Then change it to Future<Object?> in webview_flutter 4.0.0

[stuartmorgan-g]

Done. From testing it looks like a nil return value for evaluateJavaScript would return the String (null). This implementation returns null. [...] Should we make these align?

Yes, good catch. We should have it (just the legacy evaluateJavascript implementation) translate null to '(null)' for consistent behavior, since people may have been relying on the exact string that the ObjC implementation would produce.

I was considering making WKWebView.evaluateJavaScript return Future<String>. Then change it to Future<Object?> in webview_flutter 4.0.0

WKWebView.evaluateJavaScript should always return Future<Object?>, because that's what the wrapped API returns. WebKitWebViewPlatformController.evaluateJavascript is where we should do special handling.

[stuartmorgan-g]

LGTM with the 'null'->'(null)' change.

[bparrishMines]

merging on green