[webview_flutter] Add listener for content offset #3444

TheVinhLuong · 2023-03-11T16:50:50Z

Description

This PR is created from the previous PR in flutter/plugin

Related Issues

Pre-launch Checklist

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

TheVinhLuong · 2023-03-11T16:59:27Z

HI @bparrishMines, could you review my PR? Currently, this is an implementation for Android side only. I want to make sure the way the listener was added is correct before landing an additional commit with similar changes for the iOS.

packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart

reidbaker · 2023-03-30T18:26:01Z

Thank you for your contribution. It looks like you have some open comments and merge conflicts. Is this still something you plan to get to? If you are unsure how to proceed, please reach out for help on the #hackers-new channel.

TheVinhLuong · 2023-04-01T09:08:16Z

Thank you for your contribution. It looks like you have some open comments and merge conflicts. Is this still something you plan to get to? If you are unsure how to proceed, please reach out for help on the #hackers-new channel.

@reidbaker Ah yes, I still plan to continue working on it.

TheVinhLuong · 2023-04-01T17:24:58Z

@bparrishMines I think I get your idea. Could you take a look again at this commit to see if it is done as you want?

The reason why I created enableContentOffsetChangedListener in the first place is because I don't want the offset listener to abuse the platform channel unnecessarily when the user doesn't set the listener. The offset changed callback will be called on many times so I think it could put a burden on the platform channel.

Please correct me if I am wrong but with the way you wanted to implement this offset callback the platform channel will always need to carry the offset change events to the flutter side even if the users don't set the listener for it.

So do you think we should retain the enableContentOffsetChangedListener after all?

bparrishMines

@bparrishMines I think I get your idea. Could you take a look again at this commit to see if it is done as you want?

Yes this is the implementation that I was thinking. I would just update the name to match the java method name. Thanks for updating this.

The reason why I created enableContentOffsetChangedListener in the first place is because I don't want the offset listener to abuse the platform channel unnecessarily when the user doesn't set the listener. The offset changed callback will be called on many times so I think it could put a burden on the platform channel.

You're correct that this callback will lead to unnecessary use of the platform channel. This is true for every callback and I plan on addressing this in another PR. If you don't notice performance being affected by this, then I don't think it is necessary to change the design of this callback preemptively.

bparrishMines · 2023-04-05T16:53:23Z

packages/webview_flutter/webview_flutter_android/pigeons/android_webview.dart

