[webview_flutter_android][webview_flutter_wkwebview] Adds support for `setOnScrollPositionChange` for webview_flutter platform implementations #5664

TheVinhLuong · 2023-12-13T14:35:09Z

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.

Pre-launch Checklist

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

…id API closely

…l-listener

TheVinhLuong · 2023-12-17T09:59:31Z

Hi @bparrishMines, could you please review and merge it?

hellohuanlin

reviewed ios part

hellohuanlin · 2023-12-18T18:52:00Z

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

+// references.
+@property(nonatomic, weak) id<FlutterBinaryMessenger> binaryMessenger;
+// InstanceManager must be weak to prevent a circular reference with the object it stores.
+@property(nonatomic, weak) FWFInstanceManager *instanceManager;


can you explain more about this circular reference?

Sorry, but that comment is actually not come up by me, you can see that comment throughout the code base wherever the property of FWFInstanceManager is declared, I just followed the convention.

However, from my understanding, it is a safety measure to prevent a memory leak in case the instance of FWFInstanceManager is used to store the object that contains it (circular relation).

@hellohuanlin This is the pattern for all the api implementations in the plugin. The full reasoning is that the ScrollViewDelegate instance contains an instance of the ScrollViewDelegateFlutterApi. And the ScrollViewDelegateFlutterApi contains an instance of the FWFInstanceManager. And the FWFInstanceManager contains an instance of the ScrollViewDelegate.

ScrollViewDelegate -> FlutterApi -> InstanceManager -> ScrollViewDelegate.

I'm currently working on flutter/flutter#134777 to eventually make this a part of generated code and not copy pasted.

hellohuanlin · 2023-12-18T18:53:30Z

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

