Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state…

… resets itself when user changes from landscape to portrait mode (#17587)

* Dismiss sidebar when opting out

* Dismiss sidebar when tapping not now

* Use state to dismiss and show Fakespot

* Dismiss Fakespot when startAtHome

* Use iPad sidebar state

* Update Fakespot when user navigates using the back/forward buttons

* Update Fakespot when user navigates using the back/forward buttons

* Update Fakespot actions

* Open and close fakespot via state change

* Fix bugs related to deep-linking

* Fix test

* Address PR comments

* Fix merge issues

* Fix Swiftlint warning

Client/Coordinators/Browser/BrowserCoordinator.swift

@@ -432,15 +432,15 @@ class BrowserCoordinator: BaseCoordinator,
         guard let fakespotCoordinator = childCoordinators.first(where: { $0 is FakespotCoordinator }) as? FakespotCoordinator else {
             return // there is no modal to close
         }
-        fakespotCoordinator.fakespotControllerDidDismiss(animated: animated)
+        fakespotCoordinator.dismissModal(animated: animated)
     }
 
     func dismissFakespotSidebar(sidebarContainer: SidebarEnabledViewProtocol, parentViewController: UIViewController) {
         guard let fakespotCoordinator = childCoordinators.first(where: { $0 is FakespotCoordinator }) as? FakespotCoordinator else {
             return // there is no sidebar to close
         }
-        fakespotCoordinator.fakespotControllerCloseSidebar(sidebarContainer: sidebarContainer,
-                                                           parentViewController: parentViewController)
+        fakespotCoordinator.closeSidebar(sidebarContainer: sidebarContainer,
+                                         parentViewController: parentViewController)
     }
 
     func updateFakespotSidebar(productURL: URL,

Client/Frontend/Browser/BrowserViewController.swift

@@ -169,8 +169,6 @@ class BrowserViewController: UIViewController,
         return NewTabAccessors.getNewTabPage(profile.prefs)
     }
 