@@ -211,6 +211,9 @@ abstract class WebViewHostApi {
 abstract class WebViewFlutterApi {
  /// Create a new Dart instance and add it to the `InstanceManager`.
  void create(int identifier);
+
+  void onScrollPosChange(


This should be onScrollChanged since it's the name of the Java method: https://developer.android.com/reference/android/webkit/WebView#onScrollChanged(int,%20int,%20int,%20int)

You've marked this a resolved, but the name hasn't been changed.

My bad, I overlooked this.

bparrishMines · 2023-04-05T16:53:59Z

...r_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterApiImpl.java

-  // To ease adding additional methods, this value is added prematurely.
-  @SuppressWarnings({"unused", "FieldCanBeLocal"})
-  private final BinaryMessenger binaryMessenger;


These three lines need to be kept

...r_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterApiImpl.java

...tter_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewHostApiImpl.java

packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart

packages/webview_flutter/webview_flutter_android/lib/src/android_webview_api_impls.dart

packages/webview_flutter/webview_flutter_android/lib/src/android_webview_controller.dart

bparrishMines · 2023-04-05T17:13:19Z

packages/webview_flutter/webview_flutter_android/test/android_webview_controller_test.dart

@@ -868,6 +875,16 @@ void main() {
      verify(mockWebView.settings).called(1);
      verify(mockSettings.setUserAgentString('Test Framework')).called(1);
    });
+
+    test('setScrollListener', () async {


nit:

Suggested change

test('setScrollListener', () async {

test('setOnContentOffsetChanged', () async {

I decided to remove this test and instead add the integration test since this would need an actual WebView instance.

TheVinhLuong · 2023-04-14T10:58:21Z

@bparrishMines Thanks for your thorough review. I fix it up based on your comment, please review it again.

bparrishMines

This should also pull in the latest version of main. The packages have gone through some significant changes since the last review.

packages/webview_flutter/webview_flutter_android/CHANGELOG.md

...iew_flutter_android/android/src/test/java/io/flutter/plugins/webviewflutter/WebViewTest.java

...s/webview_flutter/webview_flutter_android/example/integration_test/webview_flutter_test.dart

packages/webview_flutter/webview_flutter_android/lib/src/android_webview_controller.dart

.../webview_flutter/webview_flutter_platform_interface/lib/src/platform_webview_controller.dart

TheVinhLuong · 2023-04-29T13:23:03Z

Hi @bparrishMines, could you please review this commit, thank you!

stuartmorgan

Please review all the repo_test failures; there are a number of issues it's caught.

packages/webview_flutter/webview_flutter_android/CHANGELOG.md

...r_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterApiImpl.java

...tter_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewHostApiImpl.java

stuartmorgan · 2023-05-15T00:30:53Z

packages/webview_flutter/webview_flutter_android/pigeons/android_webview.dart

@@ -211,6 +211,9 @@ abstract class WebViewHostApi {
 abstract class WebViewFlutterApi {
  /// Create a new Dart instance and add it to the `InstanceManager`.
  void create(int identifier);
+
+  void onScrollPosChange(


You've marked this a resolved, but the name hasn't been changed.

…id API closely

bparrishMines · 2024-01-18T00:46:12Z

This is the aggregate PR still waiting on the implementations PR: #5664

… `setOnScrollPositionChange` for webview_flutter platform implementations (#5664) Adds iOS and Android implementation for content offset listener This PR is part of a series of PRs that aim to close flutter/flutter#31027. The PR that contains all changes can be found at #3444.

…pdate app-facing package's CHANGELOG

packages/webview_flutter/webview_flutter/CHANGELOG.md

packages/webview_flutter/webview_flutter/example/ios/Runner.xcodeproj/project.pbxproj

..._flutter/webview_flutter/example/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme

packages/webview_flutter/webview_flutter/example/pubspec.yaml

TheVinhLuong · 2024-02-09T17:35:15Z

@stuartmorgan Please review it again, thanks.

stuartmorgan

LGTM, thanks!

…ckages#3444)

flutter/packages@11152d2...0a69259 2024-02-09 everythingoutdated@gmail.com [local_auth]: Renamed `local_auth_ios` to `local_auth_darwin` (flutter/packages#5809) 2024-02-09 ltv.luongthevinh@gmail.com [webview_flutter] Add listener for content offset (flutter/packages#3444) 2024-02-09 David.Chopin@wwt.com [go_router] Expose full `Uri` on `GoRouterState` in `GoRouterRedirect` (flutter/packages#5742) 2024-02-09 stuartmorgan@google.com [webview_flutter] Minor test cleanup (flutter/packages#6031) 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

flutter#5427) Adds the platform interface implementation for content offset listener This PR is part of a series of PRs that aim to close flutter/flutter#31027. The PR that contains all changes can be found at flutter#3444.

… `setOnScrollPositionChange` for webview_flutter platform implementations (flutter#5664) Adds iOS and Android implementation for content offset listener This PR is part of a series of PRs that aim to close flutter/flutter#31027. The PR that contains all changes can be found at flutter#3444.

## Description This PR is created from [the previous PR in flutter/plugin](flutter/plugins#2107) ## Related Issues flutter/flutter#31027

… `setOnScrollPositionChange` for webview_flutter platform implementations (#5664) Adds iOS and Android implementation for content offset listener This PR is part of a series of PRs that aim to close flutter/flutter#31027. The PR that contains all changes can be found at flutter/packages#3444.

Recreating PR from flutter/plugins

dd1196d

TheVinhLuong requested a review from bparrishMines as a code owner March 11, 2023 16:50

github-actions bot added p: webview_flutter platform-android labels Mar 11, 2023

Merge branch 'master' into webview-scroll-listener

d91b421

TheVinhLuong force-pushed the webview-scroll-listener branch from e22ed48 to d91b421 Compare March 11, 2023 17:12

bparrishMines requested changes Mar 15, 2023

View reviewed changes

packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart Outdated Show resolved Hide resolved

TheVinhLuong added 2 commits April 2, 2023 00:11

[webview_android] Remove enableContentOffsetChangedListener

2a9b827

Merge branch 'master' into webview-scroll-listener

07d30ef

TheVinhLuong requested a review from bparrishMines April 1, 2023 17:27

bparrishMines reviewed Apr 5, 2023

View reviewed changes

Fixup based on code review

5a80183

TheVinhLuong requested a review from bparrishMines April 14, 2023 10:55

bparrishMines reviewed Apr 18, 2023

View reviewed changes

TheVinhLuong added 3 commits April 29, 2023 19:51

Merge branch 'main' into webview-scroll-listener

1ea39bc

Resolve conflict

e6a5c55

Fixup based on code review

024e3a7

TheVinhLuong force-pushed the webview-scroll-listener branch from 5c4dfc2 to 024e3a7 Compare April 29, 2023 13:18

TheVinhLuong requested a review from bparrishMines April 29, 2023 13:23

stuartmorgan requested changes May 15, 2023

View reviewed changes

Merge branch 'main' into webview-scroll-listener

af3358b

TheVinhLuong force-pushed the webview-scroll-listener branch 2 times, most recently from c0bf3ef to 0c2fde1 Compare May 30, 2023 15:47

Make dart low-level code for offset changes listener mirror the Andro…

c7b5231

…id API closely

github-actions bot removed platform-android platform-ios labels Feb 9, 2024

TheVinhLuong force-pushed the webview-scroll-listener branch from c19a04a to 919e7b9 Compare February 9, 2024 08:35

github-actions bot added the platform-ios label Feb 9, 2024

TheVinhLuong force-pushed the webview-scroll-listener branch from 919e7b9 to 97ace1f Compare February 9, 2024 08:43

Merge branch 'main' into webview-scroll-listener

0d8668b

TheVinhLuong force-pushed the webview-scroll-listener branch 3 times, most recently from aa78e69 to a319981 Compare February 9, 2024 13:38

Updates minimum supported SDK version to Flutter 3.16.6/Dart 3.2.3. U…

a2400d2

…pdate app-facing package's CHANGELOG

TheVinhLuong force-pushed the webview-scroll-listener branch from a319981 to a2400d2 Compare February 9, 2024 14:11

stuartmorgan requested changes Feb 9, 2024

View reviewed changes

Fixup based on code review

7f8d4c6

github-actions bot removed the platform-ios label Feb 9, 2024

Merge branch 'main' into webview-scroll-listener

3ea68d8

TheVinhLuong requested a review from stuartmorgan February 9, 2024 17:34

stuartmorgan approved these changes Feb 9, 2024

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 9, 2024

auto-submit bot merged commit b58b33c into flutter:main Feb 9, 2024
78 checks passed

engine-flutter-autoroll mentioned this pull request Feb 12, 2024

Roll Packages from 11152d2bc8f3 to 0a692590a885 (4 revisions) flutter/flutter#143306

Merged

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

b58b33c [webview_flutter] Add listener for content offset (flutter/pa…

b8dae10

…ckages#3444)

reidbaker mentioned this pull request Feb 19, 2024

[in_app_purchase_android] Add UserChoiceBilling mode. #6162

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter] Add listener for content offset #3444

[webview_flutter] Add listener for content offset #3444

TheVinhLuong commented Mar 11, 2023 •

edited

Loading

TheVinhLuong commented Mar 11, 2023

reidbaker commented Mar 30, 2023

TheVinhLuong commented Apr 1, 2023 •

edited

Loading

TheVinhLuong commented Apr 1, 2023 •

edited

Loading

bparrishMines left a comment

bparrishMines Apr 5, 2023

stuartmorgan May 15, 2023

TheVinhLuong May 28, 2023

bparrishMines Apr 5, 2023

bparrishMines Apr 5, 2023

TheVinhLuong Apr 14, 2023

TheVinhLuong commented Apr 14, 2023 •

edited

Loading

bparrishMines left a comment

TheVinhLuong commented Apr 29, 2023

stuartmorgan left a comment

stuartmorgan May 15, 2023

bparrishMines commented Jan 18, 2024

TheVinhLuong commented Feb 9, 2024

stuartmorgan left a comment

	test('setScrollListener', () async {
	test('setOnContentOffsetChanged', () async {

[webview_flutter] Add listener for content offset #3444

[webview_flutter] Add listener for content offset #3444

Conversation

TheVinhLuong commented Mar 11, 2023 • edited Loading

Description

Related Issues

Pre-launch Checklist

TheVinhLuong commented Mar 11, 2023

reidbaker commented Mar 30, 2023

TheVinhLuong commented Apr 1, 2023 • edited Loading

TheVinhLuong commented Apr 1, 2023 • edited Loading

bparrishMines left a comment

Choose a reason for hiding this comment

bparrishMines Apr 5, 2023

Choose a reason for hiding this comment

stuartmorgan May 15, 2023

Choose a reason for hiding this comment

TheVinhLuong May 28, 2023

Choose a reason for hiding this comment

bparrishMines Apr 5, 2023

Choose a reason for hiding this comment

bparrishMines Apr 5, 2023

Choose a reason for hiding this comment

TheVinhLuong Apr 14, 2023

Choose a reason for hiding this comment

TheVinhLuong commented Apr 14, 2023 • edited Loading

bparrishMines left a comment

Choose a reason for hiding this comment

TheVinhLuong commented Apr 29, 2023

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan May 15, 2023

Choose a reason for hiding this comment

bparrishMines commented Jan 18, 2024

TheVinhLuong commented Feb 9, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

TheVinhLuong commented Mar 11, 2023 •

edited

Loading

TheVinhLuong commented Apr 1, 2023 •

edited

Loading

TheVinhLuong commented Apr 1, 2023 •

edited

Loading

TheVinhLuong commented Apr 14, 2023 •

edited

Loading