From 7d43a40dde9629dd1fce98eb9d84ae8b182fc9d4 Mon Sep 17 00:00:00 2001 From: PARAIPAN SORIN <51127880+PARAIPAN9@users.noreply.github.com> Date: Thu, 9 Nov 2023 18:58:26 +0200 Subject: [PATCH] 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 <4530+thatswinnie@users.noreply.github.com> --- .../ContextualHintView.swift | 33 ++++++++++++++----- .../ContextualHintViewModel.swift | 5 ++- .../AccessibilityIdentifiers.swift | 4 +++ .../Browser/BrowserViewController.swift | 16 +++++---- ...BrowserViewController+URLBarDelegate.swift | 24 +++++++------- .../ContextualHintEligibilityUtility.swift | 1 + .../ContextualHintViewController.swift | 3 +- Shared/TimeConstants.swift | 2 ++ ...ontextualHintEligibilityUtilityTests.swift | 13 ++++++++ 9 files changed, 73 insertions(+), 28 deletions(-) diff --git a/BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintView.swift b/BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintView.swift index ac00c66158ec..97bb5247f319 100644 --- 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 index c01a7e2ef2fc..4d69cf317e91 100644 --- 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 index 540ca5748f8e..a4f71623e482 100644 --- 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 index b4da023c3d9a..3ed60a74a9f1 100644 --- 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 index 67002328bc86..a67c4d121780 100644 --- 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 index 74b54f36f2bc..f876872d3d11 100644 --- 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 index e3da93a967da..65f05d5af974 100644 --- 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 index 8ca59cb77fa3..87ef5349e870 100644 --- 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 index e84f20256ce8..19742f0e5491 100644 --- 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) + } }