Bugfix FXIOS-7646 [v120] The 3rd CFR is not displayed (#17172)

* Use one instance for the shopping CFR * Rename contextHintVC and add new unit test * Initialize shopping contextual hint in init of BVC * Fix crash that can happen when manually changing time on the device * Fix stackview layout by using UIButton configuration * Change UIButton to LinkButton * Remove configuration from actionButton * Add a11y id and move button insets to UX struct --------- Co-authored-by: Winnie Teichmann <[email protected]>
mozilla-mobile · Nov 9, 2023 · 7d43a40 · 7d43a40
1 parent 1578d63
commit 7d43a40
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 28 deletions.
diff --git a/BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintView.swift b/BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintView.swift
@@ -15,7 +15,8 @@ public class ContextualHintView: UIView, ThemeApplicable {
         static let closeButtonTrailing: CGFloat = 5
         static let closeButtonTop: CGFloat = 23
         static let closeButtonBottom: CGFloat = 12
-        static let closeButtonInset = UIEdgeInsets(top: 0, left: 7.5, bottom: 15, right: 7.5)
+        static let closeButtonInsets = NSDirectionalEdgeInsets(top: 0, leading: 7.5, bottom: 15, trailing: 7.5)
+        static let actionButtonInsets = NSDirectionalEdgeInsets(top: 0, leading: 0, bottom: 0, trailing: 0)
         static let descriptionTextSize: CGFloat = 17
         static let stackViewLeading: CGFloat = 16
         static let stackViewTopArrowTopConstraint: CGFloat = 16
@@ -27,10 +28,12 @@ public class ContextualHintView: UIView, ThemeApplicable {
     // MARK: - UI Elements
     private lazy var contentContainer: UIView = .build { _ in }
 
-    private lazy var closeButton: ActionButton = .build { button in
+    private lazy var closeButton: UIButton = .build { button in
         button.setImage(UIImage(named: StandardImageIdentifiers.Medium.cross)?.withRenderingMode(.alwaysTemplate),
                         for: .normal)
-        button.contentEdgeInsets = UX.closeButtonInset
+        button.addTarget(self, action: #selector(self.didTapCloseButton), for: .touchUpInside)
+        button.configuration = .plain()
+        button.configuration?.contentInsets = UX.closeButtonInsets
     }
 
     private lazy var descriptionLabel: UILabel = .build { label in
@@ -39,10 +42,10 @@ public class ContextualHintView: UIView, ThemeApplicable {
         label.numberOfLines = 0
     }
 
-    private lazy var actionButton: ActionButton = .build { button in
+    private lazy var actionButton: LinkButton = .build { button in
         button.titleLabel?.textAlignment = .left
         button.titleLabel?.numberOfLines = 0
-        button.buttonEdgeSpacing = 0
+        button.addTarget(self, action: #selector(self.didTapActionButton), for: .touchUpInside)
     }
 
     private lazy var stackView: UIStackView = .build { stack in
@@ -75,6 +78,13 @@ public class ContextualHintView: UIView, ThemeApplicable {
     public func configure(viewModel: ContextualHintViewModel) {
         self.viewModel = viewModel
 
+        let actionButtonViewModel = LinkButtonViewModel(
+            title: viewModel.actionButtonTitle,
+            a11yIdentifier: viewModel.actionButtonA11yId,
+            contentInsets: UX.actionButtonInsets
+        )
+        actionButton.configure(viewModel: actionButtonViewModel)
+
         closeButton.accessibilityLabel = viewModel.closeButtonA11yLabel
         descriptionLabel.text = viewModel.description
 
@@ -90,9 +100,6 @@ public class ContextualHintView: UIView, ThemeApplicable {
         if viewModel.isActionType { stackView.addArrangedSubview(actionButton) }
 
         setupConstraints()
-
-        closeButton.touchUpAction = viewModel.closeButtonAction
-        actionButton.touchUpAction = viewModel.actionButtonAction
     }
 
     private func setupConstraints() {
@@ -132,6 +139,16 @@ public class ContextualHintView: UIView, ThemeApplicable {
         layoutIfNeeded()
     }
 
+    @objc
+    private func didTapCloseButton(sender: UIButton) {
+        viewModel.closeButtonAction?(sender)
+    }
+
+    @objc
+    private func didTapActionButton(sender: UIButton) {
+        viewModel.actionButtonAction?(sender)
+    }
+
     public func applyTheme(theme: Theme) {
         closeButton.tintColor = theme.colors.textOnDark
         descriptionLabel.textColor = theme.colors.textOnDark

diff --git a/BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintViewModel.swift b/BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintViewModel.swift
@@ -11,6 +11,7 @@ public struct ContextualHintViewModel {
     public var description: String
     public var arrowDirection: UIPopoverArrowDirection
     public var closeButtonA11yLabel: String
+    public var actionButtonA11yId: String
 
     public var closeButtonAction: ((UIButton) -> Void)?
     public var actionButtonAction: ((UIButton) -> Void)?
@@ -19,11 +20,13 @@ public struct ContextualHintViewModel {
                 actionButtonTitle: String,
                 description: String,
                 arrowDirection: UIPopoverArrowDirection,
-                closeButtonA11yLabel: String) {
+                closeButtonA11yLabel: String,
+                actionButtonA11yId: String) {
         self.isActionType = isActionType
         self.actionButtonTitle = actionButtonTitle
         self.description = description
         self.arrowDirection = arrowDirection
         self.closeButtonA11yLabel = closeButtonA11yLabel
+        self.actionButtonA11yId = actionButtonA11yId
     }
 }
diff --git a/Client/Application/AccessibilityIdentifiers.swift b/Client/Application/AccessibilityIdentifiers.swift
@@ -44,6 +44,10 @@ public struct AccessibilityIdentifiers {
         }
     }
 
+    struct ContextualHints {
+        static let actionButton = "ContextualHints.ActionButton"
+    }
+
     struct FirefoxHomepage {
         static let collectionView = "FxCollectionView"
 

diff --git a/Client/Frontend/Browser/BrowserViewController.swift b/Client/Frontend/Browser/BrowserViewController.swift
@@ -59,7 +59,8 @@ class BrowserViewController: UIViewController,
     var passBookHelper: OpenPassBookHelper?
     var overlayManager: OverlayModeManager
     var appAuthenticator: AppAuthenticationProtocol
-    var contextHintVC: ContextualHintViewController
+    var toolbarContextHintVC: ContextualHintViewController
+    let shoppingContextHintVC: ContextualHintViewController
     private var backgroundTabLoader: DefaultBackgroundTabLoader
 
     // popover rotation handling
@@ -189,7 +190,10 @@ class BrowserViewController: UIViewController,
         self.overlayManager = DefaultOverlayModeManager()
         let contextualViewProvider = ContextualHintViewProvider(forHintType: .toolbarLocation,
                                                                 with: profile)
-        self.contextHintVC = ContextualHintViewController(with: contextualViewProvider)
+        self.toolbarContextHintVC = ContextualHintViewController(with: contextualViewProvider)
+        let shoppingViewProvider = ContextualHintViewProvider(forHintType: .shoppingExperience,
+                                                              with: profile)
+        shoppingContextHintVC = ContextualHintViewController(with: shoppingViewProvider)
         self.backgroundTabLoader = DefaultBackgroundTabLoader(tabQueue: profile.queue)
         super.init(nibName: nil, bundle: nil)
         didInit()
@@ -640,11 +644,11 @@ class BrowserViewController: UIViewController,
     }
 
     private func prepareURLOnboardingContextualHint() {
-        guard contextHintVC.shouldPresentHint(),
+        guard toolbarContextHintVC.shouldPresentHint(),
               featureFlags.isFeatureEnabled(.isToolbarCFREnabled, checking: .buildOnly)
         else { return }
 
-        contextHintVC.configure(
+        toolbarContextHintVC.configure(
             anchor: urlBar,
             withArrowDirection: isBottomSearchBar ? .down : .up,
             andDelegate: self,
@@ -655,9 +659,9 @@ class BrowserViewController: UIViewController,
 
     private func presentContextualHint() {
         if IntroScreenManager(prefs: profile.prefs).shouldShowIntroScreen { return }
-        present(contextHintVC, animated: true)
+        present(toolbarContextHintVC, animated: true)
 
-        UIAccessibility.post(notification: .layoutChanged, argument: contextHintVC)
+        UIAccessibility.post(notification: .layoutChanged, argument: toolbarContextHintVC)
     }
 
     override func viewWillDisappear(_ animated: Bool) {

diff --git a/Client/Frontend/Browser/BrowserViewController/BrowserViewController+URLBarDelegate.swift b/Client/Frontend/Browser/BrowserViewController/BrowserViewController+URLBarDelegate.swift
@@ -161,22 +161,22 @@ extension BrowserViewController: URLBarDelegate {
             didStartAtHome = false
             return
         }
+        configureShoppingContextVC(at: sourceView)
+    }
 
-        let contextualViewProvider = ContextualHintViewProvider(forHintType: .shoppingExperience,
-                                                                with: profile)
-
-        let contextHintVC = ContextualHintViewController(with: contextualViewProvider)
-
-        contextHintVC.configure(
+    private func configureShoppingContextVC(at sourceView: UIView) {
+        shoppingContextHintVC.configure(
             anchor: sourceView,
             withArrowDirection: isBottomSearchBar ? .down : .up,
             andDelegate: self,
-            presentedUsing: {
-                self.present(contextHintVC, animated: true)
-                TelemetryWrapper.recordEvent(category: .action,
-                                             method: .navigate,
-                                             object: .shoppingButton,
-                                             value: .shoppingCFRsDisplayed)
+            presentedUsing: { [unowned self] in
+                self.present(shoppingContextHintVC, animated: true)
+                TelemetryWrapper.recordEvent(
+                    category: .action,
+                    method: .navigate,
+                    object: .shoppingButton,
+                    value: .shoppingCFRsDisplayed
+                )
             },
             andActionForButton: { [weak self] in
                 guard let self else { return }

diff --git a/Client/Frontend/ContextualHint/ContextualHintEligibilityUtility.swift b/Client/Frontend/ContextualHint/ContextualHintEligibilityUtility.swift
@@ -113,6 +113,7 @@ struct ContextualHintEligibilityUtility: ContextualHintEligibilityUtilityProtoco
             if cfrCounter <= 2, !hasOptedIn, hasTimePassed {
                 // - Display CFR-1
                 profile.prefs.setInt(cfrCounter + 1, forKey: PrefsKeys.ContextualHints.shoppingOnboardingCFRsCounterKey.rawValue)
+                profile.prefs.setTimestamp(Date.now(), forKey: PrefsKeys.FakespotLastCFRTimestamp)
                 return true
             } else if cfrCounter < 4, hasOptedIn, hasTimePassed {
                 // - Display CFR-2

diff --git a/Client/Frontend/ContextualHint/ContextualHintViewController.swift b/Client/Frontend/ContextualHint/ContextualHintViewController.swift
@@ -143,7 +143,8 @@ class ContextualHintViewController: UIViewController, OnViewDismissable, Themeab
                                                 actionButtonTitle: viewProvider.getCopyFor(.action),
                                                 description: viewProvider.getCopyFor(.description),
                                                 arrowDirection: arrowDirection,
-                                                closeButtonA11yLabel: .ContextualHints.ContextualHintsCloseAccessibility)
+                                                closeButtonA11yLabel: .ContextualHints.ContextualHintsCloseAccessibility,
+                                                actionButtonA11yId: AccessibilityIdentifiers.ContextualHints.actionButton)
         viewModel.closeButtonAction = { [weak self] _ in
             self?.dismissAnimated()
         }

diff --git a/Shared/TimeConstants.swift b/Shared/TimeConstants.swift
@@ -150,6 +150,8 @@ extension Date {
     /// - Returns: `true` if the specified time in hours has passed since the lastTimestamp; `false` otherwise.
     public static func hasTimePassedBy(hours: Timestamp,
                                        lastTimestamp: Timestamp) -> Bool {
+        guard Date.now() > lastTimestamp else { return false }
+
         let millisecondsInAnHour: Timestamp = 3_600_000 // Convert 1 hour to milliseconds
         let timeDifference = Date.now() - lastTimestamp
         return timeDifference >= hours * millisecondsInAnHour

diff --git a/Tests/ClientTests/Frontend/ContextualHints/ContextualHintEligibilityUtilityTests.swift b/Tests/ClientTests/Frontend/ContextualHints/ContextualHintEligibilityUtilityTests.swift
@@ -227,4 +227,17 @@ class ContextualHintEligibilityUtilityTests: XCTestCase {
         let result = subject.canPresent(.shoppingExperience)
         XCTAssertFalse(result)
     }
+
+    func test_canPresentShoppingCFR_TwoConsecutiveCFRs_UserHasNotOptedIn_() {
+        let lastTimestamp: Timestamp = 1695719918000 // Date and time (GMT): Tuesday, 26 September 2023 09:18:38
+
+        profile.prefs.setBool(true, forKey: CFRPrefsKeys.shoppingOnboardingKey.rawValue)
+        profile.prefs.setTimestamp(lastTimestamp, forKey: PrefsKeys.FakespotLastCFRTimestamp)
+        profile.prefs.setBool(false, forKey: PrefsKeys.Shopping2023OptIn)
+
+        let canPresentFirstCFR = subject.canPresent(.shoppingExperience)
+        XCTAssertTrue(canPresentFirstCFR)
+        let canPresentSecondCFR = subject.canPresent(.shoppingExperience)
+        XCTAssertFalse(canPresentSecondCFR)
+    }
 }