Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';
import 'dart:math';

import '../web_kit/web_kit.dart';

/// A view that allows the scrolling and zooming of its contained views.
///
/// Wraps [UIScrollView](https://developer.apple.com/documentation/uikit/uiscrollview?language=objc).
class UIScrollView {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

/// Constructs a [UIScrollView] that is owned by [webView].
// TODO(bparrishMines): Remove ignore once constructor is implemented.
// ignore: avoid_unused_constructor_parameters
UIScrollView.fromWebView(WKWebView webView);

/// Point at which the origin of the content view is offset from the origin of the scroll view.
Future<Point<double>> get contentOffset {
throw UnimplementedError();
}

/// Set point at which the origin of the content view is offset from the origin of the scroll view.
///
/// The default value is `Point<double>(0.0, 0.0)`.
set contentOffset(FutureOr<Point<double>> offset) {
throw UnimplementedError();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import 'package:flutter/foundation.dart';

import '../foundation/foundation.dart';
import '../ui_kit/ui_kit.dart';

/// Times at which to inject script content into a webpage.
///
Expand Down Expand Up @@ -285,6 +286,9 @@ class WKWebView {
late final WKWebViewConfiguration configuration =
WKWebViewConfiguration._fromWebView(this);

/// The scrollable view associated with the web view.
late final UIScrollView scrollView = UIScrollView.fromWebView(this);

/// Used to integrate custom user interface elements into web view interactions.
set uiDelegate(WKUIDelegate delegate) {
throw UnimplementedError();
Expand All @@ -297,4 +301,76 @@ class WKWebView {
Future<void> loadRequest(NSUrlRequest request) {
throw UnimplementedError();
}

/// Loads the contents of the specified HTML string and navigates to it.
Future<void> loadHtmlString(String string, String? baseUrl) {
throw UnimplementedError();
}

/// Loads the web content from the specified file and navigates to it.
Future<void> loadFileUrl(String url, String readAccessUrl) {
throw UnimplementedError();
}

/// Loads the Flutter asset specified in the pubspec.yaml file.
Future<void> loadFlutterAsset(String key) {
throw UnimplementedError();
}

/// Indicates whether there is a valid back item in the back-forward list.
Future<bool> get canGoBack {
throw UnimplementedError();
}

/// Indicates whether there is a valid forward item in the back-forward list.
Future<bool> get canGoForward {
throw UnimplementedError();
}

/// Navigates to the back item in the back-forward list.
Future<void> goBack() {
throw UnimplementedError();
}

/// Navigates to the forward item in the back-forward list.
Future<void> goForward() {
throw UnimplementedError();
}

/// Reloads the current webpage.
Future<void> reload() {
throw UnimplementedError();
}

/// The page title.
Future<String?> get title {
throw UnimplementedError();
}

/// An estimate of what fraction of the current navigation has been loaded.
Future<double> get estimatedProgress {
throw UnimplementedError();
}

/// Indicates whether horizontal swipe gestures trigger page navigation.
///
/// The default value is false.
set allowsBackForwardNavigationGestures(bool allow) {
throw UnimplementedError();
}

/// The custom user agent string.
///
/// The default value of this property is null.
set customUserAgent(String? userAgent) {
throw UnimplementedError();
}

/// Evaluates the specified JavaScript string.
///
/// Throws a `PlatformException` if an error occurs or return value is not
/// supported.
Future<String?> evaluateJavaScript(String javaScriptString) {
throw UnimplementedError();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
// found in the LICENSE file.

import 'dart:async';
import 'dart:math';

import 'package:flutter/foundation.dart';
import 'package:flutter/widgets.dart';
import 'package:path/path.dart' as path;
import 'package:webview_flutter_platform_interface/webview_flutter_platform_interface.dart';

import 'foundation/foundation.dart';
import 'web_kit/web_kit.dart';

/// A [Widget] that displays a [WKWebView].
Expand Down Expand Up @@ -161,6 +164,112 @@ class WebKitWebViewPlatformController extends WebViewPlatformController {
};
}

@override
Future<void> loadHtmlString(String html, {String? baseUrl}) {
return webView.loadHtmlString(html, baseUrl);
}

@override
Future<void> loadFile(String absoluteFilePath) async {
await webView.loadFileUrl(
absoluteFilePath,
path.dirname(absoluteFilePath),
);
}

@override
Future<void> loadFlutterAsset(String key) async {
assert(key.isNotEmpty);
return webView.loadFlutterAsset(key);
}

@override
Future<void> loadUrl(String url, Map<String, String>? headers) async {
final NSUrlRequest request = NSUrlRequest(
url: url,
allHttpHeaderFields: headers ?? <String, String>{},
);
return webView.loadRequest(request);
}

@override
Future<void> loadRequest(WebViewRequest request) async {
if (!request.uri.hasScheme) {
throw ArgumentError('WebViewRequest#uri is required to have a scheme.');
}

final NSUrlRequest urlRequest = NSUrlRequest(
url: request.uri.toString(),
allHttpHeaderFields: request.headers,
httpMethod: describeEnum(request.method),
httpBody: request.body,
);

return webView.loadRequest(urlRequest);
}

@override
Future<bool> canGoBack() => webView.canGoBack;

@override
Future<bool> canGoForward() => webView.canGoForward;

@override
Future<void> goBack() => webView.goBack();

@override
Future<void> goForward() => webView.goForward();

@override
Future<void> reload() => webView.reload();

@override
Future<String> evaluateJavascript(String javascript) async {
return runJavascriptReturningResult(javascript);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@override
Future<void> runJavascript(String javascript) async {
await webView.evaluateJavaScript(javascript);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@override
Future<String> runJavascriptReturningResult(String javascript) async {
return await webView.evaluateJavaScript(javascript) ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Changed to throw ArgumentError on null values.

}

@override
Future<String?> getTitle() => webView.title;

@override
Future<void> scrollTo(int x, int y) async {
webView.scrollView.contentOffset = Point<double>(
x.toDouble(),
y.toDouble(),
);
}

@override
Future<void> scrollBy(int x, int y) async {
final Point<double> offset = await webView.scrollView.contentOffset;
webView.scrollView.contentOffset = Point<double>(
offset.x + x.toDouble(),
offset.y + y.toDouble(),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@override
Future<int> getScrollX() async {
final Point<double> offset = await webView.scrollView.contentOffset;
return offset.x.toInt();
}

@override
Future<int> getScrollY() async {
final Point<double> offset = await webView.scrollView.contentOffset;
return offset.y.toInt();
}

@override
Future<void> addJavascriptChannels(Set<String> javascriptChannelNames) async {
await Future.wait<void>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ flutter:
dependencies:
flutter:
sdk: flutter
path: ^1.8.0
webview_flutter_platform_interface: ^1.8.0

dev_dependencies:
Expand Down
Loading