[webview_flutter_wkwebview] Add javascript panel interface for wkwebview #5795

jsharp83 · 2024-01-04T05:12:39Z

There are cases where Web calls System Popup with javascript on webview_flutter
At this time, the message comes in the WKUIDelegate part in iOS.
- https://developer.apple.com/documentation/webkit/wkuidelegate/1537406-webview
- https://developer.apple.com/documentation/webkit/wkuidelegate/1536489-webview
Related issue: alert() method doesn't work in WebView flutter#30358 (comment)
Related Interface PR: [webview_flutter] Implement platform interface for JavaScript dialog #5670
The PR that contains all changes can be found at [webview_flutter] Add interface for showing javascript dialog message #4704

Pre-launch Checklist

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

hellohuanlin · 2024-01-11T20:14:29Z

i remember that i have reviewed a similar PR before. Why are we opening a new PR?

Also can you invite the same reviewers that you requested review before, so all previous reviewers have context and make sure their comments are addressed

jsharp83 · 2024-01-11T23:45:08Z

@hellohuanlin

Previous PR is #4704 (comment)
Since previous PR includes implementations for multiple packages, I was requested to split it into separate PRs for each package.
This PR only contains modifications related to wkwebview.

I don't have the permission to invite other reviewers. I will mention another reviewer to check Thank you.

@stuartmorgan @bparrishMines Can you check this PR. Thanks

bparrishMines · 2024-01-16T17:46:09Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart

+          );
+        });
+
+    return result.runtimeType == bool && result as bool;


I think this would be easier to read as result is bool ? result : false.

@bparrishMines I just remove the result of dynamic type on aebf948

bparrishMines · 2024-01-16T17:53:54Z

packages/webview_flutter/webview_flutter_wkwebview/example/lib/main.dart

+          );
+        });
+
+    return result.runtimeType == String ? result as String : '';


This one could be result is String ? result : ''.

Although I just realized that the prompt() dialog can technically return a null String and should provide support for this according to Android docs: https://developer.android.com/reference/android/webkit/WebChromeClient#onJsPrompt(android.webkit.WebView,%20java.lang.String,%20java.lang.String,%20java.lang.String,%20android.webkit.JsPromptResult). I'll actually go ahead and create a PR to update the interface method.

@bparrishMines

I agree with your opinion. but one thing I worried that confirm() dialog also should be handle null case with this opinion.

I just thought that dismissing dialog without selection should be false in confirm and empty string in prompt.
But if we handle it as optional null, confirm dialog also return null when we just dismiss dialog.

For confirm(), I considered the same thing. However, I think canceling the operation is the same as returning false.

prompt() is slightly different because there could be scenario where a user might want to return an empty string. And this could be considered different from a user that wants to cancel the operation.

bparrishMines · 2024-01-16T17:54:41Z

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

+                                                     completionHandler:^{
+                                                       completionHandler();
+                                                     }];


I think this can be simplified to:

Suggested change

completionHandler:^{

completionHandler();

}];

completionHandler:completionHandler];

bparrishMines · 2024-01-16T17:55:03Z

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

+                                                       completionHandler:^(BOOL isConfirmed) {
+                                                         completionHandler(isConfirmed);
+                                                       }];


Same

Suggested change

completionHandler:^(BOOL isConfirmed) {

completionHandler(isConfirmed);

}];

completionHandler:completionHandler];

bparrishMines · 2024-01-16T17:55:27Z

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

+                                         completionHandler:^(NSString *_Nullable inputMessage) {
+                                           completionHandler(inputMessage);
+                                         }];


Same

Suggested change

completionHandler:^(NSString *_Nullable inputMessage) {

completionHandler(inputMessage);

}];

completionHandler:completionHandler];

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

packages/webview_flutter/webview_flutter_wkwebview/pigeons/web_kit.dart

bparrishMines

LGTM

cc @stuartmorgan or @hellohuanlin for secondary review.

I'll update the version on approval

hellohuanlin · 2024-01-18T02:26:59Z

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

