[url_launcher] When not fully loaded, clicking close causes the callback to not be triggered correctly. #8582
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,24 @@ final class URLLauncherTests: XCTestCase { | |
| XCTAssertEqual(launcher.passedOptions?[.universalLinksOnly] as? Bool, true) | ||
| } | ||
|
|
||
| func testLaunchSafariViewControllerWithClose() { | ||
| let launcher = FakeLauncher() | ||
| let plugin = createPlugin(launcher: launcher) | ||
|
|
||
| let expectation = XCTestExpectation(description: "completion called") | ||
| plugin.openUrlInSafariViewController(url: "https://flutter.dev") { result in | ||
| switch result { | ||
| case .success(let details): | ||
| XCTAssertEqual(details, .failedToLoad) | ||
| case .failure(let error): | ||
| XCTFail("Unexpected error: \(error)") | ||
| } | ||
| expectation.fulfill() | ||
| } | ||
| plugin.closeSafariViewController() | ||
| wait(for: [expectation], timeout: 30) | ||
|
There was a problem hiding this comment. Is this From my understanding I apologize if this is a silly question, I'm not very familiar with XCTest. There was a problem hiding this comment. Our standard for expectation timeouts in the repo is 30 (older code still uses lower timeouts). Flutter style is not to have timeouts at all but that's awkward with XCTest APIs, so we just use a value that is absurdly long for unit tests. There was a problem hiding this comment. Under normal circumstances, wait(for: [expectation], timeout: 30) won’t actually wait, because the closure has already been triggered before it. The 30-second timeout is set to prevent accidentally removing this PR functionality. If this PR functionality is deleted, setting the wait time to 1 might not make sense, as the callback may never be reached for verification. |
||
| } | ||
|
|
||
| } | ||
|
|
||
| final private class FakeLauncher: NSObject, Launcher { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ final class URLLaunchSession: NSObject, SFSafariViewControllerDelegate { | |
|
|
||
| private let completion: OpenInSafariCompletionHandler | ||
| private let url: URL | ||
| private var isLoadCompleted: Bool = false | ||
|
|
||
| /// The Safari view controller used for displaying the URL. | ||
| let safariViewController: SFSafariViewController | ||
|
|
@@ -46,12 +47,16 @@ final class URLLaunchSession: NSObject, SFSafariViewControllerDelegate { | |
| } else { | ||
| completion(.success(.failedToLoad)) | ||
| } | ||
| isLoadCompleted = true | ||
| } | ||
|
|
||
| /// 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)) | ||
|
There was a problem hiding this comment. Is 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 There was a problem hiding this comment. 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. There was a problem hiding this comment.
Do you think it’s possible to modify the comment like this: // The URL did not load successfully (or close the SFSafariViewController earlier) |
||
| } | ||
| controller.dismiss(animated: true, completion: nil) | ||
| didFinish?() | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that this test would flake if
https://flutter.devloads before the test callscloseSafariViewController. Is there some URL we could test that is known to hang forever? Or am I overthinking this? cc @stuartmorganThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not expect it to be possible for that to happen. Unless
SFSafariViewController'sdidCompleteInitialLoadcallback happens on a non-main thread (which I wouldn't expect since it passes the view controller as a parameter), it shouldn't be possible for the callback to trigger until the runloop runs again, which I would think could not happen before thewaitcall.There are some assumptions in there, so I wouldn't be confident in saying that it's impossible, but I'd be fine with waiting to see if it does actually happen before we add a bunch of extra logic (since the reliable way to avoid that would be dependency injection of a fake
SFSafariViewControllerthat we know won't load anything) to avoid I case that I strongly suspect is in fact impossible.