Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[webview_flutter] Add interface for showing javascript dialog message #4704

Merged
merged 12 commits into from
Feb 9, 2024

Conversation

jsharp83
Copy link
Contributor

@jsharp83 jsharp83 commented Aug 15, 2023

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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.

@jsharp83
Copy link
Contributor Author

@bparrishMines Hi, Can you check this PR? This is my first contribution here so I don't know exact routine for contribution. Help me please. Thanks :)

@jsharp83 jsharp83 force-pushed the add-javascript-panel-interface branch from 7dfb120 to 850e9f5 Compare August 28, 2023 00:16
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this for Android and iOS! I left my first comments.

Also, we're working on updating pigeon to handle more of the wrapping code if you would prefer to wait for that before continuing.


* Updates minimum supported SDK version to Flutter 3.7/Dart 2.19.
* Add support to show JavaScript dialog. See `PlatformWebViewController.setJavaScriptAlertDialogCallback`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
* Add support to show JavaScript dialog. See `PlatformWebViewController.setJavaScriptAlertDialogCallback`.
* Adds support to show JavaScript dialog. See `PlatformWebViewController.setJavaScriptAlertDialogCallback`.

This can also list the other two callback methods.

Comment on lines 283 to 285
Future<void> setJavaScriptAlertDialogCallback(
Future<void> Function(String message)
javaScriptAlertDialogCallback) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is only setting one callback method, the pattern should be