+                          uiScrollView:(UIScrollView *)scrollView
+                            completion:(void (^)(FlutterError *_Nullable))completion {
+  [self scrollViewDidScrollWithIdentifier:[self identifierForDelegate:instance]
+                   uiScrollViewIdentifier:[self.instanceManager


if instanceManager is weak, do you want to deal with nil case?

I don't think instanceManager can be nil here. Again, I just followed the pattern of other host API was implemented before me.

The reason why I think instanceManager couldn't be nil is because it is strongly referenced by the plugin here

instanceManager can be null in an add-to-app scenario. However, Objective-c will just return nil and not cause a crash. This is the desired behavior since the plugin holds the only strong reference and we want this to be null when that changes. I will make this clearer when flutter/flutter#134777 is implemented in this language.

hellohuanlin · 2023-12-18T18:55:24Z

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

+  [self.scrollViewDelegateAPI scrollViewDidScrollForDelegate:self
+                                                uiScrollView:scrollView
+                                                  completion:^(FlutterError *error) {
+                                                    NSAssert(!error, @"%@", error);


should we pipe the error to flutter side?

As I understand, the completion callback here is for the native side to handle the error pipe from the flutter side, so I don't think it is logical to passing it back to the flutter side. Besides, every callback that was implemented before me is doing the same, I just followed the convention.

hellohuanlin · 2023-12-18T18:56:05Z

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

+  if (self) {
+    _scrollViewDelegateAPI =
+        [[FWFScrollViewDelegateFlutterApiImpl alloc] initWithBinaryMessenger:binaryMessenger
+                                                             instanceManager:instanceManager];


this seems to be just a 1-line wrapper. can FWFScrollViewDelegate itself conform to FWFScrollViewDelegateFlutterApi?

I believe it's possible, but I adhered to the conventions established by the previously implemented API, such as the FWFUIDelegate.

The reason this pattern doesn't have the wrapped class implement their own FlutterApi is because it wouldn't work well with inheritance. For example if we had

class A { } class B implements A { } interface AFlutterApi { } interface BFlutterApi { }

class B would need access to an implementation of AFlutterApi and BFlutterApi. So, there wouldn't be an easy way to do this if we did class A implements AFlutterApi.

Note that this will also be solved by flutter/flutter#134777 by handling inherited flutter api automatically.

TheVinhLuong · 2023-12-23T10:55:45Z

@hellohuanlin Thank you for your review! By the way, this PR of mine is extracted from my PR here, which has already been reviewed and approved by 2 people. That is why, in my opinion, it is a waste of effort to be thoroughly reviewed again. What I think should be reviewed here is that are there any difference between the code change here compared to the PR that contains a full changes (including platform interface,...)

…l-listener-platforms

bparrishMines

LGTM

@hellohuanlin This code was already reviewed in #3444 and follows the design from https://docs.google.com/document/d/1wXB1zNzYhd2SxCu1_BK3qmNWRhonTB6qdv4erdtBQqo/edit?usp=sharing&resourcekey=0-WOBqqOKiO9SARnziBg28pg. Let me know if you have questions about it.

cc @stuartmorgan

bparrishMines · 2024-01-05T20:47:37Z

I merged in the latest changes from the main branch, but this is still blocked by an ifra error failing the tests.

TheVinhLuong · 2024-01-09T18:26:13Z

I just changed the unit test a bit.

bparrishMines · 2024-01-18T00:48:26Z

@stuartmorgan This is ready for a secondary review.

@hellohuanlin You can secondary review the iOS portion too if you would like.

I will handle the version bump once this is done.

hellohuanlin · 2024-01-18T02:20:35Z

...lutter/webview_flutter_wkwebview/example/ios/RunnerTests/FWFScrollViewDelegateHostApiTests.m

+  FWFScrollViewDelegateFlutterApiImpl *flutterAPI = [[FWFScrollViewDelegateFlutterApiImpl alloc]
+      initWithBinaryMessenger:OCMProtocolMock(@protocol(FlutterBinaryMessenger))
+              instanceManager:instanceManager];
+  return OCMPartialMock(flutterAPI);


i'd just inline these 2 helpers.

stuartmorgan

LGTM with nit

stuartmorgan · 2024-01-24T21:22:44Z

packages/webview_flutter/webview_flutter_wkwebview/pigeons/web_kit.dart

+@FlutterApi()
+abstract class UIScrollViewDelegateFlutterApi {
+  @ObjCSelector(
+    'scrollViewDidScrollWithIdentifier:uiScrollViewIdentifier:x:y:',


Nit: ObjC naming should be ...:UIScrollViewIdentifier:x:y:. Capitalizing acronyms trumps first letter being lower case for camel case in Obj-C naming conventions.

Lemme fix that.

jmagman · 2024-01-31T21:52:56Z

@TheVinhLuong can you fix up the conflicts, including the CHANGELOG and pubspec version? Once @stuartmorgan's UIScrollViewIdentifier issue is addressed this will be ready to merge!

Adhere to ObjC naming Update AUTHORS

TheVinhLuong · 2024-02-07T07:07:09Z

@TheVinhLuong can you fix up the conflicts, including the CHANGELOG and pubspec version? Once @stuartmorgan's UIScrollViewIdentifier issue is addressed this will be ready to merge!

I fixed up the code conflict. Also, the UIScrollViewIdentifier issue also addressed.

…port for `setOnScrollPositionChange` for webview_flutter platform implementations (flutter/packages#5664)

flutter/packages@e4ea6bf...29d8cc0 2024-02-07 td@technodisaster.com [local_auth] fix: isDeviceSupported on ios (flutter/packages#5125) 2024-02-07 jason-simmons@users.noreply.github.com [image_picker] Suppress analyzer warning about deprecation of UnmodifiableUint8ListView (flutter/packages#6076) 2024-02-07 58767497+GiftShower@users.noreply.github.com Add transitionDuration parameter (flutter/packages#5854) 2024-02-07 engine-flutter-autoroll@skia.org Roll Flutter from e6ba809 to 8431cae (23 revisions) (flutter/packages#6075) 2024-02-07 ltv.luongthevinh@gmail.com [webview_flutter_android][webview_flutter_wkwebview] Adds support for `setOnScrollPositionChange` for webview_flutter platform implementations (flutter/packages#5664) 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

… `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.

TheVinhLuong and others added 30 commits March 11, 2023 22:21

Recreating PR from flutter/plugins

dd1196d

Merge branch 'master' into webview-scroll-listener

d91b421

[webview_android] Remove enableContentOffsetChangedListener

2a9b827

Merge branch 'master' into webview-scroll-listener

07d30ef

Fixup based on code review

5a80183

Merge branch 'main' into webview-scroll-listener

1ea39bc

Resolve conflict

e6a5c55

Fixup based on code review

024e3a7

Merge branch 'main' into webview-scroll-listener

af3358b

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

c7b5231

…id API closely

Add iOS implementation

44ca259

Merge branch 'main' into webview-scroll-listener

726d1a8

Add iOS related unit test codes

ac8eacc

Merge branch 'main' into webview-scroll-listener

59fa39d

Fix CI failing checks

cfbdee9

Merge branch 'main' into webview-scroll-listener

f24f661

Apply changes based on review comments

562bd04

Merge branch 'main' into webview-scroll-listener

1b5e9d3

Fix code based on code review

d256a35

Merge branch 'main' into webview-scroll-listener

df20af9

Merge branch 'main' into webview-scroll-listener

3c2a2ca

Merge branch 'main' of github.com:flutter/packages into webview-scrol…

0c6aac1

…l-listener

some improvements

ac9dcee

switch to doubles

5dfa5b2

update webview_flutter

94eed4b

formatting

b0153f9

Merge branch 'main' of github.com:flutter/packages into webview-scrol…

a37dc83

…l-listener

pubspec ordering and doc

4348c1b

Merge branch 'main' of github.com:flutter/packages into webview-scrol…

48aa520

…l-listener

PR feedback

02a4758

TheVinhLuong added 2 commits December 17, 2023 16:31

Merge branch 'main' into webview-scroll-listener-platforms

066155a

Fix wrong formatting

36e176c

hellohuanlin reviewed Dec 18, 2023

View reviewed changes

TheVinhLuong requested a review from hellohuanlin December 23, 2023 10:39

bparrishMines added 3 commits January 4, 2024 13:04

Merge branch 'main' of github.com:flutter/packages into webview-scrol…

a435962

…l-listener-platforms

Merge branch 'main' of github.com:flutter/packages into webview-scrol…

a67f520

…l-listener-platforms

update lint changes

7a9f6a6

bparrishMines approved these changes Jan 5, 2024

View reviewed changes

stuartmorgan and others added 2 commits January 8, 2024 07:50

Merge branch 'main' into webview-scroll-listener-platforms

8e7f8e9

Change expect logic for OnScollPositionChange

062d19e

bparrishMines mentioned this pull request Jan 18, 2024

[webview_flutter] Add listener for content offset #3444

Merged

11 tasks

hellohuanlin approved these changes Jan 18, 2024

View reviewed changes

stuartmorgan approved these changes Jan 24, 2024

View reviewed changes

TheVinhLuong force-pushed the webview-scroll-listener-platforms branch from 3b75f43 to bb67922 Compare February 3, 2024 10:50

Merge branch 'main' into webview-scroll-listener-platforms

72a9988

Adhere to ObjC naming Update AUTHORS

TheVinhLuong force-pushed the webview-scroll-listener-platforms branch from bb67922 to 72a9988 Compare February 3, 2024 11:07

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

auto-submit bot merged commit 0c2473f into flutter:main Feb 7, 2024
78 checks passed

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

Roll Packages from e4ea6bf72e1b to 29d8cc006b0e (5 revisions) flutter/flutter#143161

Merged

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_android][webview_flutter_wkwebview] Adds support for `setOnScrollPositionChange` for webview_flutter platform implementations #5664

[webview_flutter_android][webview_flutter_wkwebview] Adds support for `setOnScrollPositionChange` for webview_flutter platform implementations #5664

TheVinhLuong commented Dec 13, 2023

TheVinhLuong commented Dec 17, 2023

hellohuanlin left a comment

hellohuanlin Dec 18, 2023

TheVinhLuong Dec 23, 2023

bparrishMines Jan 4, 2024

hellohuanlin Dec 18, 2023

TheVinhLuong Dec 23, 2023

bparrishMines Jan 5, 2024

hellohuanlin Dec 18, 2023

TheVinhLuong Dec 23, 2023

hellohuanlin Dec 18, 2023

TheVinhLuong Dec 23, 2023

bparrishMines Jan 5, 2024

TheVinhLuong commented Dec 23, 2023

bparrishMines left a comment

bparrishMines commented Jan 5, 2024

TheVinhLuong commented Jan 9, 2024

bparrishMines commented Jan 18, 2024

hellohuanlin Jan 18, 2024

stuartmorgan left a comment

stuartmorgan Jan 24, 2024

TheVinhLuong Feb 3, 2024

jmagman commented Jan 31, 2024

TheVinhLuong commented Feb 7, 2024

[webview_flutter_android][webview_flutter_wkwebview] Adds support for setOnScrollPositionChange for webview_flutter platform implementations #5664

[webview_flutter_android][webview_flutter_wkwebview] Adds support for setOnScrollPositionChange for webview_flutter platform implementations #5664

Conversation

TheVinhLuong commented Dec 13, 2023

Pre-launch Checklist

TheVinhLuong commented Dec 17, 2023

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheVinhLuong commented Dec 23, 2023

bparrishMines left a comment

Choose a reason for hiding this comment

bparrishMines commented Jan 5, 2024

TheVinhLuong commented Jan 9, 2024

bparrishMines commented Jan 18, 2024

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman commented Jan 31, 2024

TheVinhLuong commented Feb 7, 2024

[webview_flutter_android][webview_flutter_wkwebview] Adds support for `setOnScrollPositionChange` for webview_flutter platform implementations #5664

[webview_flutter_android][webview_flutter_wkwebview] Adds support for `setOnScrollPositionChange` for webview_flutter platform implementations #5664