[webview_flutter] Add listener for content offset

[TheVinhLuong]

Description

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

Related Issues

https://github.com/flutter/flutter/issues/31027

Pre-launch Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [x] 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.)
• [x] I signed the CLA.
• [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making, or this PR is test-exempt.
• [x] All existing and new tests are passing.

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

[TheVinhLuong]

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.

[reidbaker]

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]

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]

@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?

[TheVinhLuong]

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

[TheVinhLuong]

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

[TheVinhLuong]

Hi @stuartmorgan, I updated the PR but have 1 failing test case here. Just want to confirm that I need to split this PR to 2 PRs (each for webview_flutter_platform_interface and webview_flutter_android) right?

[TheVinhLuong]

This generally looks good to me. (besides the name change).

This is still missing the iOS portion though. Are you able to add this part or would you like for me to handle it?

I think I will be able to add the iOS portion. Please let me do it.

[gmackall]

Thanks for your contribution! Since we haven't seen a reply, I'm going to reluctantly close this pull request for now, but if you get time to address the comments, please feel free to re-open this PR or submit a new one! Also please reach out on the community discord if you need help!

[TheVinhLuong]

@gmackall I still actively working on this so can you reopen this? You must misread since I did have a reply back then.

[TheVinhLuong]

Hi @cyanglaz, regarding my iOS implementation commit, it is still not ready yet. I pushed my commit because I have an issue that I would like to ask for your opinion on: In this line, I have to add a delay before setting a delegate to the UIScrollView in order for it to work. Otherwise, the scrollViewDidScroll method will not be called on the iOS side. Do you know why this is happening and where would be a suitable place for me to call it?

[cyanglaz]

Hi @cyanglaz, regarding my iOS implementation commit, it is still not ready yet. I pushed my commit because I have an issue that I would like to ask for your opinion on: In this line, I have to add a delay before setting a delegate to the UIScrollView in order for it to work. Otherwise, the scrollViewDidScroll method will not be called on the iOS side. Do you know why this is happening and where would be a suitable place for me to call it?

Sorry, I'm actually not familiar with this code. Maybe @bparrishMines knows?

[bparrishMines]

@TheVinhLuong Needing a delay does seem weird. It could have to do something with an async object creation. Are you seeing any error messages? Could you verify the Objective-C code setting the delegate is not being called on a null object?

[TheVinhLuong]

@TheVinhLuong Needing a delay does seem weird. It could have to do something with an async object creation. Are you seeing any error messages? Could you verify the Objective-C code setting the delegate is not being called on a null object?

@bparrishMines I have debugged multiple times on the iOS side and can confirm that neither UIScrollView nor UIScrollViewDelegate are null at the time of delegation setting, not even once.

Do you think the reason it is not working is because of the timing? In all examples of setting the delegate for a scrollview I have seen, it is done within UIViewController's viewDidLoad method.

[bparrishMines]

I think I see the issue. The UIScrollView only has a weak reference to the UIScrollViewDelegate:

https://developer.apple.com/documentation/uikit/uiscrollview/1619430-delegate?language=objc

There's a good chance it could be getting garbage collected based on the timing and other factors. You could try to solve this by saving the instance of UIScrollViewDelegate in WebKitWebViewController.

[TheVinhLuong]

I think I see the issue. The UIScrollView only has a weak reference to the UIScrollViewDelegate:

https://developer.apple.com/documentation/uikit/uiscrollview/1619430-delegate?language=objc

There's a good chance it could be getting garbage collected based on the timing and other factors. You could try to solve this by saving the instance of UIScrollViewDelegate in WebKitWebViewController.

@bparrishMines Thanks for the insight, that solves the problem. Could you please review my iOS implementations?

[TheVinhLuong]

@bparrishMines Please review it again. thanks! There are a few checks failing which I don't think related to the code I added.

[stuartmorgan]

There are a few checks failing which I don't think related to the code I added.

Those are indeed a tree breakage we're working to resolve right now.

[bparrishMines]

@TheVinhLuong Merging in main now should fix the failing checks.

[TheVinhLuong]

@TheVinhLuong Merging in main now should fix the failing checks.

@bparrishMines Yes, it is ok now. There is 1 check fail though, should I need to split this PR into 2 PRs as instructed here? If that's the case, would it be appropriate to proceed with the split at this time?

[TheVinhLuong]

@bparrishMines I pushed a commit to fix failed tests on the iOS platform. There is a failed test which I couldn't recreate on my local machine, can you look into it?

[bparrishMines]

@TheVinhLuong I merged in the latest changes and it looks like the test stop failing. It may have been caused by a regression in the flutter/flutter repo. This PR should now be ready to be broken up. You should be able to create a separate PR with just the changes in webview_flutter_platform_inteface.

[jmagman]

Has there been progress made on breaking this up? Is more guidance needed?

[bparrishMines]

This is the aggregate PR still waiting on the implementations PR: https://github.com/flutter/packages/pull/5664

[TheVinhLuong]

@stuartmorgan Please review it again, thanks.