Repository navigation
[webview_flutter] Adds NavigationDelegate.onCreateWindow for target=_blank / window.open - #12312
mateusz-ramp wants to merge 4 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
589efad to
0d9fcf8
Compare
There was a problem hiding this comment.
Code Review
This pull request adds the NavigationDelegate.onCreateWindow callback to the webview_flutter packages to support handling new-window requests, such as target=_blank and window.open, on Android and iOS. The implementation spans the platform interface, Android, and iOS implementations, alongside corresponding tests and version bumps. Feedback on the iOS implementation recommends avoiding an asynchronous IPC call to retrieve the URL when onCreateWindow is not set, and ensuring that empty or null URLs fall back to the default loading behavior instead of being silently dropped.
1214698 to
017722e
Compare
|
@bparrishMines hello, this is my first contribution to Flutter Packages repo, I'm not sure how things work here. Seems like |
017722e to
c34e3a5
Compare
c34e3a5 to
71dfbf3
Compare
a0e3e1d to
05fd8ad
Compare
9c33f76 to
c5ff038
Compare
bparrishMines
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
As mentioned in #12236 (comment), I think the workarounds to deal with the synchronous callbacks is not something we want to add while we plan to transition to bringing ffi/jni support to pigeon. They add a nontrivial amount of complexity to the plugin that we would have to maintain.
Feel free to set this to draft and we can revisit adding this feature once flutter/flutter#190148 has been finished.
| if (webViewClient instanceof WebViewClientProxyApi.WebViewClientImpl) { | ||
| ((WebViewClientProxyApi.WebViewClientImpl) webViewClient).notifyCreateWindow(url); | ||
| } else if (!webViewClient.shouldOverrideUrlLoading(view, request)) { | ||
| view.loadurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Fflutter%2Fpackages%2Fpull%2Furl); |
There was a problem hiding this comment.
I'm concerned that this would be a breaking change for users. It looks like for the iOS implementation, if the setOnCreateWindow is not set, then the plugin will continue to do what is does now. This changes Android to call notifyCreateWindow by default. This potentially needs a check that notifyCreateWindow was even set to ensure this behavior is opt in. The WebViewClient would probably need another setter that set whether notifyCreateWindow should be handled.
| // Only pay the pigeon IPC cost for getUrl when the host opted in. | ||
| final String? url = await navigationAction.request.geturl(); | ||
| if (url != null && url.isNotEmpty) { | ||
| onCreateWindow(url); |
There was a problem hiding this comment.
WKUIDelegate.onCreateWebView requires returning a synchronous WebView because WebKit loads the request in the returned web view. according to the documentation. This seems like a workaround where the url is passed to the user. Does this behave properly when tested?
…ateWindow. # Conflicts: # packages/webview_flutter/webview_flutter_android/CHANGELOG.md # Conflicts: # packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md # packages/webview_flutter/webview_flutter_wkwebview/pubspec.yaml
baaf1c2 to
c773ed4
Compare
Adds an optional
NavigationDelegate.onCreateWindowcallback so hosts canhandle
target=_blank/window.open(e.g. open in an external browser)instead of only loading in the same WebView.
Behavior
onCreateWindowis set: Android (WebChromeClient.onCreateWindow→pigeon) and iOS/macOS (
WKUIDelegate.createWebViewWith) invoke the callbackwith the requested URL.
Packages (federated review PR)
webview_flutter_platform_interface2.16.0webview_flutter_android4.14.0webview_flutter_wkwebview3.27.0webview_flutter4.15.0Path
dependency_overridesare present for combined review only(
make-deps-path-based). This PR is not intended to land as-is; afterapproval we will land/publish in the usual federated order (platform
interface → implementations → app-facing).
Related issues
Related to:
Background (prior same-window
_blankhandling):Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2