Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (backport #17587)

Client/Coordinators/Browser/BrowserCoordinator.swift

@@ -420,15 +420,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

@@ -161,8 +161,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 {
@@ -460,34 +458,56 @@ 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
 
+<<<<<<< HEAD
     private func subscribeRedux() {
         guard isReduxIntegrationEnabled else { return }
         store.dispatch(ActiveScreensStateAction.showScreen(.fakespot))
+=======
+    func subscribeToRedux() {
+        store.dispatch(ActiveScreensStateAction.showScreen(.browserViewController))
+>>>>>>> f8565a9a1 (Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (#17587))
 
         store.subscribe(self, transform: {
             $0.select(FakespotState.init)
         })
     }
 
+<<<<<<< HEAD
     func newState(state: FakespotState) {
+=======
+    func unsubscribeFromRedux() {
+        store.unsubscribe(self)
+    }
+
+    func newState(state: BrowserViewControllerState) {
+>>>>>>> f8565a9a1 (Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (#17587))
         ensureMainThread { [weak self] in
             guard let self else { return }
 
             fakespotState = state
 
             // opens or close sidebar/bottom sheet to match the saved state
+<<<<<<< HEAD
             if state.isOpenOnProductPage {
                 guard let productURL = urlBar.currentURL else { return }
                 handleFakespotFlow(productURL: productURL)
             } else {
                 _ = dismissFakespotIfNeeded()
+=======
+            if state.fakespotState.isOpen {
+                guard let productURL = urlBar.currentURL else { return }
+                handleFakespotFlow(productURL: productURL)
+            } else if !state.fakespotState.isOpen {
+                dismissFakespotIfNeeded()
+>>>>>>> f8565a9a1 (Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (#17587))
             }
         }
     }
@@ -757,12 +777,17 @@ class BrowserViewController: UIViewController,
         var fakespotNeedsUpdate = false
         if urlBar.currentURL != nil {
             fakespotNeedsUpdate = contentStackView.isSidebarVisible != FakespotUtils().shouldDisplayInSidebar(viewSize: size)
+<<<<<<< HEAD
             if isReduxIntegrationEnabled, let fakespotState = fakespotState {
                 fakespotNeedsUpdate = fakespotNeedsUpdate && fakespotState.isOpenOnProductPage
+=======
+            if let fakespotState = browserViewControllerState?.fakespotState {
+                fakespotNeedsUpdate = fakespotNeedsUpdate && fakespotState.isOpen
+>>>>>>> f8565a9a1 (Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (#17587))
             }
 
             if fakespotNeedsUpdate {
-                _ = dismissFakespotIfNeeded(animated: false)
+                dismissFakespotIfNeeded(animated: false)
             }
         }
 
@@ -1688,26 +1713,48 @@ 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)
+<<<<<<< HEAD
         } else if product.product != nil,
                   !tab.isPrivate,
                   FakespotUtils().shouldDisplayInSidebar(),
                   isReduxIntegrationEnabled,
                   let fakespotState = fakespotState,
                   fakespotState.isOpenOnProductPage {
+=======
+        } else if FakespotUtils().shouldDisplayInSidebar(),
+                  let fakespotState = browserViewControllerState?.fakespotState,
+                  fakespotState.isOpen {
+            // Sidebar should be displayed and Fakespot is open, display Fakespot
+>>>>>>> f8565a9a1 (Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (#17587))
             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

@@ -760,6 +760,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,35 +6,49 @@ import Common
 import Redux
 
 struct FakespotState: ScreenState, Equatable {
-    var isOpenOnProductPage: Bool
+    var isOpen: Bool
+    var sidebarOpenForiPadLandscape: Bool
 
+<<<<<<< HEAD
     init(_ appState: AppState) {
         guard let fakespotState = store.state.screenState(FakespotState.self, for: .fakespot) else {
             self.init()
             return
         }
 
         self.init(isOpenOnProductPage: fakespotState.isOpenOnProductPage)
+=======
+    init(_ appState: BrowserViewControllerState) {
+        self.init(isOpen: appState.fakespotState.isOpen,
+                  sidebarOpenForiPadLandscape: appState.fakespotState.sidebarOpenForiPadLandscape)
+>>>>>>> f8565a9a1 (Refactor FXIOS-7812 [v121] Fakespot - iPad - sidebar open/close state resets itself when user changes from landscape to portrait mode (#17587))
     }
 
     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
     }
 }