Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[web-browser] Fix issue when the browser is dismissed quickly #28452

Merged
merged 3 commits into from Apr 29, 2024
Merged

[web-browser] Fix issue when the browser is dismissed quickly #28452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa65cd3
[web-browser] Fix issue when the browser is dismissed quickly
alanjhughes Apr 25, 2024
b39a049
[web-browser] changelog
alanjhughes Apr 25, 2024
eef90b1
[web-browser] remove check in dismiss browser
alanjhughes Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/expo-web-browser/CHANGELOG.md
View file
Open in desktop
Expand Up @@ -8,6 +8,8 @@

### 🐛 Bug fixes

- On `iOS`, fix an issue where rapidly opening and closing the browser would leave the module in a bad state, preventing opening the browser again. ([#28452](https://github.com/expo/expo/pull/28452) by [@alanjhughes](https://github.com/alanjhughes))

### 💡 Others

## 13.0.2 — 2024-04-24
Expand Down
2 changes: 1 addition & 1 deletion packages/expo-web-browser/build/WebBrowser.d.ts.map
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 3 additions & 17 deletions packages/expo-web-browser/build/WebBrowser.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/expo-web-browser/build/WebBrowser.js.map
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions packages/expo-web-browser/ios/WebBrowserModule.swift
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ import AuthenticationServices
final public class WebBrowserModule: Module {
private var currentWebBrowserSession: WebBrowserSession?
private var currentAuthSession: WebAuthSession?
private var vcDidPresent = false

private func isValid(url: URL) -> Bool {
return url.scheme == "http" || url.scheme == "https"
Expand All @@ -16,6 +17,11 @@ final public class WebBrowserModule: Module {
Name("ExpoWebBrowser")

AsyncFunction("openBrowserAsync") { (url: URL, options: WebBrowserOptions, promise: Promise) in
if vcDidPresent {
self.currentWebBrowserSession = nil
vcDidPresent = false
}

guard self.currentWebBrowserSession == nil else {
throw WebBrowserAlreadyOpenException()
}
Expand All @@ -27,6 +33,8 @@ final public class WebBrowserModule: Module {
self.currentWebBrowserSession = WebBrowserSession(url: url, options: options) { [promise] type in
promise.resolve(["type": type])
self.currentWebBrowserSession = nil
} didPresent: {
self.vcDidPresent = true
}

self.currentWebBrowserSession?.open()
Expand Down
2 changes: 1 addition & 1 deletion packages/expo-web-browser/ios/WebBrowserOptions.swift
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ struct WebBrowserOptions: Record {

@Field
var controlsColor: UIColor?

// Defaults to .overFullScreen to keep backwards compatibility
@Field
var presentationStyle: PresentationStyle = .overFullScreen
Expand Down
10 changes: 7 additions & 3 deletions packages/expo-web-browser/ios/WebBrowserSession.swift
View file
Open in desktop
Expand Up @@ -6,9 +6,11 @@ import ExpoModulesCore
internal class WebBrowserSession: NSObject, SFSafariViewControllerDelegate, UIAdaptivePresentationControllerDelegate {
let viewController: SFSafariViewController
let onDismiss: (String) -> Void
let didPresent: () -> Void

init(url: URL, options: WebBrowserOptions, onDismiss: @escaping (String) -> Void) {
init(url: URL, options: WebBrowserOptions, onDismiss: @escaping (String) -> Void, didPresent: @escaping () -> Void) {
self.onDismiss = onDismiss
self.didPresent = didPresent

let configuration = SFSafariViewController.Configuration()
configuration.barCollapsingEnabled = options.enableBarCollapsing
Expand All @@ -30,7 +32,9 @@ internal class WebBrowserSession: NSObject, SFSafariViewControllerDelegate, UIAd
while currentViewController?.presentedViewController != nil {
currentViewController = currentViewController?.presentedViewController
}
currentViewController?.present(viewController, animated: true, completion: nil)
currentViewController?.present(viewController, animated: true) {
self.didPresent()
}
}

func dismiss() {
Expand All @@ -44,7 +48,7 @@ internal class WebBrowserSession: NSObject, SFSafariViewControllerDelegate, UIAd
func safariViewControllerDidFinish(_ controller: SFSafariViewController) {
finish(type: "cancel")
}

// MARK: - UIAdaptivePresentationControllerDelegate

func presentationControllerDidDismiss(_ presentationController: UIPresentationController) {
Expand Down
25 changes: 3 additions & 22 deletions packages/expo-web-browser/src/WebBrowser.ts
View file
Open in desktop
Expand Up @@ -143,8 +143,6 @@ export async function coolDownAsync(browserPackage?: string): Promise<WebBrowser
}
}

let browserLocked = false;

// @needsAudit
/**
* Opens the url with Safari in a modal on iOS using [`SFSafariViewController`](https://developer.apple.com/documentation/safariservices/sfsafariviewcontroller),
Expand All @@ -169,25 +167,11 @@ export async function openBrowserAsync(
throw new UnavailabilityError('WebBrowser', 'openBrowserAsync');
}

if (browserLocked) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to remove on Android?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android uses intents, it always navigates away from the app so it had no guards anyway. It's not possible to call it twice. I think this was aimed at iOS specifically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did just notice one issue, dismissBrowser shouldn't be called on android.

// Prevent multiple sessions from running at the same time, WebBrowser doesn't
// support it this makes the behavior predictable.
if (__DEV__) {
console.warn(
'Attempted to call WebBrowser.openBrowserAsync multiple times while already active. Only one WebBrowser controller can be active at any given time.'
);
}

return { type: WebBrowserResultType.LOCKED };
}
browserLocked = true;

let result: WebBrowserResult;
try {
result = await ExponentWebBrowser.openBrowserAsync(url, _processOptions(browserParams));
} finally {
// WebBrowser session complete, unset lock
browserLocked = false;
} catch {
return { type: WebBrowserResultType.LOCKED };
}

return result;
Expand All @@ -201,10 +185,7 @@ export async function openBrowserAsync(
* @platform ios
*/
export function dismissBrowser(): void {
if (!ExponentWebBrowser.dismissBrowser) {
throw new UnavailabilityError('WebBrowser', 'dismissBrowser');
}
ExponentWebBrowser.dismissBrowser();
ExponentWebBrowser.dismissBrowser?.();
}

// @needsAudit
Expand Down