[MOB-4042] Handle links meant to be opened in a new tab in EcosiaWebViewModal

[d4r1091]

MOB-4042

Context

We noticed that some links meant to be opened in a new tab within our EcosiaWebViewModal weren't working.
We don't, in fact, handle tabs or special navigation in this modal view.

Approach

We wanted to implement the simplest approach possible that wouldn't interfere much with a convoluted UX or the Firefox code.

Problem: Handle target="_blank" links in profile modal.

Attempts:

1. Callback-based → Failed: Closure retention issues through SwiftUI struct hierarchy
2. Load directly → Partial: Caused infinite back-navigation loops
3. Track navigation history ✅ Success: Record origin pages, skip them on back navigation

Other

We also removed some callbacks we identified not working.

Before merging

Checklist

• I performed some relevant testing on a real device and/or simulator for both iPhone and iPad

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Navigation Loop Risk The back-navigation skip relies on matching exact URL strings in blankTargetURLs. Query params, hashes, or redirects may cause mismatches and reintroduce loops or prevent intended back behavior. Consider normalizing URLs or tracking via WKBackForwardListItem/host-only comparison. func webView(_ webView: WKWebView, decidePolicyFor navigationAction: WKNavigationAction, decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) { guard let url = navigationAction.request.url else { decisionHandler(.allow) return } // If this is a back navigation to a page we loaded from a target="_blank" link, // prevent it and go back further (to the page before the blank link was clicked) if navigationAction.navigationType == .backForward, blankTargetURLs.contains(url.absoluteString) { EcosiaLogger.auth.debug("🔐 [WEBVIEW] Preventing navigation back to target='_blank' origin: \(url)") decisionHandler(.cancel) // Keep going back to escape the blank-target page if webView.canGoBack { webView.goBack() } else { // If we can't go back, reload the initial URL (root) webView.load(URLRequest(url: parent.url)) blankTargetURLs.removeAll() } return } State Reset After handling a target="_blank" flow, blankTargetURLs persists across future navigations. Lack of cleanup on successful forward navigation may affect later back actions. Consider clearing entries when navigating away from origins or after non-blank navigations complete. private var blankTargetURLs: Set<String> = [] init(parent: WebViewRepresentable) { self.parent = parent self.retryCount = parent.retryCount self.remainingRetries = parent.retryCount } func webView(_ webView: WKWebView, didStartProvisionalNavigation navigation: WKNavigation!) { parent.isLoading = true parent.hasError = false } func webView(_ webView: WKWebView, didFinish navigation: WKNavigation!) { parent.isLoading = false parent.pageTitle = webView.title ?? "" parent.onLoadComplete?() } func webView(_ webView: WKWebView, decidePolicyFor navigationAction: WKNavigationAction, decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) { guard let url = navigationAction.request.url else { decisionHandler(.allow) return } // If this is a back navigation to a page we loaded from a target="_blank" link, // prevent it and go back further (to the page before the blank link was clicked) if navigationAction.navigationType == .backForward, blankTargetURLs.contains(url.absoluteString) { EcosiaLogger.auth.debug("🔐 [WEBVIEW] Preventing navigation back to target='_blank' origin: \(url)") decisionHandler(.cancel) // Keep going back to escape the blank-target page if webView.canGoBack { webView.goBack() } else { // If we can't go back, reload the initial URL (root) webView.load(URLRequest(url: parent.url)) blankTargetURLs.removeAll() } return } Retry Logic Semantics remainingRetries is updated on init and decremented in didFailProvisionalNavigation, but it's not reset between loads. Confirm intended behavior; otherwise consider resetting on successful load start/finish to avoid suppressing legitimate error handling on subsequent navigations. guard remainingRetries == 0 else { remainingRetries -= 1 return } parent.isLoading = false parent.hasError = true parent.errorMessage = error.localizedDescription }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid re-entrant back navigation Remove the automatic goBack() cascade after cancelling, and just cancel the back-forward navigation. Forcing goBack() inside decidePolicyFor can cause re-entrant navigation and unpredictable history jumps. Also clear only the matched URL from blankTargetURLs to avoid wiping unrelated entries. firefox-ios/Ecosia/UI/Account/EcosiaWebViewModal.swift [197-211] if navigationAction.navigationType == .backForward, blankTargetURLs.contains(url.absoluteString) { EcosiaLogger.auth.debug("🔐 [WEBVIEW] Preventing navigation back to target='_blank' origin: \(url)") decisionHandler(.cancel) - - // Keep going back to escape the blank-target page - if webView.canGoBack { - webView.goBack() - } else { - // If we can't go back, reload the initial URL (root) - webView.load(URLRequest(url: parent.url)) - blankTargetURLs.removeAll() - } + blankTargetURLs.remove(url.absoluteString) return } Suggestion importance[1-10]: 6 __ Why: The concern about triggering re-entrant navigation from within decidePolicyFor is reasonable and removing the forced goBack() can reduce unexpected history behavior; selectively removing only the matched URL is also sensible. However, the current approach may be intentional to escape blank-target loops, so impact is moderate rather than critical.	Low
General	Use URL-based set membership Track origins using URL rather than raw strings to avoid subtle mismatches due to normalization, fragments, or trailing slashes. Store URL and compare via absoluteURL to ensure reliable detection in back-forward decisions. firefox-ios/Ecosia/UI/Account/EcosiaWebViewModal.swift [168-169] -private var blankTargetURLs: Set<String> = [] +private var blankTargetURLs: Set<URL> = [] +// When recording: +if let currentURL = webView.url?.absoluteURL { + blankTargetURLs.insert(currentURL) + EcosiaLogger.auth.info("🔐 [WEBVIEW] Recorded blank-target origin: \(currentURL)") +} + +// When checking: +if navigationAction.navigationType == .backForward, + let target = url.absoluteURL as URL?, + blankTargetURLs.contains(target) { + ... +} + Suggestion importance[1-10]: 5 __ Why: Switching from String to URL for tracking improves robustness against normalization issues and is a reasonable enhancement, but it’s a maintainability improvement rather than fixing a clear bug.	Low
	Avoid race in new-window handling Use the provided configuration to instantiate and return a transient web view only when you need to intercept, but prefer loading via the existing webView without side effects inside this delegate. At minimum, call the decisionHandler path by canceling the navigation in decidePolicyFor and only trigger load after that path to avoid double loads/races. firefox-ios/Ecosia/UI/Account/EcosiaWebViewModal.swift [244-268] func webView( _ webView: WKWebView, createWebViewWith configuration: WKWebViewConfiguration, for navigationAction: WKNavigationAction, windowFeatures: WKWindowFeatures ) -> WKWebView? { - // When targetFrame is nil, it means the link should open in a new tab/window - // (e.g., target="_blank" or window.open()) guard navigationAction.targetFrame == nil, let url = navigationAction.request.url else { return nil } - // Record the current page before loading the blank target if let currentURL = webView.url { blankTargetURLs.insert(currentURL.absoluteString) EcosiaLogger.auth.info("🔐 [WEBVIEW] Recorded blank-target origin: \(currentURL)") } - EcosiaLogger.auth.info("🔐 [WEBVIEW] Loading target='_blank' URL in modal: \(url)") + // Load the URL in the same webView and prevent spawning a new window webView.load(URLRequest(url: url)) - - // Return nil to prevent creating a new window return nil } Suggestion importance[1-10]: 4 __ Why: The suggestion reiterates loading the URL in the same webView and returning nil, which matches the existing implementation and adds no concrete change beyond general advice about decision handling; impact is minor.	Low

[ecotopian]

The WebView delegates carry quite some logic. Can you please add unit test coverage making sure the logic works? As well to the addition below? Thank you

[ecotopian]

Looks good to me. Would be neat to have some unit tests covering the new logic. No blocker, but nice to have.