[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

StanleyCocos · 2025-02-07T06:43:58Z

When SFSafariViewController is loading a webpage, if the close button is clicked during the loading process, it can prevent the callback from being triggered correctly.

List which issues are fixed by this PR. You must list at least one issue.

Fixes Bug Report: CupertinoListTile Remains backgroundColorActivated After launchUrl(url) flutter#161790
Fixes [url_launcher] Result doesn't fire on early dismissal (iOS) flutter#47271

Pre-launch Checklist

…allback to not be triggered correctly.

flutter-dashboard · 2025-02-07T06:44:01Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

stuartmorgan-g · 2025-02-07T13:15:50Z

This should be testable with native unit tests that directly drive calls to the URLLaunchSession.

stuartmorgan-g · 2025-02-07T13:16:47Z

...uncher/url_launcher_ios/ios/url_launcher_ios/Sources/url_launcher_ios/URLLaunchSession.swift

@@ -12,6 +12,7 @@ final class URLLaunchSession: NSObject, SFSafariViewControllerDelegate {

  private let completion: OpenInSafariCompletionHandler
  private let url: URL
+  private var isLoadCompleted: Bool?


A nullable boolean should only be used when there are three conceptually different states. The code in this PR only actually recognizes two.

stuartmorgan-g · 2025-02-08T12:11:20Z

That's a Dart unit test, which does not run the code you changed; native unit tests are here, as described in our contributor docs.

stuartmorgan-g · 2025-02-08T22:12:58Z

(Marking as Draft pending the test, so it doesn't show up in review queues.)

StanleyCocos · 2025-02-10T03:19:46Z

Oh, my God. I forgot. Thanks for the reminder. I’ve added the test. If you have time, you can continue review

packages/url_launcher/url_launcher_ios/CHANGELOG.md

loic-sharma · 2025-02-12T23:46:46Z

...uncher/url_launcher_ios/ios/url_launcher_ios/Sources/url_launcher_ios/URLLaunchSession.swift

  }

  /// Called when the user finishes using the Safari view controller.
  ///
  /// - Parameter controller: The Safari view controller.
  func safariViewControllerDidFinish(_ controller: SFSafariViewController) {
+    if !isLoadCompleted {
+      completion(.success(.failedToLoad))


Is .failedToLoad the correct result here? Or we should add a new InAppLoadResult.dismissed result?

If this is the correct result, should we update this comment to indicate this case can happen if the user dismisses the Safari view before the initial load completes?

packages/packages/url_launcher/url_launcher_ios/pigeons/messages.dart

Lines 30 to 31 in fd53793

/// The URL did not load successfully.

failedToLoad,

At first, I did consider adding a new InAppLoadResult to describe the close action, but it seems it could lead to confusion among developers. This is because, currently, after a successful load, clicking the close button does not trigger any callback.

Additionally, when the user clicks close before the loading is complete, triggering the failedToLoad callback seems reasonable in a broader sense, as the interruption of the loading process can be considered a loading failure.

If this is the correct result, should we update this comment to indicate this case can happen if the user dismisses the Safari view before the initial load completes?

Do you think it’s possible to modify the comment like this:

// The URL did not load successfully (or close the SFSafariViewController earlier)

Returning this will throw an exception saying there was an error loading the page, which seems misleading. I think a new Pigeon return type that Dart interprets to return false would be better.

I created a new dismissed state to represent the current status.

cc @stuartmorgan
Could you take another look?

… the callback to not be triggered correctly. (flutter/packages#8582)

flutter/packages@125c117...07496eb 2025-04-02 alexander.irion@siemens.com [multicast_dns] MDnsClient::listen supports onError callback (flutter/packages#8888) 2025-04-02 15619084+vashworth@users.noreply.github.com Upgrade tests to use Xcode 16 and iOS 18 (flutter/packages#8968) 2025-04-01 engine-flutter-autoroll@skia.org Manual roll Flutter from 1d954f4 to 05b5e79 (225 revisions) (flutter/packages#8960) 2025-04-01 kevmoo@users.noreply.github.com Drop deprecated HTML head meta tags (flutter/packages#8970) 2025-04-01 stuartmorgan@google.com Adjust PR checklist formatter discussion (flutter/packages#8924) 2025-04-01 stuartmorgan@google.com Update CODEOWNERS username (flutter/packages#8933) 2025-04-01 69054810+M97Chahboun@users.noreply.github.com [flutter_markdown] Added sizedImageBuilder to Markdown widget (flutter/packages#6739) 2025-04-01 stuartmorgan@google.com [google_maps_flutter] Skip impl copy of iOS tests (flutter/packages#8975) 2025-04-01 stuartmorgan@google.com [google_maps_flutter] Skip more hanging iOS tests (flutter/packages#8967) 2025-04-01 stanleycocoa@gmail.com [url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. (flutter/packages#8582) 2025-04-01 robert.odrowaz@leancode.pl [camera_avfoundation] Test utils and mocks swift migration - part 2 (flutter/packages#8892) 2025-03-31 15619084+vashworth@users.noreply.github.com [webview_flutter] Skip flaky legacy tests on iOS (flutter/packages#8911) 2025-03-31 davidmartos96@gmail.com [camera_android] Don't override default fps range when not recording (flutter/packages#8891) 2025-03-31 filiph@users.noreply.github.com [google_maps_flutter] Fix typo and remove duplicitous CHANGELOG entry (flutter/packages#8754) 2025-03-31 kevmoo@users.noreply.github.com [extension_gsi] Support the latest version of googleapis_auth (flutter/packages#8931) 2025-03-31 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.0.0 to 8.9.1 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#8955) 2025-03-31 linxunfeng@yeah.net [webview_flutter_wkwebview] Fixes crash when sending undefined message via javascript channel (flutter/packages#8776) 2025-03-31 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.0.0 to 8.9.1 in /packages/pigeon/platform_tests/alternate_language_test_plugin/android (flutter/packages#8946) 2025-03-31 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Fixes `AdEventType`s not triggering on iOS in release mode (flutter/packages#8918) 2025-03-31 stuartmorgan@google.com [google_maps_flutter] Skip test that hangs iOS CI (flutter/packages#8958) 2025-03-28 engine-flutter-autoroll@skia.org Manual roll Flutter from b16430b to 1d954f4 (114 revisions) (flutter/packages#8922) 2025-03-28 robert.odrowaz@leancode.pl [camera_avfoundation] Test utils and mocks swift migration - part 3 (flutter/packages#8912) 2025-03-27 jacksongardner@google.com Use a more deterministic way of waiting for ad widgets to be ready. (flutter/packages#8920) 2025-03-27 robert.odrowaz@leancode.pl [camera_avfoundation] Test utils and mocks swift migration - part 1 (flutter/packages#8890) 2025-03-27 tarrinneal@gmail.com [pigeon] correct usage of extended generics in generator methods (flutter/packages#8910) 2025-03-27 34892635+fa0311@users.noreply.github.com [video_player] Fix layout issue caused by `Transform.rotate` not affecting space calculation. (flutter/packages#8685) 2025-03-27 43759233+kenzieschmoll@users.noreply.github.com [shared_preferences] Fix a late initialized error with the example app (flutter/packages#8540) 2025-03-26 stuartmorgan@google.com [various] Disable sandbox in Chrome dart tests (flutter/packages#8909) 2025-03-25 stuartmorgan@google.com [tool] Move changed file detection to base command class (flutter/packages#8730) 2025-03-25 34327253+lenzpaul@users.noreply.github.com [Camera] Add lens type information (iOS) (flutter/packages#8723) 2025-03-25 tarrinneal@gmail.com [pigeon] kotlin equality methods (flutter/packages#8887) 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 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 --------- Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>

… the callback to not be triggered correctly. (flutter/packages#8582)

…ack to not be triggered correctly. (flutter#8582) When SFSafariViewController is loading a webpage, if the close button is clicked during the loading process, it can prevent the callback from being triggered correctly. *List which issues are fixed by this PR. You must list at least one issue.* - Fixes flutter/flutter#161790 - Fixes flutter/flutter#47271

…er#166457) flutter/packages@125c117...07496eb 2025-04-02 alexander.irion@siemens.com [multicast_dns] MDnsClient::listen supports onError callback (flutter/packages#8888) 2025-04-02 15619084+vashworth@users.noreply.github.com Upgrade tests to use Xcode 16 and iOS 18 (flutter/packages#8968) 2025-04-01 engine-flutter-autoroll@skia.org Manual roll Flutter from 1d954f4 to 05b5e79 (225 revisions) (flutter/packages#8960) 2025-04-01 kevmoo@users.noreply.github.com Drop deprecated HTML head meta tags (flutter/packages#8970) 2025-04-01 stuartmorgan@google.com Adjust PR checklist formatter discussion (flutter/packages#8924) 2025-04-01 stuartmorgan@google.com Update CODEOWNERS username (flutter/packages#8933) 2025-04-01 69054810+M97Chahboun@users.noreply.github.com [flutter_markdown] Added sizedImageBuilder to Markdown widget (flutter/packages#6739) 2025-04-01 stuartmorgan@google.com [google_maps_flutter] Skip impl copy of iOS tests (flutter/packages#8975) 2025-04-01 stuartmorgan@google.com [google_maps_flutter] Skip more hanging iOS tests (flutter/packages#8967) 2025-04-01 stanleycocoa@gmail.com [url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. (flutter/packages#8582) 2025-04-01 robert.odrowaz@leancode.pl [camera_avfoundation] Test utils and mocks swift migration - part 2 (flutter/packages#8892) 2025-03-31 15619084+vashworth@users.noreply.github.com [webview_flutter] Skip flaky legacy tests on iOS (flutter/packages#8911) 2025-03-31 davidmartos96@gmail.com [camera_android] Don't override default fps range when not recording (flutter/packages#8891) 2025-03-31 filiph@users.noreply.github.com [google_maps_flutter] Fix typo and remove duplicitous CHANGELOG entry (flutter/packages#8754) 2025-03-31 kevmoo@users.noreply.github.com [extension_gsi] Support the latest version of googleapis_auth (flutter/packages#8931) 2025-03-31 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.0.0 to 8.9.1 in /packages/pigeon/platform_tests/test_plugin/android (flutter/packages#8955) 2025-03-31 linxunfeng@yeah.net [webview_flutter_wkwebview] Fixes crash when sending undefined message via javascript channel (flutter/packages#8776) 2025-03-31 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump com.android.tools.build:gradle from 8.0.0 to 8.9.1 in /packages/pigeon/platform_tests/alternate_language_test_plugin/android (flutter/packages#8946) 2025-03-31 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Fixes `AdEventType`s not triggering on iOS in release mode (flutter/packages#8918) 2025-03-31 stuartmorgan@google.com [google_maps_flutter] Skip test that hangs iOS CI (flutter/packages#8958) 2025-03-28 engine-flutter-autoroll@skia.org Manual roll Flutter from b16430b to 1d954f4 (114 revisions) (flutter/packages#8922) 2025-03-28 robert.odrowaz@leancode.pl [camera_avfoundation] Test utils and mocks swift migration - part 3 (flutter/packages#8912) 2025-03-27 jacksongardner@google.com Use a more deterministic way of waiting for ad widgets to be ready. (flutter/packages#8920) 2025-03-27 robert.odrowaz@leancode.pl [camera_avfoundation] Test utils and mocks swift migration - part 1 (flutter/packages#8890) 2025-03-27 tarrinneal@gmail.com [pigeon] correct usage of extended generics in generator methods (flutter/packages#8910) 2025-03-27 34892635+fa0311@users.noreply.github.com [video_player] Fix layout issue caused by `Transform.rotate` not affecting space calculation. (flutter/packages#8685) 2025-03-27 43759233+kenzieschmoll@users.noreply.github.com [shared_preferences] Fix a late initialized error with the example app (flutter/packages#8540) 2025-03-26 stuartmorgan@google.com [various] Disable sandbox in Chrome dart tests (flutter/packages#8909) 2025-03-25 stuartmorgan@google.com [tool] Move changed file detection to base command class (flutter/packages#8730) 2025-03-25 34327253+lenzpaul@users.noreply.github.com [Camera] Add lens type information (iOS) (flutter/packages#8723) 2025-03-25 tarrinneal@gmail.com [pigeon] kotlin equality methods (flutter/packages#8887) 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 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 --------- Co-authored-by: Maurice Parrish <10687576+bparrishMines@users.noreply.github.com>

…ack to not be triggered correctly. (flutter#8582) When SFSafariViewController is loading a webpage, if the close button is clicked during the loading process, it can prevent the callback from being triggered correctly. *List which issues are fixed by this PR. You must list at least one issue.* - Fixes flutter/flutter#161790 - Fixes flutter/flutter#47271

fix(url_launcher): When not fully loaded, clicking close causes the c…

3403990

…allback to not be triggered correctly.

StanleyCocos requested review from jmagman and stuartmorgan-g as code owners February 7, 2025 06:43

github-actions bot added platform-ios p: url_launcher labels Feb 7, 2025

StanleyCocos mentioned this pull request Feb 7, 2025

fix(CupertinoListTile): Click to wait. flutter/flutter#161799

Closed

9 tasks

StanleyCocos added 4 commits February 7, 2025 14:56

fix: change log

6082e8c

fix: format

13e3058

fix: chenge log

87a4a3a

fix: test

940b311

This comment was marked as resolved.

Sign in to view

stuartmorgan-g reviewed Feb 7, 2025

View reviewed changes

fix: type

4f733e5

This comment was marked as resolved.

Sign in to view

StanleyCocos requested a review from stuartmorgan-g February 8, 2025 02:46

stuartmorgan-g changed the title ~~fix(url_launcher): When not fully loaded, clicking close causes the callback to not be triggered correctly.~~ [url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. Feb 8, 2025

stuartmorgan-g marked this pull request as draft February 8, 2025 22:12

StanleyCocos added 2 commits February 10, 2025 11:02

fix: test

2c0d46b

Merge branch 'main' into fix/url_launcher_ios

4c258ab

StanleyCocos marked this pull request as ready for review February 10, 2025 03:17

stuartmorgan-g added the triage-ios Should be looked at in iOS triage label Feb 10, 2025

jmagman requested review from loic-sharma and removed request for jmagman February 12, 2025 22:25

loic-sharma reviewed Feb 12, 2025

View reviewed changes

packages/url_launcher/url_launcher_ios/CHANGELOG.md Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 12, 2025

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 1, 2025

auto-submit bot merged commit 1063222 into flutter:main Apr 1, 2025
82 checks passed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

aeb0d4d

… the callback to not be triggered correctly. (flutter/packages#8582)

This was referenced Apr 1, 2025

Roll Packages from 125c117a432f to 0ef3d446fd44 (23 revisions) flutter/flutter#166341

Closed

Roll Packages from 125c117a432f to 799a5908904c (24 revisions) flutter/flutter#166346

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

bc66c62

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

d90667e

… the callback to not be triggered correctly. (flutter/packages#8582)

This was referenced Apr 1, 2025

Roll Packages from 125c117a432f to bba066f89dd4 (25 revisions) flutter/flutter#166349

Closed

Roll Packages from 125c117a432f to 28676de193f2 (26 revisions) flutter/flutter#166352

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

94fcd4f

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll mentioned this pull request Apr 1, 2025

Roll Packages from 125c117a432f to eb8cb324582e (27 revisions) flutter/flutter#166356

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 1, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

4f3b427

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 2, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

33df594

… the callback to not be triggered correctly. (flutter/packages#8582)

This was referenced Apr 2, 2025

Roll Packages from 125c117a432f to 84ef201205ea (29 revisions) flutter/flutter#166435

Closed

Roll Packages from 125c117a432f to 5cee1525909d (30 revisions) flutter/flutter#166456

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 2, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

8fa3c0e

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll mentioned this pull request Apr 2, 2025

Roll Packages from 125c117a432f to 07496eb03035 (31 revisions) flutter/flutter#166457

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 2, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

0d95fd7

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 2, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

c6b9455

… the callback to not be triggered correctly. (flutter/packages#8582)

This was referenced Apr 2, 2025

Roll Packages from 125c117a432f to 07496eb03035 (31 revisions) flutter/flutter#166469

Closed

Roll Packages from 125c117a432f to 8f5844af8135 (32 revisions) flutter/flutter#166472

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 2, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

797b7a4

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 3, 2025

106322222 [url_launcher] When not fully loaded, clicking close causes…

20fed0d

… the callback to not be triggered correctly. (flutter/packages#8582)

engine-flutter-autoroll mentioned this pull request Apr 3, 2025

Roll Packages from 125c117a432f to 8f5844af8135 (32 revisions) flutter/flutter#166534

Closed

StanleyCocos deleted the fix/url_launcher_ios branch April 7, 2025 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

Uh oh!

StanleyCocos commented Feb 7, 2025 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Feb 7, 2025

Uh oh!

This comment was marked as resolved.

stuartmorgan-g commented Feb 7, 2025

Uh oh!

stuartmorgan-g Feb 7, 2025

Uh oh!

This comment was marked as resolved.

stuartmorgan-g commented Feb 8, 2025

Uh oh!

stuartmorgan-g commented Feb 8, 2025

Uh oh!

StanleyCocos commented Feb 10, 2025

Uh oh!

Uh oh!

loic-sharma Feb 12, 2025

Uh oh!

StanleyCocos Feb 13, 2025

Uh oh!

StanleyCocos Feb 13, 2025

Uh oh!

stuartmorgan-g Mar 7, 2025

Uh oh!

StanleyCocos Mar 17, 2025

Uh oh!

StanleyCocos Mar 25, 2025

Uh oh!

Uh oh!

Uh oh!

[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582

Uh oh!

Conversation

StanleyCocos commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 7, 2025

Uh oh!

This comment was marked as resolved.

stuartmorgan-g commented Feb 7, 2025

Uh oh!

stuartmorgan-g Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

stuartmorgan-g commented Feb 8, 2025

Uh oh!

stuartmorgan-g commented Feb 8, 2025

Uh oh!

StanleyCocos commented Feb 10, 2025

Uh oh!

Uh oh!

loic-sharma Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

StanleyCocos Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

StanleyCocos Feb 13, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

StanleyCocos Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

StanleyCocos Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StanleyCocos commented Feb 7, 2025 •

edited

Loading