Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -60,7 +60,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 @@ -187,7 +188,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 @@ -636,11 +640,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 @@ -651,9 +655,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 @@ -134,22 +134,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)
}
}
Loading