-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[webview_flutter] Add listener for scroll position change in Android #2107
Conversation
5ebbd10
to
8ceeb22
Compare
Thanks for the contribution! I'm following the initial PR review policy, this PR isn't trivial to review so I'm labeling it with "backlog" and we will prioritize according to the issue's priority. Relevant issue: flutter/flutter#42442 |
@amirh any progress on this? |
I took an initial look at this; at a high level it looks good, but per this is a case where we should land it in parallel with an iOS implementation per the policy described at https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#platform-support If anyone is interested in moving this forward, I encourage you to open a PR based on this one that adds an iOS implementation; once we have both we can review this one and the iOS portion of that one. This will also need to be updated for the federation changes (which is likely currently blocked on flutter/flutter#94051 for the moment; we hope to resolve that at the beginning of 2022). |
@TheVinhLuong Are you planning on updating this for the structural changes in the plugin (federation, Android conversion to a more Dart-based implementation), so that it would be ready as the foundation for a cross-platform PR? |
@stuartmorgan Sorry for my late reply. I will update the PR for the Android side, also for iOS if I can make it. |
@TheVinhLuong Just a friendly check-in to see if this is still something you have on your radar? |
Yes, I have implemented it both for android and ios. I am writing test now. |
8ceeb22
to
647c19b
Compare
9ede0a4
to
61a88bd
Compare
679a062
to
b65687d
Compare
8c1e359
to
5fe5d42
Compare
4d6a4e7
to
6072891
Compare
6072891
to
de090aa
Compare
Hi @stuartmorgan, can you review my PR? I have updated the PR according to the new plugin federation format. Since this PR is just an implementation for the Android side the automated test for changelogs is failing for other platforms. Please add an 'override: no versioning needed' label for this PR if there is nothing wrong. Thank you! |
You just need to revert the tool-generated pubspec changes in those other packages. I'll review when I get a chance, but in the meantime you have Dart unit test and analysis failures in CI that will need to be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some early feedback. Are you able to do the iOS portion as well or would you like help with that?
|
||
import androidx.annotation.Nullable; | ||
/** Define extending APIs for {@link android.webkit.WebView} */ | ||
public interface WebViewExtendedApi { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new method to WebView
, you should add a nonScrollChanged
callback to the Dart WebView
. The same way WebViewClient
adds a callback: https://github.com/flutter/plugins/blob/main/packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart#L759
I would also make the callback include the WebView
for convenience:
final void Function(WebView webView, int left, int top, int oldLeft, int oldTop)? onScrollChanged;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean WebViewExtendedApi
is not necessarily needed? I did read how the WebViewClient
and attempted to do the same but couldn't find another way to set a scroll listener to a WebView
without defining the WebViewExtendedApi
.
The difference between setting a ScrollListener
and a WebViewClient
is that the webkit.WebView
expose the method setWebViewClient
for public use regardless of the Android required API level, this is unlike setOnScrollChangeListener
since this is the method required min API level 23. The workaround here is to override the methodonScrollChanged
of the WebView
and get the offset changed event from there.
Do you have another approach for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea you can start by adding the onScrollChanged
parameter to WebView
:
class WebView {
WebView({this.useHybridComposition = false, this.onScrollChanged}) : super.detached() {
api.createFromInstance(this);
}
final void Function(WebView webView, int left, int top, int oldLeft, int oldTop)? onScrollChanged;
}
Then will need to establish a Flutter API for WebView in the pigeons/android_webview.dart
file:
@FlutterApi()
abstract class WebViewFlutterApi {
void onScrollChanged(int identifier, ...);
}
Then you can run the generator and implement the Flutter API that is generated. The implementation goes in lib/src/android_webview_api_impl.dart
.
Then in Java you'll need to override the onScrollChanged
method in the WebViewPlatformView
class found in WebViewHostApiImpl.java
class. You can then instantiate a WebViewFlutterApi
and call the WebViewFlutterApi.onScrollChanged
method in this overridden method.
I may have missed a few things in this explanation, so definitely feel free to ask me for more clarification. The plugin is still suffering from tech debt, so adding new features still requires more work than they will in the future. I can also contribute code to this PR if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. I did as you said but even though it helped to make things simple I still end up needing to retain WebViewExtendedApi
.
@@ -1,3 +1,7 @@ | |||
## 3.2.3 | |||
|
|||
* Added `setScrollListener` method to the `AndroidWebViewController`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ios can also support listening to scroll events, so we should add the method to the platform interface and implement for both platforms. This will probably change to something like "Implements support for PlatformWebViewController.setOnScrollChange
".
@@ -259,6 +259,12 @@ abstract class PlatformWebViewController extends PlatformInterface { | |||
throw UnimplementedError( | |||
'setUserAgent is not implemented on the current platform'); | |||
} | |||
|
|||
/// Toggle the listener for scroll position | |||
Future<void> setScrollListener(void Function(int x, int y)? onOffsetChange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To follow the current convention of the interface, this should probably be setOnScrollChanged(...)
or setOnContentOffsetChanged(...)
. I'm impartial to either of these.
Sadly, I don't think this feature is currently implementable in the web, since there's no communication mechanism between the iframe that is being injected and the flutter web app. |
I think I can do it. Can I open a new PR for the iOS implementation? |
This PR should follow https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins and include the iOS implementation in this PR. Then once this has been approval, you can split the code from each package into their own PRs. |
7fbf579
to
f0755b5
Compare
@bparrishMines Thanks for your feedback. Could you please take a look again at Android implementations for this? If no problems arise I will apply the same logic for the iOS side. |
We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages. Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord. Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state! |
Description
Adding scroll listener and ability to scroll programmatically in Android.
Related Issues
flutter/flutter#42442
flutter/flutter#31027
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?