From 2454ca2d9579ed7bf6cc4e205644265a94c79d4d Mon Sep 17 00:00:00 2001 From: Alexey Martemyanov Date: Fri, 20 Oct 2023 19:26:02 +0600 Subject: [PATCH] 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: https://github.com/duckduckgo/macos-browser/pull/1767 --- DuckDuckGo/Tab/Model/NewWindowPolicy.swift | 9 +++++ DuckDuckGo/Tab/Model/Tab+UIDelegate.swift | 14 +------ DuckDuckGo/Tab/Model/Tab.swift | 38 +++++++------------ .../AutoconsentIntegrationTests.swift | 8 ++-- .../Downloads/DownloadsIntegrationTests.swift | 6 +-- .../HTTPSUpgradeIntegrationTests.swift | 4 +- .../History/HistoryIntegrationTests.swift | 19 +++++----- ...NavigationProtectionIntegrationTests.swift | 8 ++-- .../PrivacyDashboardIntegrationTests.swift | 4 +- .../Permissions/TabPermissionsTests.swift | 14 +++---- 10 files changed, 55 insertions(+), 69 deletions(-) diff --git a/DuckDuckGo/Tab/Model/NewWindowPolicy.swift b/DuckDuckGo/Tab/Model/NewWindowPolicy.swift index 8db6c89b1a..ca9e575d29 100644 --- 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 index c6bd9c42a5..c87cd89b93 100644 --- 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 index 503b6acc02..f5dd8871d7 100644 --- a/DuckDuckGo/Tab/Model/Tab.swift +++ b/DuckDuckGo/Tab/Model/Tab.swift @@ -529,7 +529,7 @@ protocol NewWindowPolicyDecisionMaker { } @discardableResult - func setContent(_ newContent: TabContent) -> Task? { + 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? { + 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 index ee99dfa5d2..e458fff93a 100644 --- 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 index f279dd8fc1..3a59a6de02 100644 --- 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.. 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 index 5d9f91aa0c..e491656a70 100644 --- 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) diff --git a/UnitTests/Permissions/TabPermissionsTests.swift b/UnitTests/Permissions/TabPermissionsTests.swift index ef6e0d3000..3e62249108 100644 --- a/UnitTests/Permissions/TabPermissionsTests.swift +++ b/UnitTests/Permissions/TabPermissionsTests.swift @@ -77,7 +77,7 @@ final class TabPermissionsTests: XCTestCase { return .ok(.html("")) }] - _=await tab.setUrl(urls.url, userEntered: nil)?.value?.result + _=await tab.setUrl(urls.url, userEntered: nil)?.result workspace.appUrl = Bundle.main.bundleURL @@ -158,7 +158,7 @@ final class TabPermissionsTests: XCTestCase { return .ok(.html("")) }] - _=await tab.setUrl(urls.url, userEntered: nil)?.value?.result + _=await tab.setUrl(urls.url, userEntered: nil)?.result workspace.appUrl = Bundle.main.bundleURL @@ -277,7 +277,7 @@ final class TabPermissionsTests: XCTestCase { return .ok(.html("")) }] - _=await tab.setUrl(urls.url, userEntered: nil)?.value?.result + _=await tab.setUrl(urls.url, userEntered: nil)?.result workspace.appUrl = Bundle.main.bundleURL @@ -321,7 +321,7 @@ final class TabPermissionsTests: XCTestCase { return .ok(.html("")) }] - _=await tab.setUrl(urls.url, userEntered: nil)?.value?.result + _=await tab.setUrl(urls.url, userEntered: nil)?.result workspace.appUrl = nil @@ -337,7 +337,7 @@ final class TabPermissionsTests: XCTestCase { XCTFail("Unexpected permissions query \(query)") } - let result = await tab.setUrl(externalUrl, userEntered: nil)?.value?.result + let result = await tab.setUrl(externalUrl, userEntered: nil)?.result guard case .failure(let error) = result else { XCTFail("unexpected result \(String(describing: result))") @@ -359,7 +359,7 @@ final class TabPermissionsTests: XCTestCase { return .ok(.html("")) }] - _=await tab.setUrl(urls.url, userEntered: nil)?.value?.result + _=await tab.setUrl(urls.url, userEntered: nil)?.result workspace.appUrl = nil @@ -375,7 +375,7 @@ final class TabPermissionsTests: XCTestCase { XCTFail("Unexpected permissions query \(query)") } - let result = await tab.setUrl(externalUrl, userEntered: externalUrl.absoluteString)?.value?.result + let result = await tab.setUrl(externalUrl, userEntered: externalUrl.absoluteString)?.result guard case .failure(let error) = result, let error = error as? DidCancelError,