redo the New Window Policy decision making for macOS 14.1 (#1776)

Task/Issue URL: https://app.asana.com/0/1177771139624306/1205690527704551/f Original PR: #1767
duckduckgo · Oct 20, 2023 · 2454ca2 · 2454ca2
1 parent 285aeb8
commit 2454ca2
Show file tree

Hide file tree

Showing 10 changed files with 55 additions and 69 deletions.
diff --git a/DuckDuckGo/Tab/Model/NewWindowPolicy.swift b/DuckDuckGo/Tab/Model/NewWindowPolicy.swift
@@ -30,6 +30,15 @@ enum NewWindowPolicy {
                         burner: isBurner)
         } else if windowFeatures.width != nil {
             self = .popup(origin: windowFeatures.origin, size: windowFeatures.size)
+
+        } else
+        // This is a temporary fix for macOS 14.1 WKWindowFeatures being empty when opening a new regular tab
+        // Instead of defaulting to window policy, we default to tab policy, and allow popups in some limited scenarios.
+        // See https://app.asana.com/0/1177771139624306/1205690527704551/f.
+        if #available(macOS 14.1, *),
+           windowFeatures.statusBarVisibility == nil && windowFeatures.menuBarVisibility == nil {
+            self = .tab(selected: shouldSelectNewTab, burner: isBurner)
+
         } else {
             self = .window(active: true, burner: isBurner)
         }

