Skip to content

Conversation

dmeijboom
Copy link

In flutters webview package you can set a navigation delegate which is triggered when the user clicks on a link in the webview. This can be used to load these pages in an in-app browser or external browser for example but I can imagine it has other use-cases as well.

On Android this seems to work well but on iOS the delegate is triggered onload without any user action involved. It seems like this is an implementation issue. Currently, we the decidePolicyForNavigationAction handler is used to trigger the navigation delegate in flutter. This is fine but it seems like this handler is more generic on iOS and is also triggered whenever an URL is loaded. The fix has been discussed already:

See: flutter/flutter#73242 (comment)

Now that flutter/plugins#6863 has added support for WKNavigationAction.navigationType we can actually solve this issue by excluding other navigation types. This is what this PR does, it excludes the other navigation type so that both Android and iOS behave similarly.

Issue: flutter/flutter#73242

Pre-launch Checklist

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

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

@dmeijboom dmeijboom requested a review from cyanglaz as a code owner October 6, 2023 09:33
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@bparrishMines
Copy link
Contributor

I was considering doing this, but I decided against it to avoid requiring a breaking change for the long term solution.

iOS provides the WKNavigationType and Android provides [WebResourceRequest.hasGesture]. I think it would be better to update the platform interface to provide a NavigationType in NavigationRequest.

If you would be interested in implementing this, you can follow https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins. Otherwise we can close this PR.

@dmeijboom
Copy link
Author

I was considering doing this, but I decided against it to avoid requiring a breaking change for the long term solution.

iOS provides the WKNavigationType and Android provides [WebResourceRequest.hasGesture]. I think it would be better to update the platform interface to provide a NavigationType in NavigationRequest.

If you would be interested in implementing this, you can follow https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins. Otherwise we can close this PR.

I guess it makes sense. Even though it's probably 'fine' for most apps it would be a breaking change which can be easily avoided by passing the navigation type. We will make sure this gets implemented but I don't have a concrete timeline yet.

@MitchellGoodwin MitchellGoodwin marked this pull request as draft October 23, 2023 19:30
@MitchellGoodwin
Copy link

Marking this as a draft pending the changes discussed above.

@stuartmorgan-g
Copy link
Collaborator

Since this is marked as a draft and hasn't been updated in several months I'm going to close it to clean out our review queue. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants