From 6fc8a2e3ba467654adc690c9e8f069b5aab44aae Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Tue, 8 Oct 2024 17:23:24 +0200 Subject: [PATCH 01/11] Open DuckPlayer Videos in New Tab #1 --- Core/UserDefaultsPropertyWrapper.swift | 1 + DuckDuckGo/AppSettings.swift | 1 + DuckDuckGo/AppUserDefaults.swift | 5 ++++ .../DuckPlayerNavigationHandler.swift | 29 +++++++++++++++++-- .../DuckPlayerNavigationHandling.swift | 2 ++ DuckDuckGo/TabViewController.swift | 8 ++++- 6 files changed, 43 insertions(+), 3 deletions(-) diff --git a/Core/UserDefaultsPropertyWrapper.swift b/Core/UserDefaultsPropertyWrapper.swift index 6df337d168..2e119e09df 100644 --- a/Core/UserDefaultsPropertyWrapper.swift +++ b/Core/UserDefaultsPropertyWrapper.swift @@ -159,6 +159,7 @@ public struct UserDefaultsWrapper { case duckPlayerMode = "com.duckduckgo.ios.duckPlayerMode" case duckPlayerAskModeOverlayHidden = "com.duckduckgo.ios.duckPlayerAskModeOverlayHidden" case userInteractedWithDuckPlayer = "com.duckduckgo.ios.userInteractedWithDuckPlayer" + case duckPlayerOpenInNewTab = "com.duckduckgo.ios.duckPlayerOpenInNewTab" case vpnRedditWorkaroundInstalled = "com.duckduckgo.ios.vpn.workaroundInstalled" diff --git a/DuckDuckGo/AppSettings.swift b/DuckDuckGo/AppSettings.swift index d3b99acc70..e2c31dda2b 100644 --- a/DuckDuckGo/AppSettings.swift +++ b/DuckDuckGo/AppSettings.swift @@ -82,6 +82,7 @@ protocol AppSettings: AnyObject, AppDebugSettings { var duckPlayerMode: DuckPlayerMode { get set } var duckPlayerAskModeOverlayHidden: Bool { get set } + var duckPlayerOpenInNewTab: Bool { get set } } protocol AppDebugSettings { diff --git a/DuckDuckGo/AppUserDefaults.swift b/DuckDuckGo/AppUserDefaults.swift index 2c54a3a749..ad58c046db 100644 --- a/DuckDuckGo/AppUserDefaults.swift +++ b/DuckDuckGo/AppUserDefaults.swift @@ -78,6 +78,7 @@ public class AppUserDefaults: AppSettings { static let duckPlayerMode = "com.duckduckgo.ios.duckPlayerMode" static let duckPlayerAskModeOverlayHidden = "com.duckduckgo.ios.duckPlayerAskModeOverlayHidden" + static let duckPlayerOpenInNewTab = "com.duckduckgo.ios.duckPlayerOpenInNewTab" } private struct DebugKeys { @@ -414,6 +415,10 @@ public class AppUserDefaults: AppSettings { object: duckPlayerMode) } } + + @UserDefaultsWrapper(key: .duckPlayerOpenInNewTab, defaultValue: false) + var duckPlayerOpenInNewTab: Bool + @UserDefaultsWrapper(key: .debugOnboardingHighlightsEnabledKey, defaultValue: false) var onboardingHighlightsEnabled: Bool diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index b2316a331e..3c77deabbc 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -33,6 +33,7 @@ final class DuckPlayerNavigationHandler { var lastHandledVideoID: String? var featureFlagger: FeatureFlagger var appSettings: AppSettings + var navigationType: WKNavigationType = .other var experiment: DuckPlayerLaunchExperimentHandling private lazy var internalUserDecider = AppDependencyProvider.shared.internalUserDecider @@ -338,7 +339,15 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { webView: WKWebView) { Logger.duckPlayer.debug("Handling DecidePolicyFor for \(navigationAction.request.url?.absoluteString ?? "")") - + + // This means navigation originated in user Event + // and not automatic. This is used further to + // determine how navigation is performed (new tab, etc) + // Resets on next attachment + if navigationAction.navigationType == .linkActivated { + self.navigationType = navigationAction.navigationType + } + guard let url = navigationAction.request.url else { completion(.cancel) return @@ -404,6 +413,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { return } + // Assume JS Navigation is user-triggered + self.navigationType = .linkActivated + handleURLChange(url: url, webView: webView) } @@ -491,12 +503,25 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } // Handle custom events + // This method is used to delegate tasks to DuckPlayerHandler, such as firing pixels and etc. func handleEvent(event: DuckPlayerNavigationEvent, url: URL?, navigationAction: WKNavigationAction?) { - switch event { case .youtubeVideoPageVisited: handleYouTubePageVisited(url: url, navigationAction: navigationAction) } } + // Determine if the links should be open in a new tab, based on the User setting + func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { + // let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = true + let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false + let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk + + if openInNewTab && isDuckPlayer && navigationType == .linkActivated && isDuckPlayerEnabled { + return true + } + return false + } + } diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index 766959fdd2..f2a6e391a8 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -38,4 +38,6 @@ protocol DuckPlayerNavigationHandling: AnyObject { func handleEvent(event: DuckPlayerNavigationEvent, url: URL?, navigationAction: WKNavigationAction?) + func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool + } diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 668ff0364a..4d76752360 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -1886,7 +1886,13 @@ extension TabViewController: WKNavigationDelegate { duckPlayerNavigationHandler?.handleEvent(event: .youtubeVideoPageVisited, url: url, navigationAction: navigationAction) - duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView) + + // Validate Duck Player setting to open in new tab or locally + if duckPlayerNavigationHandler?.shouldOpenInNewTab(navigationAction, webView: webView) ?? false { + delegate?.tab(self, didRequestNewTabForUrl: url, openedByPage: false, inheritingAttribution: nil) + } else { + duckPlayerNavigationHandler?.handleNavigation(navigationAction, webView: webView) + } completion(.cancel) return From 5c75a0c73053ca90ec5a6b82531b2a8a90210838 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 13:25:43 +0200 Subject: [PATCH 02/11] Handle JS Based navigation --- .../DuckPlayerNavigationHandler.swift | 30 ++++++++++++++++++- .../DuckPlayerNavigationHandling.swift | 1 + DuckDuckGo/TabViewController.swift | 6 ++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 3c77deabbc..15fcd502b1 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -245,6 +245,23 @@ final class DuckPlayerNavigationHandler { } + // Determines if the link should be opened in a new tab + // And sets the correct navigationType + // This is uses for JS based navigation links + private func setOpenInNewTab(url: URL?) { + guard let url else { + return + } + let openInNewTab = true + let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk + let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled + if newTab { + navigationType = .linkActivated + } else { + navigationType = .other + } + } + } extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { @@ -508,11 +525,16 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { switch event { case .youtubeVideoPageVisited: handleYouTubePageVisited(url: url, navigationAction: navigationAction) + case .JSTriggeredNavigation: + setOpenInNewTab(url: url) + } } - // Determine if the links should be open in a new tab, based on the User setting + // Determine if the links should be open in a new tab, based on the navigationAction and User setting + // This is used for manually activated links func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { + // let openInNewTab = appSettings.duckPlayerOpenInNewTab let openInNewTab = true let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false @@ -525,3 +547,9 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { } } + +extension WKWebView { + var isEmptyTab: Bool { + return self.url == nil || self.url?.absoluteString == "about:blank" + } +} diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift index f2a6e391a8..2c9ce16bd0 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift @@ -21,6 +21,7 @@ import WebKit enum DuckPlayerNavigationEvent { case youtubeVideoPageVisited + case JSTriggeredNavigation } protocol DuckPlayerNavigationHandling: AnyObject { diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 4d76752360..63798e93b2 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -766,6 +766,12 @@ class TabViewController: UIViewController { } if let url { duckPlayerNavigationHandler?.referrer = url.isYoutube ? .youtube : .other + + // Open in new tab if required + // If the lastRenderedURL is nil, it means we're already in a new tab + if webView.url != nil && lastRenderedURL != nil { + duckPlayerNavigationHandler?.handleEvent(event: .JSTriggeredNavigation, url: webView.url, navigationAction: nil) + } } } From 425ec8fd84fbd420648b41c48cfae9a8b929b9b3 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 16:39:22 +0200 Subject: [PATCH 03/11] Add Setting --- .../DuckPlayer/DuckPlayerNavigationHandler.swift | 4 ++-- DuckDuckGo/SettingsDuckPlayerView.swift | 3 +++ DuckDuckGo/SettingsState.swift | 4 +++- DuckDuckGo/SettingsViewModel.swift | 13 ++++++++++++- DuckDuckGo/UserText.swift | 3 +++ DuckDuckGo/en.lproj/Localizable.strings | 3 +++ 6 files changed, 26 insertions(+), 4 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 15fcd502b1..d2bda3d8a7 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -252,7 +252,7 @@ final class DuckPlayerNavigationHandler { guard let url else { return } - let openInNewTab = true + let openInNewTab = appSettings.duckPlayerOpenInNewTab let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled if newTab { @@ -536,7 +536,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { // let openInNewTab = appSettings.duckPlayerOpenInNewTab - let openInNewTab = true + let openInNewTab = appSettings.duckPlayerOpenInNewTab let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk diff --git a/DuckDuckGo/SettingsDuckPlayerView.swift b/DuckDuckGo/SettingsDuckPlayerView.swift index 8df6eb7678..89349832cb 100644 --- a/DuckDuckGo/SettingsDuckPlayerView.swift +++ b/DuckDuckGo/SettingsDuckPlayerView.swift @@ -72,6 +72,9 @@ struct SettingsDuckPlayerView: View { options: DuckPlayerMode.allCases, selectedOption: viewModel.duckPlayerModeBinding) .disabled(viewModel.shouldDisplayDuckPlayerContingencyMessage) + + SettingsCellView(label: UserText.settingsOpenDuckPlayerNewTabLabel, + accessory: .toggle(isOn: viewModel.duckPlayerOpenInNewTabBinding)) } } .applySettingsListModifiers(title: UserText.duckPlayerFeatureName, diff --git a/DuckDuckGo/SettingsState.swift b/DuckDuckGo/SettingsState.swift index 2548e20354..977df1b404 100644 --- a/DuckDuckGo/SettingsState.swift +++ b/DuckDuckGo/SettingsState.swift @@ -99,6 +99,7 @@ struct SettingsState { // Duck Player Mode var duckPlayerEnabled: Bool var duckPlayerMode: DuckPlayerMode? + var duckPlayerOpenInNewTab: Bool static var defaults: SettingsState { return SettingsState( @@ -136,7 +137,8 @@ struct SettingsState { sync: SyncSettings(enabled: false, title: ""), syncSource: nil, duckPlayerEnabled: false, - duckPlayerMode: .alwaysAsk + duckPlayerMode: .alwaysAsk, + duckPlayerOpenInNewTab: true ) } } diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index 8dbc850832..b3f5ff2e5f 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -278,6 +278,16 @@ final class SettingsViewModel: ObservableObject { } ) } + + var duckPlayerOpenInNewTabBinding: Binding { + Binding( + get: { self.state.duckPlayerOpenInNewTab }, + set: { + self.appSettings.duckPlayerOpenInNewTab = $0 + self.state.duckPlayerOpenInNewTab = $0 + } + ) + } func setVoiceSearchEnabled(to value: Bool) { if value { @@ -410,7 +420,8 @@ extension SettingsViewModel { sync: getSyncState(), syncSource: nil, duckPlayerEnabled: featureFlagger.isFeatureOn(.duckPlayer) || shouldDisplayDuckPlayerContingencyMessage, - duckPlayerMode: appSettings.duckPlayerMode + duckPlayerMode: appSettings.duckPlayerMode, + duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab ) updateRecentlyVisitedSitesVisibility() diff --git a/DuckDuckGo/UserText.swift b/DuckDuckGo/UserText.swift index d120f76951..b8fd72171c 100644 --- a/DuckDuckGo/UserText.swift +++ b/DuckDuckGo/UserText.swift @@ -1277,6 +1277,9 @@ But if you *do* want a peek under the hood, you can find more information about public static let duckPlayerDisabledLabel = NSLocalizedString("duckPlayer.never.label", value: "Never", comment: "Text displayed when DuckPlayer is in off.") public static let settingsOpenVideosInDuckPlayerLabel = NSLocalizedString("duckplayer.settings.open-videos-in", value: "Open Videos in Duck Player", comment: "Settings screen cell text for DuckPlayer settings") public static let duckPlayerFeatureName = NSLocalizedString("duckplayer.settings.title", value: "Duck Player", comment: "Settings screen cell text for DuckPlayer settings") + + public static let settingsOpenDuckPlayerNewTabLabel = NSLocalizedString("duckplayer.settings.open-new-tab-label", value: "Open Duck Player in a new tab", comment: "Settings screen cell text for DuckPlayer settings to open in new tab") + public static let settingsOpenVideosInDuckPlayerTitle = NSLocalizedString("duckplayer.settings.title", value: "Duck Player", comment: "Settings screen cell text for DuckPlayer settings") public static let settingsDuckPlayerFooter = NSLocalizedString("duckplayer.settings.footer", value: "DuckDuckGo provides all the privacy essentials you need to protect yourself as you browse the web.", comment: "Footer label in the settings screen for Duck Player") diff --git a/DuckDuckGo/en.lproj/Localizable.strings b/DuckDuckGo/en.lproj/Localizable.strings index 6c9843290a..04e1c6f25f 100644 --- a/DuckDuckGo/en.lproj/Localizable.strings +++ b/DuckDuckGo/en.lproj/Localizable.strings @@ -1049,6 +1049,9 @@ /* Button that takes the user to learn more about Duck Player. */ "duckplayer.settings.learn-more" = "Learn More"; +/* Settings screen cell text for DuckPlayer settings to open in new tab */ +"duckplayer.settings.open-new-tab-label" = "Open Duck Player in a new tab"; + /* Settings screen cell text for DuckPlayer settings */ "duckplayer.settings.open-videos-in" = "Open Videos in Duck Player"; From 0fef7ec00b1924afc2a31371f5535bcf90e02302 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 17:30:16 +0200 Subject: [PATCH 04/11] Added Unit tests --- .../DuckPlayerNavigationHandler.swift | 7 +- DuckDuckGoTests/AppSettingsMock.swift | 5 +- DuckDuckGoTests/DuckPlayerMocks.swift | 8 +- ...YoutublePlayerNavigationHandlerTests.swift | 89 +++++++++++++++++++ 4 files changed, 103 insertions(+), 6 deletions(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index d2bda3d8a7..510df94c00 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -252,7 +252,7 @@ final class DuckPlayerNavigationHandler { guard let url else { return } - let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled if newTab { @@ -527,7 +527,6 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { handleYouTubePageVisited(url: url, navigationAction: navigationAction) case .JSTriggeredNavigation: setOpenInNewTab(url: url) - } } @@ -536,11 +535,11 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { // let openInNewTab = appSettings.duckPlayerOpenInNewTab - let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk - if openInNewTab && isDuckPlayer && navigationType == .linkActivated && isDuckPlayerEnabled { + if openInNewTab && isDuckPlayer && navigationAction.navigationType == .linkActivated && isDuckPlayerEnabled { return true } return false diff --git a/DuckDuckGoTests/AppSettingsMock.swift b/DuckDuckGoTests/AppSettingsMock.swift index ecdc159b97..dab92872d3 100644 --- a/DuckDuckGoTests/AppSettingsMock.swift +++ b/DuckDuckGoTests/AppSettingsMock.swift @@ -86,7 +86,8 @@ class AppSettingsMock: AppSettings { var duckPlayerMode: DuckDuckGo.DuckPlayerMode = .alwaysAsk var duckPlayerAskModeOverlayHidden: Bool = false - + var duckPlayerOpenInNewTab: Bool = false + var newTabPageShortcutsSettings: Data? var newTabPageSectionsSettings: Data? @@ -94,4 +95,6 @@ class AppSettingsMock: AppSettings { var newTabPageIntroMessageSeenCount: Int = 0 var onboardingHighlightsEnabled: Bool = false + + } diff --git a/DuckDuckGoTests/DuckPlayerMocks.swift b/DuckDuckGoTests/DuckPlayerMocks.swift index 03240debfb..cf387e5578 100644 --- a/DuckDuckGoTests/DuckPlayerMocks.swift +++ b/DuckDuckGoTests/DuckPlayerMocks.swift @@ -76,14 +76,20 @@ class MockWebView: WKWebView { class MockNavigationAction: WKNavigationAction { private let _request: URLRequest + private let _navigationType: WKNavigationType - init(request: URLRequest) { + init(request: URLRequest, navigationType: WKNavigationType = .linkActivated ) { self._request = request + self._navigationType = navigationType } override var request: URLRequest { return _request } + + override var navigationType: WKNavigationType { + return _navigationType + } } final class MockDuckPlayerSettings: DuckPlayerSettings { diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index fcd991c918..034f2d16d3 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -411,5 +411,94 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { } + func testShouldOpenInNewTabWhenEnabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + + mockAppSettings.duckPlayerOpenInNewTab = true + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + + XCTAssertTrue(handler.shouldOpenInNewTab(navigationAction, webView: webView)) + } + + func testShouldNotOpenInNewTabWhenNotDuckPlayerURL() { + let youtubeURL = URL(string: "https://www.youtube.com/watch?v=I9J120SZT14")! + let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + + mockAppSettings.duckPlayerOpenInNewTab = true + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + + XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) + } + + func testShouldNotOpenInNewTabWhenDisabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + let navigationAction = MockNavigationAction(request: URLRequest(url: youtubeURL)) + + mockAppSettings.duckPlayerOpenInNewTab = false + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + + XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) + } + + func testHandleJSNavigationEventWhenEnabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + mockAppSettings.duckPlayerOpenInNewTab = true + + handler.handleEvent(event: .JSTriggeredNavigation, url: youtubeURL, navigationAction: nil) + + XCTAssertTrue(handler.navigationType == .linkActivated) + } + + func testHandleJSNavigationEventWhenDisabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .enabled + mockAppSettings.duckPlayerOpenInNewTab = false + + handler.handleEvent(event: .JSTriggeredNavigation, url: youtubeURL, navigationAction: nil) + + XCTAssertFalse(handler.navigationType == .linkActivated) + } + + func testHandleJSNavigationEventWhenDuckPlayerDisabled() { + let youtubeURL = URL(string: "duck://player/abc123")! + + let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) + let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) + let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + + playerSettings.mode = .disabled + mockAppSettings.duckPlayerOpenInNewTab = true + + handler.handleEvent(event: .JSTriggeredNavigation, url: youtubeURL, navigationAction: nil) + + XCTAssertFalse(handler.navigationType == .linkActivated) + } } From e3510f05ac5de009a4840cfa12d0b12685ed217e Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 18:42:13 +0200 Subject: [PATCH 05/11] Added Feature Flag --- Core/FeatureFlag.swift | 3 +++ .../DuckPlayerNavigationHandler.swift | 24 +++++++++++++++---- DuckDuckGo/SettingsDuckPlayerView.swift | 6 +++-- DuckDuckGo/SettingsState.swift | 4 +++- DuckDuckGo/SettingsViewModel.swift | 4 +++- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/Core/FeatureFlag.swift b/Core/FeatureFlag.swift index 6d5b363635..4c06c6bd79 100644 --- a/Core/FeatureFlag.swift +++ b/Core/FeatureFlag.swift @@ -37,6 +37,7 @@ public enum FeatureFlag: String { case history case newTabPageSections case duckPlayer + case duckPlayerOpenInNewTab case sslCertificatesBypass case syncPromotionBookmarks case syncPromotionPasswords @@ -80,6 +81,8 @@ extension FeatureFlag: FeatureFlagSourceProviding { return .remoteDevelopment(.feature(.newTabPageImprovements)) case .duckPlayer: return .remoteReleasable(.subfeature(DuckPlayerSubfeature.enableDuckPlayer)) + case .duckPlayerOpenInNewTab: + return .remoteReleasable(.subfeature(DuckPlayerSubfeature.openInNewTab)) case .sslCertificatesBypass: return .remoteReleasable(.subfeature(SslCertificatesSubfeature.allowBypass)) case .syncPromotionBookmarks: diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 510df94c00..767b0bac9f 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -252,10 +252,17 @@ final class DuckPlayerNavigationHandler { guard let url else { return } - let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) + + // let openInNewTab = appSettings.duckPlayerOpenInNewTab + let openInNewTab = appSettings.duckPlayerOpenInNewTab + let isFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayer) + let isSubFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayerOpenInNewTab) || internalUserDecider.isInternalUser let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk - let newTab = url.isDuckPlayer && openInNewTab && isDuckPlayerEnabled - if newTab { + + if openInNewTab && + isFeatureEnabled && + isSubFeatureEnabled && + isDuckPlayerEnabled { navigationType = .linkActivated } else { navigationType = .other @@ -535,11 +542,18 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { func shouldOpenInNewTab(_ navigationAction: WKNavigationAction, webView: WKWebView) -> Bool { // let openInNewTab = appSettings.duckPlayerOpenInNewTab - let openInNewTab = appSettings.duckPlayerOpenInNewTab && featureFlagger.isFeatureOn(.duckPlayer) + let openInNewTab = appSettings.duckPlayerOpenInNewTab + let isFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayer) + let isSubFeatureEnabled = featureFlagger.isFeatureOn(.duckPlayerOpenInNewTab) || internalUserDecider.isInternalUser let isDuckPlayer = navigationAction.request.url?.isDuckPlayer ?? false let isDuckPlayerEnabled = duckPlayer.settings.mode == .enabled || duckPlayer.settings.mode == .alwaysAsk - if openInNewTab && isDuckPlayer && navigationAction.navigationType == .linkActivated && isDuckPlayerEnabled { + if openInNewTab && + isFeatureEnabled && + isSubFeatureEnabled && + isDuckPlayer && + navigationAction.navigationType == .linkActivated && + isDuckPlayerEnabled { return true } return false diff --git a/DuckDuckGo/SettingsDuckPlayerView.swift b/DuckDuckGo/SettingsDuckPlayerView.swift index 89349832cb..2c1b9465cb 100644 --- a/DuckDuckGo/SettingsDuckPlayerView.swift +++ b/DuckDuckGo/SettingsDuckPlayerView.swift @@ -73,8 +73,10 @@ struct SettingsDuckPlayerView: View { selectedOption: viewModel.duckPlayerModeBinding) .disabled(viewModel.shouldDisplayDuckPlayerContingencyMessage) - SettingsCellView(label: UserText.settingsOpenDuckPlayerNewTabLabel, - accessory: .toggle(isOn: viewModel.duckPlayerOpenInNewTabBinding)) + if viewModel.state.duckPlayerOpenInNewTabEnabled || viewModel.isInternalUser { + SettingsCellView(label: UserText.settingsOpenDuckPlayerNewTabLabel, + accessory: .toggle(isOn: viewModel.duckPlayerOpenInNewTabBinding)) + } } } .applySettingsListModifiers(title: UserText.duckPlayerFeatureName, diff --git a/DuckDuckGo/SettingsState.swift b/DuckDuckGo/SettingsState.swift index 977df1b404..299cfeba21 100644 --- a/DuckDuckGo/SettingsState.swift +++ b/DuckDuckGo/SettingsState.swift @@ -100,6 +100,7 @@ struct SettingsState { var duckPlayerEnabled: Bool var duckPlayerMode: DuckPlayerMode? var duckPlayerOpenInNewTab: Bool + var duckPlayerOpenInNewTabEnabled: Bool static var defaults: SettingsState { return SettingsState( @@ -138,7 +139,8 @@ struct SettingsState { syncSource: nil, duckPlayerEnabled: false, duckPlayerMode: .alwaysAsk, - duckPlayerOpenInNewTab: true + duckPlayerOpenInNewTab: true, + duckPlayerOpenInNewTabEnabled: false ) } } diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index b3f5ff2e5f..c65e6e70c7 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -421,7 +421,9 @@ extension SettingsViewModel { syncSource: nil, duckPlayerEnabled: featureFlagger.isFeatureOn(.duckPlayer) || shouldDisplayDuckPlayerContingencyMessage, duckPlayerMode: appSettings.duckPlayerMode, - duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab + duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab, + duckPlayerOpenInNewTabEnabled: featureFlagger.isFeatureOn(.duckPlayer) + ) updateRecentlyVisitedSitesVisibility() From 20ea63262e83cd47f5cbfa741d16c53c437c3947 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 19:10:20 +0200 Subject: [PATCH 06/11] Correct Feature Flag --- DuckDuckGo/SettingsViewModel.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index c65e6e70c7..c8739bb941 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -422,7 +422,7 @@ extension SettingsViewModel { duckPlayerEnabled: featureFlagger.isFeatureOn(.duckPlayer) || shouldDisplayDuckPlayerContingencyMessage, duckPlayerMode: appSettings.duckPlayerMode, duckPlayerOpenInNewTab: appSettings.duckPlayerOpenInNewTab, - duckPlayerOpenInNewTabEnabled: featureFlagger.isFeatureOn(.duckPlayer) + duckPlayerOpenInNewTabEnabled: featureFlagger.isFeatureOn(.duckPlayerOpenInNewTab) ) From 5212141fceeacd7b3e74cfc8d77b04f5f4c5a928 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 20:03:31 +0200 Subject: [PATCH 07/11] Use Local navigationType instead --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 2 +- DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index 767b0bac9f..ae0c929a13 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -552,7 +552,7 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { isFeatureEnabled && isSubFeatureEnabled && isDuckPlayer && - navigationAction.navigationType == .linkActivated && + self.navigationType == .linkActivated && isDuckPlayerEnabled { return true } diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index 034f2d16d3..a2f33fcd76 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -493,6 +493,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + handler.navigationType = .linkActivated playerSettings.mode = .disabled mockAppSettings.duckPlayerOpenInNewTab = true From c63c339e7b13b5b7a860369c0e014107bc3ff1ec Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Fri, 11 Oct 2024 20:27:05 +0200 Subject: [PATCH 08/11] Fix unit tests --- DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift index a2f33fcd76..3b50f07aef 100644 --- a/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift +++ b/DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift @@ -421,6 +421,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + handler.navigationType = .linkActivated playerSettings.mode = .enabled XCTAssertTrue(handler.shouldOpenInNewTab(navigationAction, webView: webView)) @@ -435,7 +436,8 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let playerSettings = MockDuckPlayerSettings(appSettings: mockAppSettings, privacyConfigManager: mockPrivacyConfig) let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) - + + handler.navigationType = .linkActivated playerSettings.mode = .enabled XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) @@ -451,6 +453,7 @@ class DuckPlayerNavigationHandlerTests: XCTestCase { let player = MockDuckPlayer(settings: playerSettings, featureFlagger: featureFlagger) let handler = DuckPlayerNavigationHandler(duckPlayer: player, featureFlagger: featureFlagger, appSettings: mockAppSettings, experiment: DuckPlayerExperimentMock()) + handler.navigationType = .linkActivated playerSettings.mode = .enabled XCTAssertFalse(handler.shouldOpenInNewTab(navigationAction, webView: webView)) From 5dd2f22c0fda9ee144a3f2d6cad9a4ea5849554b Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 14 Oct 2024 12:23:32 +0200 Subject: [PATCH 09/11] Adding Pixels --- Core/PixelEvent.swift | 4 ++++ DuckDuckGo/SettingsViewModel.swift | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/Core/PixelEvent.swift b/Core/PixelEvent.swift index 5f450c2359..e274dd0240 100644 --- a/Core/PixelEvent.swift +++ b/Core/PixelEvent.swift @@ -799,6 +799,8 @@ extension Pixel { case duckPlayerSettingNeverOverlayYoutube case duckPlayerContingencySettingsDisplayed case duckPlayerContingencyLearnMoreClicked + case duckPlayerNewTabSettingOn + case duckPlayerNewTabSettingOff // MARK: enhanced statistics case usageSegments @@ -1618,6 +1620,8 @@ extension Pixel.Event { case .duckPlayerSettingNeverOverlayYoutube: return "duckplayer_setting_never_overlay_youtube" case .duckPlayerContingencySettingsDisplayed: return "duckplayer_ios_contingency_settings-displayed" case .duckPlayerContingencyLearnMoreClicked: return "duckplayer_ios_contingency_learn-more-clicked" + case .duckPlayerNewTabSettingOn: return "duckplayer_ios_newtab_setting-on" + case .duckPlayerNewTabSettingOff: return "duckplayer_ios_newtab_setting-off" // MARK: Enhanced statistics case .usageSegments: return "m_retention_segments" diff --git a/DuckDuckGo/SettingsViewModel.swift b/DuckDuckGo/SettingsViewModel.swift index c8739bb941..c9deef5f24 100644 --- a/DuckDuckGo/SettingsViewModel.swift +++ b/DuckDuckGo/SettingsViewModel.swift @@ -285,6 +285,11 @@ final class SettingsViewModel: ObservableObject { set: { self.appSettings.duckPlayerOpenInNewTab = $0 self.state.duckPlayerOpenInNewTab = $0 + if self.state.duckPlayerOpenInNewTab { + Pixel.fire(pixel: Pixel.Event.duckPlayerNewTabSettingOn) + } else { + Pixel.fire(pixel: Pixel.Event.duckPlayerNewTabSettingOff) + } } ) } From 44bd50ee5430116d32e85f9581492633922f13f9 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 14 Oct 2024 19:36:15 +0200 Subject: [PATCH 10/11] Only trigger JS Navigation if not WatchInYoutube --- DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift index ae0c929a13..4c9e4f2c71 100644 --- a/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift +++ b/DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift @@ -440,7 +440,11 @@ extension DuckPlayerNavigationHandler: DuckPlayerNavigationHandling { // Assume JS Navigation is user-triggered self.navigationType = .linkActivated - handleURLChange(url: url, webView: webView) + // Only handle URL changes if the allowFirstVideo is set to false + // This prevents Youtube redirects from triggering DuckPlayer when is not expected + if !duckPlayer.settings.allowFirstVideo { + handleURLChange(url: url, webView: webView) + } } @MainActor From d59d70010a9d2582a025f68f305d4ad5b2e584b2 Mon Sep 17 00:00:00 2001 From: Daniel Bernal Date: Mon, 14 Oct 2024 19:56:20 +0200 Subject: [PATCH 11/11] Default to true --- DuckDuckGo/AppUserDefaults.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DuckDuckGo/AppUserDefaults.swift b/DuckDuckGo/AppUserDefaults.swift index ad58c046db..408b03a116 100644 --- a/DuckDuckGo/AppUserDefaults.swift +++ b/DuckDuckGo/AppUserDefaults.swift @@ -416,7 +416,7 @@ public class AppUserDefaults: AppSettings { } } - @UserDefaultsWrapper(key: .duckPlayerOpenInNewTab, defaultValue: false) + @UserDefaultsWrapper(key: .duckPlayerOpenInNewTab, defaultValue: true) var duckPlayerOpenInNewTab: Bool