diff --git a/DuckDuckGo/Tab/Model/Tab+UIDelegate.swift b/DuckDuckGo/Tab/Model/Tab+UIDelegate.swift
@@ -78,7 +78,7 @@ extension Tab: WKUIDelegate, PrintingUserScriptDelegate {
                                             windowFeatures: WKWindowFeatures,
                                             completionHandler: @escaping (WKWebView?) -> Void) {
 
-        switch newWindowPolicy(for: navigationAction, windowFeatures: windowFeatures) {
+        switch newWindowPolicy(for: navigationAction) {
         // popup kind is known, action doesn‘t require Popup Permission
         case .allow(let targetKind):
             // proceed to web view creation
@@ -123,7 +123,7 @@ extension Tab: WKUIDelegate, PrintingUserScriptDelegate {
         }
     }
 
-    private func newWindowPolicy(for navigationAction: WKNavigationAction, windowFeatures: WKWindowFeatures) -> NavigationDecision? {
+    private func newWindowPolicy(for navigationAction: WKNavigationAction) -> NavigationDecision? {
         if let newWindowPolicy = self.decideNewWindowPolicy(for: navigationAction) {
             return newWindowPolicy
         }
@@ -134,16 +134,6 @@ extension Tab: WKUIDelegate, PrintingUserScriptDelegate {
             return newWindowPolicy
         }
 
-        // This is a temporary fix for macOS 14.1 WKWindowFeatures being empty when opening a new regular tab
-        // Instead of defaulting to no policy, we default to tab policy, and allow popups in some limited scenarios.
-        // See https://app.asana.com/0/1177771139624306/1205690527704551/f.
-        if #available(macOS 14.1, *) {
-            if windowFeatures.statusBarVisibility != nil || windowFeatures.menuBarVisibility != nil {
-                return nil
-            }
-            return .allow(.tab(selected: true, burner: burnerMode.isBurner))
-        }
-
         // allow popups opened from an empty window console
         let sourceUrl = navigationAction.safeSourceFrame?.safeRequest?.url ?? self.url ?? .empty
         if sourceUrl.isEmpty || sourceUrl.scheme == URL.NavigationalScheme.about.rawValue {

diff --git a/DuckDuckGo/Tab/Model/Tab.swift b/DuckDuckGo/Tab/Model/Tab.swift
@@ -529,7 +529,7 @@ protocol NewWindowPolicyDecisionMaker {
     }
 
     @discardableResult
-    func setContent(_ newContent: TabContent) -> Task<ExpectedNavigation?, Never>? {
+    func setContent(_ newContent: TabContent) -> ExpectedNavigation? {
         guard contentChangeEnabled else { return nil }
 
         let oldContent = self.content
@@ -554,13 +554,11 @@ protocol NewWindowPolicyDecisionMaker {
             self.title = title
         }
 
-        return Task {
-            await reloadIfNeeded(shouldLoadInBackground: true)
-        }
+        return reloadIfNeeded(shouldLoadInBackground: true)
     }
 
     @discardableResult
-    func setUrl(_ url: URL?, userEntered: String?) -> Task<ExpectedNavigation?, Never>? {
+    func setUrl(_ url: URL?, userEntered: String?) -> ExpectedNavigation? {
         if url == .welcome {
             OnboardingViewModel().restart()
         }
@@ -697,7 +695,7 @@ protocol NewWindowPolicyDecisionMaker {
 
         let canGoBack = webView.canGoBack || self.error != nil
         let canGoForward = webView.canGoForward && self.error == nil
-        let canReload = (self.content.urlForWebView?.scheme ?? URL.NavigationalScheme.about.rawValue) != URL.NavigationalScheme.about.rawValue
+        let canReload = self.content.userEditableUrl != nil
 
         if canGoBack != self.canGoBack {
             self.canGoBack = canGoBack
@@ -763,26 +761,18 @@ protocol NewWindowPolicyDecisionMaker {
 
         if webView.url == nil, content.isUrl {
             // load from cache or interactionStateData when called by lazy loader
-            Task { @MainActor [weak self] in
-                await self?.reloadIfNeeded(shouldLoadInBackground: true)
-            }
+            reloadIfNeeded(shouldLoadInBackground: true)
         } else {
             webView.reload()
         }
     }
 
-    @MainActor
+    @MainActor(unsafe)
     @discardableResult
-    private func reloadIfNeeded(shouldLoadInBackground: Bool = false) async -> ExpectedNavigation? {
+    private func reloadIfNeeded(shouldLoadInBackground: Bool = false) -> ExpectedNavigation? {
+        guard case .url(let url, credential: _, userEntered: let userEntered) = content, url.scheme != "about" else { return nil }
 
-        let content = self.content
-        guard let url = content.urlForWebView,
-              url.scheme.map(URL.NavigationalScheme.init) != .about else { return nil }
-
-        var userForcedReload = false
-        if case .url(let url, _, let userEntered) = content, url.absoluteString == userEntered {
-            userForcedReload = shouldLoadInBackground
-        }
+        let userForcedReload = (url.absoluteString == userEntered) ? shouldLoadInBackground : false
 
         if userForcedReload || shouldReload(url, shouldLoadInBackground: shouldLoadInBackground) {
             let didRestore = restoreInteractionStateDataIfNeeded()
@@ -902,9 +892,7 @@ protocol NewWindowPolicyDecisionMaker {
         webView.observe(\.superview, options: .old) { [weak self] _, change in
             // if the webView is being added to superview - reload if needed
             if case .some(.none) = change.oldValue {
-                Task { @MainActor [weak self] in
-                    await self?.reloadIfNeeded()
-                }
+                self?.reloadIfNeeded()
             }
         }.store(in: &webViewCancellables)
 
@@ -942,10 +930,10 @@ protocol NewWindowPolicyDecisionMaker {
         }.store(in: &webViewCancellables)
 
         // background tab loading should start immediately
-        Task { @MainActor in
-            await reloadIfNeeded(shouldLoadInBackground: shouldLoadInBackground)
+        DispatchQueue.main.async {
+            self.reloadIfNeeded(shouldLoadInBackground: shouldLoadInBackground)
             if !shouldLoadInBackground {
-                addHomePageToWebViewIfNeeded()
+                self.addHomePageToWebViewIfNeeded()
             }
         }
     }

diff --git a/IntegrationTests/AutoconsentIntegrationTests.swift b/IntegrationTests/AutoconsentIntegrationTests.swift
@@ -73,7 +73,7 @@ class AutoconsentIntegrationTests: XCTestCase {
             .first()
             .promise()
 
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         let cookieConsentManaged = try await cookieConsentManagedPromise.value
         XCTAssertTrue(cookieConsentManaged)
@@ -87,7 +87,7 @@ class AutoconsentIntegrationTests: XCTestCase {
 
         let tab = self.tabViewModel.tab
 
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         // expect cookieConsent request to be published
         let cookieConsentPromptRequestPromise = tab.cookieConsentPromptRequestPublisher
@@ -130,7 +130,7 @@ class AutoconsentIntegrationTests: XCTestCase {
             .first()
             .promise()
 
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         do {
             let cookieConsentManaged = try await cookieConsentManagedPromise.value
@@ -182,7 +182,7 @@ class AutoconsentIntegrationTests: XCTestCase {
             .promise()
 
         os_log("starting navigation to http://privacy-test-pages.site/features/autoconsent/banner.html")
-        let navigation = await tab.setUrl(url, userEntered: nil)?.value
+        let navigation = tab.setUrl(url, userEntered: nil)
 
         navigation?.appendResponder(navigationResponse: { response in
             os_log("navigationResponse: %s", "\(String(describing: response))")

diff --git a/IntegrationTests/Downloads/DownloadsIntegrationTests.swift b/IntegrationTests/Downloads/DownloadsIntegrationTests.swift
@@ -61,7 +61,7 @@ class DownloadsIntegrationTests: XCTestCase {
                                      headers: ["Content-Disposition": "attachment; filename=\"fname_\(suffix).dat\"",
                                                "Content-Type": "text/html"])
         let tab = tabViewModel.tab
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         let fileUrl = try await downloadTaskFuture.get().output
             .timeout(1, scheduler: DispatchQueue.main) { .init(TimeoutError() as NSError, isRetryable: false) }.first().promise().get()
@@ -78,7 +78,7 @@ class DownloadsIntegrationTests: XCTestCase {
         let tab = tabViewModel.tab
         // load empty page
         let pageUrl = URL.testsServer.appendingTestParameters(data: data.html)
-        _=await tab.setUrl(pageUrl, userEntered: nil)?.value?.result
+        _=await tab.setUrl(pageUrl, userEntered: nil)?.result
 
         let downloadTaskFuture = FileDownloadManager.shared.downloadsPublisher.timeout(5).first().promise()
         let suffix = Int.random(in: 0..<Int.max)
@@ -110,7 +110,7 @@ class DownloadsIntegrationTests: XCTestCase {
         let tab = tabViewModel.tab
         // load empty page
         let pageUrl = URL.testsServer.appendingTestParameters(data: data.html)
-        _=await tab.setUrl(pageUrl, userEntered: nil)?.value?.result
+        _=await tab.setUrl(pageUrl, userEntered: nil)?.result
 
         let downloadTaskFuture = FileDownloadManager.shared.downloadsPublisher.timeout(5).first().promise()
         let suffix = Int.random(in: 0..<Int.max)

diff --git a/IntegrationTests/HTTPSUpgrade/HTTPSUpgradeIntegrationTests.swift b/IntegrationTests/HTTPSUpgrade/HTTPSUpgradeIntegrationTests.swift
@@ -67,7 +67,7 @@ class HTTPSUpgradeIntegrationTests: XCTestCase {
         let tabViewModel = self.tabViewModel
         let tab = tabViewModel.tab
 
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         // expect popup to open and then close
         var oldValue: TabViewModel! = self.tabViewModel
@@ -124,7 +124,7 @@ class HTTPSUpgradeIntegrationTests: XCTestCase {
         let tabViewModel = self.tabViewModel
         let tab = tabViewModel.tab
 
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         // expect popup to open and then close
         var oldValue: TabViewModel! = self.tabViewModel

diff --git a/IntegrationTests/History/HistoryIntegrationTests.swift b/IntegrationTests/History/HistoryIntegrationTests.swift
@@ -65,15 +65,14 @@ class HistoryIntegrationTests: XCTestCase {
         """
 
         let url = URL.testsServer.appendingTestParameters(data: html.utf8data)
-
         let titleChangedPromise1 = tab.$title
             .filter { $0 == "Title 1" }
             .receive(on: DispatchQueue.main)
             .timeout(5, "Title 1")
             .first()
             .promise()
 
-        _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(url, userEntered: nil)?.result.get()
         _=try await titleChangedPromise1.value
 
         XCTAssertEqual(HistoryCoordinator.shared.history?.count, 1)
@@ -119,7 +118,7 @@ class HistoryIntegrationTests: XCTestCase {
             URL(string: URL.testsServer.appendingTestParameters(data: html.utf8data).absoluteString + "#1")!,
         ]
 
-        _=try await tab.setUrl(urls[0], userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(urls[0], userEntered: nil)?.result.get()
 
         let titleChangedPromise = tab.$title
             .filter { $0 == "Title 2" }
@@ -150,9 +149,9 @@ class HistoryIntegrationTests: XCTestCase {
             URL.testsServer,
             URL.testsServer.appendingPathComponent("page1").appendingTestParameters(data: "".utf8data),
         ]
-        _=try await tab.setUrl(urls[0], userEntered: nil)?.value?.result.get()
-        _=try await tab.setUrl(urls[1], userEntered: nil)?.value?.result.get()
-        _=try await tab.setUrl(urls[0], userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(urls[0], userEntered: nil)?.result.get()
+        _=try await tab.setUrl(urls[1], userEntered: nil)?.result.get()
+        _=try await tab.setUrl(urls[0], userEntered: nil)?.result.get()
 
         let first = HistoryCoordinator.shared.history?.first(where: { $0.url == urls[0] })
         XCTAssertEqual(first?.numberOfVisits, 2)
@@ -170,8 +169,8 @@ class HistoryIntegrationTests: XCTestCase {
             URL.testsServer,
             URL.testsServer.appendingPathComponent("page1").appendingTestParameters(data: "".utf8data),
         ]
-        _=try await tab.setUrl(urls[0], userEntered: nil)?.value?.result.get()
-        _=try await tab.setUrl(urls[1], userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(urls[0], userEntered: nil)?.result.get()
+        _=try await tab.setUrl(urls[1], userEntered: nil)?.result.get()
         _=try await tab.goBack()?.result.get()
         _=try await tab.goForward()?.result.get()
 
@@ -200,7 +199,7 @@ class HistoryIntegrationTests: XCTestCase {
             .first()
             .promise()
 
-        _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(url, userEntered: nil)?.result.get()
         _=try await trackerPromise.value
 
         let first = HistoryCoordinator.shared.history?.first
@@ -228,7 +227,7 @@ class HistoryIntegrationTests: XCTestCase {
             .first()
             .promise()
 
-        _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(url, userEntered: nil)?.result.get()
         _=try await trackerPromise.value
 
         let first = HistoryCoordinator.shared.history?.first

diff --git a/IntegrationTests/NavigationProtection/NavigationProtectionIntegrationTests.swift b/IntegrationTests/NavigationProtection/NavigationProtectionIntegrationTests.swift
@@ -66,7 +66,7 @@ class NavigationProtectionIntegrationTests: XCTestCase {
         window = WindowsManager.openNewWindow(with: tab)!
 
         let url = URL(string: "https://privacy-test-pages.site/privacy-protections/amp/")!
-        _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(url, userEntered: nil)?.result.get()
 
         let itemsCount = try await tab.webView.evaluateJavaScript("document.getElementsByTagName('li').length") as? Int ?? 0
         XCTAssertTrue(itemsCount > 0, "no items")
@@ -76,7 +76,7 @@ class NavigationProtectionIntegrationTests: XCTestCase {
             print("processing", i)
             // open test page if needed
             if tab.content.url != url {
-                _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+                _=try await tab.setUrl(url, userEntered: nil)?.result.get()
             }
 
             // extract "Expected" URL
@@ -149,7 +149,7 @@ class NavigationProtectionIntegrationTests: XCTestCase {
         window = WindowsManager.openNewWindow(with: tab)!
 
         let url = URL(string: "https://privacy-test-pages.site/privacy-protections/referrer-trimming/")!
-        _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(url, userEntered: nil)?.result.get()
 
         // run test
         _=try await tab.webView.evaluateJavaScript("(function() { document.getElementById('start').click(); return true })()")
@@ -204,7 +204,7 @@ class NavigationProtectionIntegrationTests: XCTestCase {
         let url = URL(string: "https://privacy-test-pages.site/privacy-protections/gpc/")!
         // disable GPC redirects
         PrivacySecurityPreferences.shared.gpcEnabled = false
-        _=try await tab.setUrl(url, userEntered: nil)?.value?.result.get()
+        _=try await tab.setUrl(url, userEntered: nil)?.result.get()
 
         // enable GPC redirects
         PrivacySecurityPreferences.shared.gpcEnabled = true

diff --git a/IntegrationTests/PrivacyDashboard/PrivacyDashboardIntegrationTests.swift b/IntegrationTests/PrivacyDashboard/PrivacyDashboardIntegrationTests.swift
@@ -65,7 +65,7 @@ class PrivacyDashboardIntegrationTests: XCTestCase {
 
         // load the test page
         let url = URL(string: "http://privacy-test-pages.site/tracker-reporting/1major-via-script.html")!
-        _=await tab.setUrl(url, userEntered: nil)?.value?.result
+        _=await tab.setUrl(url, userEntered: nil)?.result
 
         let trackersCount = try await trackersCountPromise.value
         XCTAssertEqual(trackersCount, 1)
@@ -78,7 +78,7 @@ class PrivacyDashboardIntegrationTests: XCTestCase {
             .timeout(10)
             .first()
             .promise()
-        _=await tab.setUrl(URL.testsServer, userEntered: nil)?.value?.result
+        _=await tab.setUrl(URL.testsServer, userEntered: nil)?.result
 
         let trackersCount2 = try await trackersCountPromise2.value
         XCTAssertEqual(trackersCount2, 0)