Suggested change
Future<void> setJavaScriptAlertDialogCallback(
Future<void> Function(String message)
javaScriptAlertDialogCallback) async {
Future<void> setOnJavaScriptAlertDialog(
Future<void> Function(String message)
onJavaScriptAlertDialog) async {

Same for the two methods below.

@@ -4,7 +4,7 @@ repository: https://github.com/flutter/packages/tree/main/packages/webview_flutt
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+webview_flutter%22
# NOTE: We strongly prefer non-breaking changes, even at the expense of a
# less-clean API. See https://flutter.dev/go/platform-interface-breaking-changes
version: 2.5.0
version: 2.5.1
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a minor version bump since it is adding a new feature. (e.g. 2.6.0)

@@ -38,6 +40,8 @@ public static class WebChromeClientImpl extends SecureWebChromeClient {
private final WebChromeClientFlutterApiImpl flutterApi;
private boolean returnValueForOnShowFileChooser = false;

private boolean hasJavaScriptDialogCallback = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the platform interface separates handling the callbacks into three separate methods, you should also have three separate flags for each alert/confirm/prompt. What happens if a user only wants to set the confirm dialog?

'runJavaScriptDialogForDelegateWithIdentifier:type:message:defaultText:',
)
@async
JavaScriptDialogCompletionData runJavaScriptDialog(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the native WKUIDelegate separates handling dialogs into separate methods, this should also.

@jsharp83 jsharp83 force-pushed the add-javascript-panel-interface branch 4 times, most recently from 232e186 to 41bf657 Compare September 20, 2023 13:45
@jsharp83
Copy link
Contributor Author

@bparrishMines It has been modified according to the review. Could you please check the code again?


/// Sets a callback that notifies the host application that the web page
/// wants to display a JavaScript alert() dialog.
Future<void> setOnJavaScriptAlertDialogCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

This still uses Callback at the end. It should be setOnJavaScriptAlertDialog. Same for the confirm and textInputDialog methods below.

@@ -1150,6 +1170,25 @@ class WebChromeClient extends JavaObject {
);
}

Future<void> setHasJavaScriptAlertCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the two methods below should follow the same pattern as onShowFileChooser and be called setSynchronousReturnValueForOnJsAlert.

@@ -1121,6 +1133,14 @@ class WebChromeClient extends JavaObject {
/// mode.
final HideCustomViewCallback? onHideCustomView;

final Future<void> Function(String message)? onHandleJavaScriptAlert;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should have the same name as the corresponding Java method. (e.g. onJsAlert). They should also have a return value of void since they can't return a synchronous value.

Copy link
Contributor Author

@jsharp83 jsharp83 Sep 28, 2023

Choose a reason for hiding this comment

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

@bparrishMines
Sorry, I cannot fully understand on your comment.

They should also have a return value of void since they can't return a synchronous value.

They should have a Future value to pass the value for JSResult and it works well in my test.

Simulator Screen Recording - iPhone 13 mini - 2023-09-28 at 15 04 27

FileChooser in android_webview also return a List previously, So I just follow it to implement.

Can you explain more to fix this?
I really appreciate your help :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, now I understand. Your implementation is correct. I didn't notice the JSResult in the params.

Comment on lines 1140 to 1142
final bool Function()? hasJavaScriptAlertCallback;
final bool Function()? hasJavaScriptConfirmCallback;
final bool Function()? hasJavaScriptPromptCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be removed. I don't believe they are used and they are redundant with the callback methods above.

@@ -231,6 +232,35 @@ class AndroidWebViewController extends PlatformWebViewController {
};
},
),
onHandleJavaScriptAlert: withWeakReferenceTo(this, (WeakReference<AndroidWebViewController> weakReference) {
return (String message) async {
final Future<void> Function(String)? callback = _onJavaScriptAlertCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use weakReference?.target._onJavaScriptAlertCallback, otherwise this will cause a leak. Same for the other dialog fields below.

'runJavaScriptAlertPanelForDelegateWithIdentifier:message:',
)
@async
void runJavaScriptAlertPanelWithIdentifier(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be runJavaScriptAlertPanel. The withIdentifier should only be added to the @ObjCSelector annotation above and not in the method.

'runJavaScriptConfirmPanelForDelegateWithIdentifier:message:',
)
@async
bool runJavaScriptConfirmPanelWithIdentifier(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this method

@@ -216,6 +216,34 @@ class WebKitWebViewController extends PlatformWebViewController {
return decisionCompleter.future;
}
},
runJavaScriptAlertDialog: (String message) async {
final Future<void> Function(String message)? callback =
_onJavaScriptAlertDialogCallback;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be weakThis.target?._onJavaScriptAlertDialogCallback. The methods below also need to be updated.

@jsharp83 jsharp83 force-pushed the add-javascript-panel-interface branch 11 times, most recently from 54a7f6b to 634c05f Compare September 28, 2023 06:23
/// Sets a callback that notifies the host application that the web page
/// wants to display a JavaScript alert() dialog.
Future<void> setOnJavaScriptAlertDialog(
Future<void> Function(String message)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like WebChromeClient.onJsAlert and WKUIDelegate.runJavaScriptAlertPanelWithMessage also provide the URL for these callbacks. We should change String message to a class that contains a message field where we can add additional information at a later point. You could create a class named something like JavaScriptAlertDialogRequest. See JavaScriptConsoleMessage as an example.

This goes for the methods below as well. (e.g. JavaScriptConfirmDialogRequest and JavaScriptTextInputDialogRequest).

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 fixed it. Can you check it again?

@@ -1,3 +1,8 @@
## 2.7.0

* Adds support to show JavaScript dialog. Sees `PlatformWebViewController.setOnJavaScriptAlertDialog`, `PlatformWebViewController.setOnJavaScriptConfirmDialog` and `PlatformWebViewController.setOnJavaScriptTextInputDialog`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
* Adds support to show JavaScript dialog. Sees `PlatformWebViewController.setOnJavaScriptAlertDialog`, `PlatformWebViewController.setOnJavaScriptConfirmDialog` and `PlatformWebViewController.setOnJavaScriptTextInputDialog`.
* Adds support to show JavaScript dialog. See
`PlatformWebViewController.setOnJavaScriptAlertDialog`,
`PlatformWebViewController.setOnJavaScriptConfirmDialog`, and
`PlatformWebViewController.setOnJavaScriptTextInputDialog`.

@jsharp83 jsharp83 force-pushed the add-javascript-panel-interface branch from 2e83c14 to 3930794 Compare October 5, 2023 09:18
@jsharp83 jsharp83 force-pushed the add-javascript-panel-interface branch from d18d501 to 2b35ddd Compare February 5, 2024 10:23
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024
Copy link
Contributor

auto-submit bot commented Feb 9, 2024

auto label is removed for flutter/packages/4704, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@bparrishMines
Copy link
Contributor

@stuartmorgan It looks like this needs the approval along with your LGTM

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM; looks like I just pressed the wrong button last time.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024
@auto-submit auto-submit bot merged commit 11152d2 into flutter:main Feb 9, 2024
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 9, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 9, 2024
flutter/packages@29d8cc0...11152d2

2024-02-09 jsharp83@gmail.com [webview_flutter] Add interface for showing javascript dialog message (flutter/packages#4704)
2024-02-09 32931186+erdzan12@users.noreply.github.com [pigeon] Implement Screaming Snake Case Conversion for Kotlin Enum Cases (flutter/packages#5918)
2024-02-09 43054281+camsim99@users.noreply.github.com [camerax] Small fixes to starting/stopping video capture (flutter/packages#6068)
2024-02-08 engine-flutter-autoroll@skia.org Roll Flutter from 8431cae to eb5d0a4 (33 revisions) (flutter/packages#6079)

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 flutter-ecosystem@google.com,rmistry@google.com 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
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…lutter#5670)

Adds the platform interface implementation for JavaScript dailog.

This PR is part of a series of PRs that aim to close flutter/flutter#30358 (comment)
The PR that contains all changes can be found at flutter#4704
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…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
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…lutter#5796)

* There are cases where Web calls System Popup with javascript on webview_flutter
* Android has a interface on WebChromeClient
   * https://developer.android.com/reference/android/webkit/WebChromeClient#onJsAlert(android.webkit.WebView,%20java.lang.String,%20java.lang.String,%20android.webkit.JsResult)

* Related issue: flutter/flutter#30358 (comment)
* Related Interface PR: flutter#5670
* The PR that contains all changes can be found at flutter#4704
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
…flutter#4704)

* 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
* Android also has a interface on WebChromeClient
   * https://developer.android.com/reference/android/webkit/WebChromeClient#onJsAlert(android.webkit.WebView,%20java.lang.String,%20java.lang.String,%20android.webkit.JsResult)
* It was implemented according to the requirements of the code review of the flutter#4555
* Related issue: flutter/flutter#30358 (comment)
* Related Interface PR: flutter#5670
bill-skdnd pushed a commit to skdnd-metaspace/webview_flutter_wkwebview that referenced this pull request Aug 2, 2024
…iew (#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/packages#5670
* The PR that contains all changes can be found at flutter/packages#4704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App federated: all_changes PR that contains changes for all packages for a federated plugin change p: webview_flutter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants