Skip to content

Commit

Permalink
Bugfix FXIOS-7646 [v120] The 3rd CFR is not displayed (#17172)
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
PARAIPAN9 and thatswinnie authored Nov 9, 2023
1 parent 1578d63 commit 7d43a40
Show file tree
Hide file tree
Showing 9 changed files with 73 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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() {
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)?
Expand All @@ -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
}
}
4 changes: 4 additions & 0 deletions Client/Application/AccessibilityIdentifiers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ public struct AccessibilityIdentifiers {
}
}

struct ContextualHints {
static let actionButton = "ContextualHints.ActionButton"
}

struct FirefoxHomepage {
static let collectionView = "FxCollectionView"

Expand Down
16 changes: 10 additions & 6 deletions Client/Frontend/Browser/BrowserViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
2 changes: 2 additions & 0 deletions Shared/TimeConstants.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

0 comments on commit 7d43a40

Please sign in to comment.