-    lazy var isReduxIntegrationEnabled: Bool = ReduxFlagManager.isReduxEnabled
-
     @available(iOS 13.4, *)
     func keyboardPressesHandler() -> KeyboardPressesHandler {
         guard let keyboardPressesHandlerValue = keyboardPressesHandlerValue as? KeyboardPressesHandler else {
@@ -453,15 +451,15 @@ class BrowserViewController: UIViewController,
 
     private func dismissModalsIfStartAtHome() {
         if tabManager.startAtHomeCheck() {
-            guard !dismissFakespotIfNeeded(), presentedViewController != nil else { return }
+            store.dispatch(FakespotAction.setAppearanceTo(false))
+            guard presentedViewController != nil else { return }
             dismissVC()
         }
     }
 
     // MARK: - Redux
 
     func subscribeToRedux() {
-        guard isReduxIntegrationEnabled else { return }
         store.dispatch(ActiveScreensStateAction.showScreen(.browserViewController))
 
         store.subscribe(self, transform: {
@@ -470,9 +468,7 @@ class BrowserViewController: UIViewController,
     }
 
     func unsubscribeFromRedux() {
-        if isReduxIntegrationEnabled {
-            store.unsubscribe(self)
-        }
+        store.unsubscribe(self)
     }
 
     func newState(state: BrowserViewControllerState) {
@@ -482,11 +478,11 @@ class BrowserViewController: UIViewController,
             browserViewControllerState = state
 
             // opens or close sidebar/bottom sheet to match the saved state
-            if state.fakespotState.isOpenOnProductPage {
+            if state.fakespotState.isOpen {
                 guard let productURL = urlBar.currentURL else { return }
                 handleFakespotFlow(productURL: productURL)
-            } else if !state.fakespotState.isOpenOnProductPage {
-                _ = dismissFakespotIfNeeded()
+            } else if !state.fakespotState.isOpen {
+                dismissFakespotIfNeeded()
             }
         }
     }
@@ -766,12 +762,12 @@ class BrowserViewController: UIViewController,
         var fakespotNeedsUpdate = false
         if urlBar.currentURL != nil {
             fakespotNeedsUpdate = contentStackView.isSidebarVisible != FakespotUtils().shouldDisplayInSidebar(viewSize: size)
-            if isReduxIntegrationEnabled, let fakespotState = browserViewControllerState?.fakespotState {
-                fakespotNeedsUpdate = fakespotNeedsUpdate && fakespotState.isOpenOnProductPage
+            if let fakespotState = browserViewControllerState?.fakespotState {
+                fakespotNeedsUpdate = fakespotNeedsUpdate && fakespotState.isOpen
             }
 
             if fakespotNeedsUpdate {
-                _ = dismissFakespotIfNeeded(animated: false)
+                dismissFakespotIfNeeded(animated: false)
             }
         }
 
@@ -1663,26 +1659,39 @@ class BrowserViewController: UIViewController,
         }
     }
 
-    private func updateFakespot(tab: Tab) {
+    internal func updateFakespot(tab: Tab) {
         guard let webView = tab.webView, let url = webView.url else {
+            // We're on homepage or a blank tab
+            store.dispatch(FakespotAction.setAppearanceTo(false))
             return
         }
 
         let environment = featureFlags.isCoreFeatureEnabled(.useStagingFakespotAPI) ? FakespotEnvironment.staging : .prod
         let product = ShoppingProduct(url: url, client: FakespotClient(environment: environment))
-        if product.product != nil, !tab.isPrivate, contentStackView.isSidebarVisible {
+
+        guard product.product != nil, !tab.isPrivate else {
+            store.dispatch(FakespotAction.setAppearanceTo(false))
+
+            // Quick fix: make sure to sidebar is hidden when opened from deep-link
+            // Relates to FXIOS-7844
+            contentStackView.hideSidebar(self)
+            return
+        }
+
+        if contentStackView.isSidebarVisible {
+            // Sidebar is visible, update content
             navigationHandler?.updateFakespotSidebar(productURL: url,
                                                      sidebarContainer: contentStackView,
                                                      parentViewController: self)
-        } else if product.product != nil,
-                  !tab.isPrivate,
-                  FakespotUtils().shouldDisplayInSidebar(),
-                  isReduxIntegrationEnabled,
+        } else if FakespotUtils().shouldDisplayInSidebar(),
                   let fakespotState = browserViewControllerState?.fakespotState,
-                  fakespotState.isOpenOnProductPage {
+                  fakespotState.isOpen {
+            // Sidebar should be displayed and Fakespot is open, display Fakespot
             handleFakespotFlow(productURL: url)
-        } else if contentStackView.isSidebarVisible {
-            _ = dismissFakespotIfNeeded(animated: true)
+        } else if let fakespotState = browserViewControllerState?.fakespotState,
+                  fakespotState.sidebarOpenForiPadLandscape {
+            // Sidebar should be displayed, display Fakespot
+            store.dispatch(FakespotAction.setAppearanceTo(true))
         }
     }
 

Client/Frontend/Browser/BrowserViewController/BrowserViewController+URLBarDelegate.swift

@@ -113,38 +113,26 @@ extension BrowserViewController: URLBarDelegate {
         }
     }
 
-    func urlBarDidPressShopping(_ urlBar: URLBarView, shoppingButton: UIButton) {
-        guard let productURL = urlBar.currentURL else { return }
-        TelemetryWrapper.recordEvent(category: .action, method: .tap, object: .shoppingButton)
-
-        let dismissedFakespot = dismissFakespotIfNeeded()
-        if !dismissedFakespot {
-            // open flow
-            handleFakespotFlow(productURL: productURL)
-            if isReduxIntegrationEnabled {
-                store.dispatch(FakespotAction.toggleAppearance(true))
-            }
-        } else if isReduxIntegrationEnabled {
-            // Fakespot was closed/dismissed
-            store.dispatch(FakespotAction.toggleAppearance(false))
-        }
-    }
-
-    internal func dismissFakespotIfNeeded(animated: Bool = true) -> Bool {
-        if contentStackView.isSidebarVisible {
+    internal func dismissFakespotIfNeeded(animated: Bool = true) {
+        guard !contentStackView.isSidebarVisible else {
             // hide sidebar as user tapped on shopping icon for a second time
             navigationHandler?.dismissFakespotSidebar(sidebarContainer: contentStackView, parentViewController: self)
-            return true
-        } else if presentedViewController as? FakespotViewController != nil {
-            // dismiss modal as user tapped on shopping icon for a second time
-            navigationHandler?.dismissFakespotModal(animated: animated)
-            return true
+            return
         }
-        return false
+
+        // dismiss modal as user tapped on shopping icon for a second time
+        navigationHandler?.dismissFakespotModal(animated: animated)
     }
 
     internal func handleFakespotFlow(productURL: URL, viewSize: CGSize? = nil) {
-        if FakespotUtils().shouldDisplayInSidebar(viewSize: viewSize) {
+        let shouldDisplayInSidebar = FakespotUtils().shouldDisplayInSidebar(viewSize: viewSize)
+        if !shouldDisplayInSidebar, contentStackView.isSidebarVisible {
+            // Quick fix: make sure to sidebar is hidden
+            // Relates to FXIOS-7844
+            contentStackView.hideSidebar(self)
+        }
+
+        if shouldDisplayInSidebar {
             navigationHandler?.showFakespotFlowAsSidebar(productURL: productURL,
                                                          sidebarContainer: contentStackView,
                                                          parentViewController: self)
@@ -171,10 +159,8 @@ extension BrowserViewController: URLBarDelegate {
                     value: .shoppingCFRsDisplayed
                 )
             },
-            andActionForButton: { [weak self] in
-                guard let self else { return }
-                guard let productURL = self.urlBar.currentURL else { return }
-                self.handleFakespotFlow(productURL: productURL)
+            andActionForButton: {
+                store.dispatch(FakespotAction.show)
             },
             overlayState: overlayManager)
     }

Client/Frontend/Browser/BrowserViewController/BrowserViewController+WebViewDelegates.swift

@@ -764,6 +764,7 @@ extension BrowserViewController: WKNavigationDelegate {
         if tabManager.selectedTab === tab {
             self.scrollController.showToolbars(animated: false)
             updateUIForReaderHomeStateForTab(tab, focusUrlBar: true)
+            updateFakespot(tab: tab)
         }
     }
 

Client/Frontend/Fakespot/FakespotAction.swift

@@ -7,5 +7,8 @@ import Redux
 
 enum FakespotAction: Action {
     // UI trigger actions
-    case toggleAppearance(Bool)
+    case pressedShoppingButton
+    case show
+    case dismiss
+    case setAppearanceTo(Bool)
 }

Client/Frontend/Fakespot/FakespotCoordinator.swift

@@ -10,11 +10,7 @@ protocol FakespotCoordinatorDelegate: AnyObject {
     // Define any coordinator delegate methods
 }
 
-protocol FakespotViewControllerDelegate: AnyObject {
-    func fakespotControllerDidDismiss(animated: Bool)
-}
-
-class FakespotCoordinator: BaseCoordinator, FakespotViewControllerDelegate, FeatureFlaggable {
+class FakespotCoordinator: BaseCoordinator, FeatureFlaggable {
     weak var parentCoordinator: ParentCoordinatorDelegate?
     private var profile: Profile
 
@@ -48,13 +44,13 @@ class FakespotCoordinator: BaseCoordinator, FakespotViewControllerDelegate, Feat
         sidebarContainer.showSidebar(viewController, parentViewController: parentViewController)
     }
 
-    func fakespotControllerCloseSidebar(sidebarContainer: SidebarEnabledViewProtocol,
-                                        parentViewController: UIViewController) {
+    func closeSidebar(sidebarContainer: SidebarEnabledViewProtocol,
+                      parentViewController: UIViewController) {
         sidebarContainer.hideSidebar(parentViewController)
-        fakespotControllerDidDismiss(animated: true)
+        dismissModal(animated: true)
     }
 
-    func fakespotControllerDidDismiss(animated: Bool) {
+    func dismissModal(animated: Bool) {
         router.dismiss(animated: animated, completion: nil)
         parentCoordinator?.didFinish(from: self)
     }
@@ -69,7 +65,6 @@ class FakespotCoordinator: BaseCoordinator, FakespotViewControllerDelegate, Feat
     private func createFakespotViewController(productURL: URL) -> FakespotViewController {
         let viewModel = createFakespotViewModel(productURL: productURL)
         let fakespotViewController = FakespotViewController(viewModel: viewModel)
-        fakespotViewController.delegate = self
         return fakespotViewController
     }
 

Client/Frontend/Fakespot/FakespotState.swift

@@ -6,30 +6,39 @@ import Common
 import Redux
 
 struct FakespotState: ScreenState, Equatable {
-    var isOpenOnProductPage: Bool
+    var isOpen: Bool
+    var sidebarOpenForiPadLandscape: Bool
 
     init(_ appState: BrowserViewControllerState) {
-        self.init(isOpenOnProductPage: appState.fakespotState.isOpenOnProductPage)
+        self.init(isOpen: appState.fakespotState.isOpen,
+                  sidebarOpenForiPadLandscape: appState.fakespotState.sidebarOpenForiPadLandscape)
     }
 
     init() {
-        self.init(isOpenOnProductPage: false)
+        self.init(isOpen: false, sidebarOpenForiPadLandscape: false)
     }
 
-    init(isOpenOnProductPage: Bool) {
-        self.isOpenOnProductPage = isOpenOnProductPage
+    init(isOpen: Bool, sidebarOpenForiPadLandscape: Bool) {
+        self.isOpen = isOpen
+        self.sidebarOpenForiPadLandscape = sidebarOpenForiPadLandscape
     }
 
     static let reducer: Reducer<Self> = { state, action in
         switch action {
-        case FakespotAction.toggleAppearance(let isEnabled):
-            return FakespotState(isOpenOnProductPage: isEnabled)
+        case FakespotAction.pressedShoppingButton:
+            return FakespotState(isOpen: !state.isOpen,
+                                 sidebarOpenForiPadLandscape: !state.isOpen)
+        case FakespotAction.show:
+            return FakespotState(isOpen: true,
+                                 sidebarOpenForiPadLandscape: true)
+        case FakespotAction.dismiss:
+            return FakespotState(isOpen: false,
+                                 sidebarOpenForiPadLandscape: false)
+        case FakespotAction.setAppearanceTo(let isEnabled):
+            return FakespotState(isOpen: isEnabled,
+                                 sidebarOpenForiPadLandscape: state.sidebarOpenForiPadLandscape)
         default:
             return state
         }
     }
-
-    static func == (lhs: FakespotState, rhs: FakespotState) -> Bool {
-        return lhs.isOpenOnProductPage == rhs.isOpenOnProductPage
-    }
 }

Client/Frontend/Fakespot/FakespotUtils.swift

@@ -88,8 +88,12 @@ public struct FakespotUtils: FeatureFlaggable {
     func shouldDisplayInSidebar(device: UIUserInterfaceIdiom = UIDevice.current.userInterfaceIdiom,
                                 window: UIWindow? = UIWindow.attachedKeyWindow,
                                 viewSize: CGSize? = nil,
+                                isPortrait: Bool = UIWindow.isPortrait,
                                 orientation: UIDeviceOrientation = UIDevice.current.orientation) -> Bool {
         let isPadInMultitasking = isPadInMultitasking(device: device, window: window, viewSize: viewSize)
-        return device == .pad && !isPadInMultitasking && !orientation.isPortrait
+
+        // UIDevice is not always returning the correct orientation so we check against the window orientation as well
+        let isPortrait = orientation.isPortrait || isPortrait
+        return device == .pad && !isPadInMultitasking && !isPortrait
     }
 }

Client/Frontend/Fakespot/FakespotViewController.swift

@@ -41,7 +41,6 @@ class FakespotViewController:
     var themeManager: ThemeManager
     var themeObserver: NSObjectProtocol?
     private var viewModel: FakespotViewModel
-    weak var delegate: FakespotViewControllerDelegate?
 
     lazy var isReduxIntegrationEnabled: Bool = ReduxFlagManager.isReduxEnabled
 
@@ -365,9 +364,9 @@ class FakespotViewController:
         case .onboarding:
             let view: FakespotOptInCardView = .build()
             viewModel.optInCardViewModel.dismissViewController = { [weak self] action in
-                guard let self = self else { return }
-                self.delegate?.fakespotControllerDidDismiss(animated: true)
-                guard let action else { return }
+                store.dispatch(FakespotAction.setAppearanceTo(false))
+
+                guard let self = self, let action else { return }
                 viewModel.recordDismissTelemetry(by: action)
             }
             viewModel.optInCardViewModel.onOptIn = { [weak self] in
@@ -397,9 +396,8 @@ class FakespotViewController:
 
         case .qualityDeterminationCard:
             let reviewQualityCardView: FakespotReviewQualityCardView = .build()
-            viewModel.reviewQualityCardViewModel.dismissViewController = { [weak self] in
-                guard let self = self else { return }
-                self.delegate?.fakespotControllerDidDismiss(animated: true)
+            viewModel.reviewQualityCardViewModel.dismissViewController = {
+                store.dispatch(FakespotAction.setAppearanceTo(false))
             }
             reviewQualityCardView.configure(viewModel.reviewQualityCardViewModel)
             return reviewQualityCardView
@@ -408,9 +406,9 @@ class FakespotViewController:
             let view: FakespotSettingsCardView = .build()
             view.configure(viewModel.settingsCardViewModel)
             viewModel.settingsCardViewModel.dismissViewController = { [weak self] action in
-                guard let self = self else { return }
-                self.delegate?.fakespotControllerDidDismiss(animated: true)
-                guard let action else { return }
+                guard let self = self, let action else { return }
+
+                store.dispatch(FakespotAction.setAppearanceTo(false))
                 viewModel.recordDismissTelemetry(by: action)
             }
             viewModel.settingsCardViewModel.toggleAdsEnabled = { [weak self] in
@@ -430,9 +428,8 @@ class FakespotViewController:
             guard viewModel.areAdsEnabled else { return nil }
             let view: FakespotAdView = .build()
             var viewModel = FakespotAdViewModel(productAdsData: adData)
-            viewModel.dismissViewController = { [weak self] in
-                guard let self = self else { return }
-                self.delegate?.fakespotControllerDidDismiss(animated: true)
+            viewModel.dismissViewController = {
+                store.dispatch(FakespotAction.setAppearanceTo(false))
             }
             view.configure(viewModel)
             adView = view
@@ -506,11 +503,7 @@ class FakespotViewController:
     }
 
     private func triggerDismiss() {
-        delegate?.fakespotControllerDidDismiss(animated: true)
-
-        if isReduxIntegrationEnabled {
-            store.dispatch(FakespotAction.toggleAppearance(false))
-        }
+        store.dispatch(FakespotAction.dismiss)
     }
 
     deinit {