@@ -167,7 +167,7 @@ WKAudiovisualMediaTypes FWFNativeWKAudiovisualMediaTypeFromEnumData(

 FWFNSUrlRequestData *FWFNSUrlRequestDataFromNativeNSURLRequest(NSURLRequest *request) {
  return [FWFNSUrlRequestData
-              makeWithUrl:request.URL.absoluteString
+              makeWithUrl:request.URL.absoluteString.length > 0 ? request.URL.absoluteString : @""


im a bit confused here - will absoluteString ever be nil? if so, may be less confusing to do nil check rather than length check?

packages/packages/webview_flutter/webview_flutter_wkwebview/example/integration_test/legacy/webview_flutter_test.dart

Lines 1124 to 1149 in 83c2c4d

testWidgets('target _blank opens in same window',

(WidgetTester tester) async {

final Completer<WebViewController> controllerCompleter =

Completer<WebViewController>();

final Completer<void> pageLoaded = Completer<void>();

await tester.pumpWidget(

Directionality(

textDirection: TextDirection.ltr,

child: WebView(

key: GlobalKey(),

onWebViewCreated: (WebViewController controller) {

controllerCompleter.complete(controller);

},

javascriptMode: JavascriptMode.unrestricted,

onPageFinished: (String url) {

pageLoaded.complete(null);

},

),

),

);

final WebViewController controller = await controllerCompleter.future;

await controller.runJavascript('window.open("$primaryUrl", "_blank")');

await pageLoaded.future;

final String? currentUrl = await controller.currentUrl();

expect(currentUrl, primaryUrl);

});

@hellohuanlin
It can be nil with this test case.
I updated code to nil check.
Thanks

hellohuanlin · 2024-01-18T02:28:10Z

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

+                                                   frame:FWFWKFrameInfoDataFromNativeWKFrameInfo(
+                                                             frame)
+                                              completion:^(FlutterError *error) {
+                                                NSAssert(!error, @"%@", error);


when will error happen? should we pipe the error to dark side rather than doing assert here?

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

Lines 34 to 91 in 83c2c4d

- (void)onCreateWebViewForDelegate:(FWFUIDelegate *)instance

webView:(WKWebView *)webView

configuration:(WKWebViewConfiguration *)configuration

navigationAction:(WKNavigationAction *)navigationAction

completion:(void (^)(FlutterError *_Nullable))completion {

if (![self.instanceManager containsInstance:configuration]) {

[self.webViewConfigurationFlutterApi createWithConfiguration:configuration

completion:^(FlutterError *error) {

NSAssert(!error, @"%@", error);

}];

}

NSInteger configurationIdentifier =

[self.instanceManager identifierWithStrongReferenceForInstance:configuration];

FWFWKNavigationActionData *navigationActionData =

FWFWKNavigationActionDataFromNativeWKNavigationAction(navigationAction);

[self

onCreateWebViewForDelegateWithIdentifier:[self identifierForDelegate:instance]

webViewIdentifier:[self.instanceManager

identifierWithStrongReferenceForInstance:webView]

configurationIdentifier:configurationIdentifier

navigationAction:navigationActionData

completion:completion];

}

- (void)requestMediaCapturePermissionForDelegateWithIdentifier:(FWFUIDelegate *)instance

webView:(WKWebView *)webView

origin:(WKSecurityOrigin *)origin

frame:(WKFrameInfo *)frame

type:(WKMediaCaptureType)type

completion:

(void (^)(WKPermissionDecision))completion

API_AVAILABLE(ios(15.0)) {

[self

requestMediaCapturePermissionForDelegateWithIdentifier:[self identifierForDelegate:instance]

webViewIdentifier:

[self.instanceManager

identifierWithStrongReferenceForInstance:webView]

origin:

FWFWKSecurityOriginDataFromNativeWKSecurityOrigin(

origin)

frame:

FWFWKFrameInfoDataFromNativeWKFrameInfo(

frame)

type:

FWFWKMediaCaptureTypeDataFromNativeWKMediaCaptureType(

type)

completion:^(

FWFWKPermissionDecisionData *decision,

FlutterError *error) {

NSAssert(!error, @"%@", error);

completion(

FWFNativeWKPermissionDecisionFromData(

decision));

}];

}

@end

@hellohuanlin In this case, I just followed previous implementation. I also agree with that NSAssert can be dangerous sometime, but the error only came from pigeon generation part like follow code and I understood that it is serious situation if channel is not established.

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

Lines 3094 to 3098 in 83c2c4d

completion(nil,

[FlutterError errorWithCode:@"channel-error"

message:@"Unable to establish connection on channel."

details:@""]);

}

We are still deciding on a better alternative for using NSAssert in callback methods: flutter/flutter#140410

@jsharp83 is correct that we probably won't be able to send the error back to Dart, so we need a way to handle it on the platform side.

Definitely feel free to add feedback to this issue!

…t-panel-interface-wkwebview

…r wkwebview (flutter/packages#5795)

flutter/packages@83c2c4d...129e08c 2024-01-19 [email protected] [pigeon] Run swift-format on ungenerated example app Swift files (flutter/packages#5934) 2024-01-19 [email protected] [path_provider_foundation] Run swift-format on Swift files (flutter/packages#5935) 2024-01-19 [email protected] [shared_preferences_foundation] Run swift-format on Swift files (flutter/packages#5933) 2024-01-19 [email protected] [various] Run swift-format on example app Swift files (flutter/packages#5931) 2024-01-19 [email protected] [camera_avfoundation] Remove development team from Xcode example app (flutter/packages#5930) 2024-01-18 [email protected] [webview_flutter_wkwebview] Add javascript panel interface for wkwebview (flutter/packages#5795) 2024-01-18 [email protected] [two_dimensional_scrollables] Fix must_call_super (flutter/packages#5921) 2024-01-18 [email protected] [pointer_interceptor] fix width and height unset warning on web platform (flutter/packages#5864) 2024-01-18 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.23.0 to 3.23.1 (flutter/packages#5922) 2024-01-18 [email protected] [google_maps_flutter] Clean up iOS example project (flutter/packages#5925) 2024-01-18 [email protected] [ci] Add flags to formatter command to decide which formatters to run (flutter/packages#5905) 2024-01-18 [email protected] Update tests to Xcode 15 and iOS 17 simulator (flutter/packages#5914) 2024-01-18 [email protected] Roll Flutter from def6af0 to f77f824 (25 revisions) (flutter/packages#5924) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…iew (flutter#5795) * There are cases where Web calls System Popup with javascript on webview_flutter * At this time, the message comes in the WKUIDelegate part in iOS. * https://developer.apple.com/documentation/webkit/wkuidelegate/1537406-webview * https://developer.apple.com/documentation/webkit/wkuidelegate/1536489-webview * Related issue: flutter/flutter#30358 (comment) * Related Interface PR: flutter#5670 * The PR that contains all changes can be found at flutter#4704

jsharp83 requested a review from hellohuanlin as a code owner January 4, 2024 05:12

github-actions bot added platform-ios p: webview_flutter labels Jan 4, 2024

jsharp83 force-pushed the add-javascript-panel-interface-wkwebview branch 2 times, most recently from 9aea83f to 961f82e Compare January 4, 2024 05:41

Add javascript panel interface for wkwebview

1923b69

jsharp83 force-pushed the add-javascript-panel-interface-wkwebview branch from 961f82e to 1923b69 Compare January 11, 2024 23:51

bparrishMines requested changes Jan 16, 2024

View reviewed changes

remove dynamic type result

aebf948

jsharp83 force-pushed the add-javascript-panel-interface-wkwebview branch from b8bc0cf to aebf948 Compare January 17, 2024 00:05

jsharp83 and others added 3 commits January 17, 2024 09:14

remove unnecessary completionHandler definitions

d062b10

formatting

f7ea3a1

tests

4222646

bparrishMines approved these changes Jan 18, 2024

View reviewed changes

hellohuanlin reviewed Jan 18, 2024

View reviewed changes

change nil check

c480a66

hellohuanlin approved these changes Jan 18, 2024

View reviewed changes

Merge branch 'main' of github.com:flutter/packages into add-javascrip…

714a0eb

…t-panel-interface-wkwebview

bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 18, 2024

auto-submit bot merged commit 2762c54 into flutter:main Jan 18, 2024

engine-flutter-autoroll mentioned this pull request Jan 19, 2024

Roll Packages from 83c2c4da97c5 to 129e08c1dff1 (13 revisions) flutter/flutter#141865

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 19, 2024

2762c54 [webview_flutter_wkwebview] Add javascript panel interface fo…

95894c5

…r wkwebview (flutter/packages#5795)

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

	testWidgets('target _blank opens in same window',
	(WidgetTester tester) async {
	final Completer<WebViewController> controllerCompleter =
	Completer<WebViewController>();
	final Completer<void> pageLoaded = Completer<void>();
	await tester.pumpWidget(
	Directionality(
	textDirection: TextDirection.ltr,
	child: WebView(
	key: GlobalKey(),
	onWebViewCreated: (WebViewController controller) {
	controllerCompleter.complete(controller);
	},
	javascriptMode: JavascriptMode.unrestricted,
	onPageFinished: (String url) {
	pageLoaded.complete(null);
	},
	),
	),
	);
	final WebViewController controller = await controllerCompleter.future;
	await controller.runJavascript('window.open("$primaryUrl", "_blank")');
	await pageLoaded.future;
	final String? currentUrl = await controller.currentUrl();
	expect(currentUrl, primaryUrl);
	});

	- (void)onCreateWebViewForDelegate:(FWFUIDelegate *)instance
	webView:(WKWebView *)webView
	configuration:(WKWebViewConfiguration *)configuration
	navigationAction:(WKNavigationAction *)navigationAction
	completion:(void (^)(FlutterError *_Nullable))completion {
	if (![self.instanceManager containsInstance:configuration]) {
	[self.webViewConfigurationFlutterApi createWithConfiguration:configuration
	completion:^(FlutterError *error) {
	NSAssert(!error, @"%@", error);
	}];
	}

	NSInteger configurationIdentifier =
	[self.instanceManager identifierWithStrongReferenceForInstance:configuration];
	FWFWKNavigationActionData *navigationActionData =
	FWFWKNavigationActionDataFromNativeWKNavigationAction(navigationAction);

	[self
	onCreateWebViewForDelegateWithIdentifier:[self identifierForDelegate:instance]
	webViewIdentifier:[self.instanceManager
	identifierWithStrongReferenceForInstance:webView]
	configurationIdentifier:configurationIdentifier
	navigationAction:navigationActionData
	completion:completion];
	}

	- (void)requestMediaCapturePermissionForDelegateWithIdentifier:(FWFUIDelegate *)instance
	webView:(WKWebView *)webView
	origin:(WKSecurityOrigin *)origin
	frame:(WKFrameInfo *)frame
	type:(WKMediaCaptureType)type
	completion:
	(void (^)(WKPermissionDecision))completion
	API_AVAILABLE(ios(15.0)) {
	[self
	requestMediaCapturePermissionForDelegateWithIdentifier:[self identifierForDelegate:instance]
	webViewIdentifier:
	[self.instanceManager
	identifierWithStrongReferenceForInstance:webView]
	origin:
	FWFWKSecurityOriginDataFromNativeWKSecurityOrigin(
	origin)
	frame:
	FWFWKFrameInfoDataFromNativeWKFrameInfo(
	frame)
	type:
	FWFWKMediaCaptureTypeDataFromNativeWKMediaCaptureType(
	type)
	completion:^(
	FWFWKPermissionDecisionData *decision,
	FlutterError *error) {
	NSAssert(!error, @"%@", error);
	completion(
	FWFNativeWKPermissionDecisionFromData(
	decision));
	}];
	}
	@end

	completion(nil,
	[FlutterError errorWithCode:@"channel-error"
	message:@"Unable to establish connection on channel."
	details:@""]);
	}

[webview_flutter_wkwebview] Add javascript panel interface for wkwebview #5795

[webview_flutter_wkwebview] Add javascript panel interface for wkwebview #5795

Uh oh!

Conversation

jsharp83 commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

hellohuanlin commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsharp83 commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsharp83 Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsharp83 commented Jan 4, 2024 •

edited

Loading

hellohuanlin commented Jan 11, 2024 •

edited

Loading

jsharp83 commented Jan 11, 2024 •

edited

Loading

jsharp83 Jan 18, 2024 •

edited

Loading