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

[MOB-3152] Refactor Ecosia theming #834

Merged

Conversation

lucaschifino
Copy link
Collaborator

@lucaschifino lucaschifino commented Jan 30, 2025

MOB-3152

Context

On the latest upgrade, Firefox got rid of the LegacyTheme, which we were using for Ecosia colors.

Approach

  1. Introduce new Ecosia colors, following the same approach as Web and Figma with two layers - Primitives and Semantic Colors.
  2. Plug the new Ecosia Semantic Colors to the existing Firefox's ThemeColourPalette, following the same protocol-oriented approach.
  3. Replace all colors from EcosiaTheme using the nem EcosiaThemeColourPalette
    1. This meant introducing the new theme manager (which requires window now) to a number of Ecosia files
    2. This was done without changing anything in the UI, meaning Snowflakes (colors not present on our design system) were kept and will be handled on a followup ticket
  4. Remove LegacyTheme and LegacyThemeManger which had been recovered during the upgrade
  5. Review and adjust changes in Firefox colors
  6. Clean up resolved upgrade todos related to this story

Other

Crashes cause by missing window

MOB-3159

Noticed that this other bug ticket is related to theme changes and caused by a missing window objected when applying the theme. I was apply to fix that together with this PR by using ThemeApplicable on views instead of UIView.currentWindowUUID, since that can frequently still be nil if a view is not yet added to the window.

Reacting to appearance changes with the app on foreground

#834 (comment)

Before merging

Checklist

  • I performed some relevant testing on a real device and/or simulator
  • I added the // Ecosia: helper comments where needed
  • I included documentation updates to the coding standards or Confluence doc, when needed

Copy link

github-actions bot commented Jan 30, 2025

PR Reviewer Guide 🔍

(Review updated until commit f00805b)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Color Mismatches

There are several comments indicating mismatches or no matches for certain colors in the EcosiaLightSemanticColors structure. These should be reviewed to ensure they align with the intended design specifications.

private struct EcosiaLightSemanticColors: EcosiaSemanticColors {
    var backgroundPrimary: UIColor = EcosiaColor.White
    var backgroundSecondary: UIColor = EcosiaColor.Gray10
    var backgroundTertiary: UIColor = EcosiaColor.Gray20
    var backgroundQuaternary: UIColor = EcosiaColor.DarkGreen50
    var borderDecorative: UIColor = EcosiaColor.Gray30
    var brandPrimary: UIColor = EcosiaColor.Green50
    var buttonBackgroundPrimary: UIColor = EcosiaColor.Green50
    var buttonBackgroundPrimaryActive: UIColor = EcosiaColor.Green70 // ⚠️ Mismatch
    var buttonBackgroundSecondary: UIColor = EcosiaColor.White
    var buttonBackgroundSecondaryHover: UIColor = EcosiaColor.Gray10 // ⚠️ Mismatch
    var buttonContentSecondary: UIColor = EcosiaColor.Gray70
    var buttonBackgroundTransparentActive: UIColor = EcosiaColor.Green70.withAlphaComponent(0.24)
    var backgroundHighlighted: UIColor = EcosiaColor.Green10
    var iconPrimary: UIColor = EcosiaColor.Black // ⚠️ Mismatch
    var iconSecondary: UIColor = EcosiaColor.Green60 // ⚠️ Mismatch
    var iconDecorative: UIColor = EcosiaColor.Gray50
    var stateError: UIColor = EcosiaColor.Red40
    var stateInformation: UIColor = EcosiaColor.Blue50 // ⚠️ No match
    var stateDisabled: UIColor = EcosiaColor.Gray30
    var textPrimary: UIColor = EcosiaColor.Black // ⚠️ Mismatch
    var textInversePrimary: UIColor = EcosiaColor.White
    var textSecondary: UIColor = EcosiaColor.Gray50
    var textTertiary: UIColor = EcosiaColor.White

    // MARK: Unmapped Snowflakes
    var barBackground: UIColor = EcosiaColor.White
    var barSeparator: UIColor = .Photon.Grey20
    var ntpCellBackground: UIColor = EcosiaColor.White
    var ntpBackground: UIColor = EcosiaColor.Gray20
    var ntpIntroBackground: UIColor = EcosiaColor.White
    var impactMultiplyCardBackground: UIColor = EcosiaColor.White
    var newsPlaceholder: UIColor = EcosiaColor.Gray10
    var modalBackground: UIColor = EcosiaColor.Gray20
    var modalHeader: UIColor = EcosiaColor.DarkGreen50
    var secondarySelectedBackground: UIColor = EcosiaColor.Gray10
    var buttonBackgroundNTPCustomization: UIColor = EcosiaColor.Gray10
    var privateButtonBackground: UIColor = .Photon.Grey70
    var tabSelectedPrivateBackground: UIColor = EcosiaColor.Gray80
    var toastImageTint: UIColor = EcosiaColor.LightGreen30
    var newSeedCollectedCircle: UIColor = EcosiaColor.Peach30
    var tabTrayScreenshotBackground: UIColor = EcosiaColor.White
    var tableViewRowText: UIColor = .Photon.Grey90
}
Theme Application Logic

The applyTheme method in MultiplyImpact introduces a significant amount of theme-specific logic. Ensure this logic is consistent and does not introduce unintended UI inconsistencies.

}

func applyTheme() {
    let theme = themeManager.getCurrentTheme(for: windowUUID)

    view.backgroundColor = theme.colors.ecosia.modalBackground
    topBackground?.backgroundColor = theme.colors.ecosia.brandPrimary.withAlphaComponent(0.2)
    inviteButton.backgroundColor = theme.colors.ecosia.brandPrimary
    inviteButton.setTitleColor(theme.colors.ecosia.textInversePrimary, for: .normal)
    inviteButton.setTitleColor(theme.colors.ecosia.textInversePrimary, for: .highlighted)
    inviteButton.setTitleColor(theme.colors.ecosia.textInversePrimary, for: .selected)
    learnMoreButton?.setTitleColor(theme.colors.ecosia.brandPrimary, for: .normal)
    waves?.tintColor = theme.colors.ecosia.modalBackground
    topBackground?.backgroundColor = theme.colors.ecosia.modalBackground
    forestOverlay?.backgroundColor = theme.colors.ecosia.modalBackground
    subtitle?.textColor = .Dark.Text.primary
    copyControl?.backgroundColor = theme.colors.ecosia.backgroundSecondary
    copyControl?.layer.borderColor = theme.colors.ecosia.borderDecorative.cgColor
    moreSharingMethods?.textColor = theme.colors.ecosia.textSecondary
    copyText?.textColor = theme.colors.ecosia.brandPrimary

    [yourInvites, sharingYourLink, flowTitle, copyLink].forEach {
        $0?.textColor = theme.colors.ecosia.textPrimary
    }

    [sharing, flowBackground].forEach {
        $0?.backgroundColor = theme.colors.ecosia.impactMultiplyCardBackground
    }

    [firstStep, secondStep, thirdStep, fourthStep].forEach {
        $0?.applyTheme(theme: theme)
    }

    [copyDividerLeft, copyDividerRight].forEach {
        $0?.backgroundColor = theme.colors.ecosia.borderDecorative
    }

    referralImpactRowView.customBackgroundColor = theme.colors.ecosia.impactMultiplyCardBackground
    referralImpactRowView.applyTheme(theme: theme)

    updateBarAppearance(theme: theme)
}

private func updateBarAppearance(theme: Theme) {
    let appearance = UINavigationBarAppearance()
    appearance.configureWithOpaqueBackground()
    appearance.largeTitleTextAttributes = [.foregroundColor: UIColor.Dark.Text.primary]
    appearance.titleTextAttributes = [.foregroundColor: UIColor.Dark.Text.primary]
    appearance.backgroundColor = theme.colors.ecosia.modalBackground
    appearance.shadowColor = nil
    navigationItem.standardAppearance = appearance
    navigationItem.scrollEdgeAppearance = appearance
    navigationController?.navigationBar.backgroundColor = theme.colors.ecosia.modalBackground
    navigationController?.navigationBar.tintColor = theme.type == .light ? UIColor.Dark.Text.primary : theme.colors.ecosia.brandPrimary
Theme Application in Collection Views

The applyTheme method in NewsController applies themes to collection view cells and headers. Ensure this is robust and handles edge cases like recycled cells or headers.

}

func applyTheme() {
    let theme = themeManager.getCurrentTheme(for: windowUUID)
    collection.visibleSupplementaryViews(ofKind: UICollectionView.elementKindSectionHeader).forEach({
        ($0 as? Themeable)?.applyTheme()
        ($0 as? ThemeApplicable)?.applyTheme(theme: theme)
    })
    collection.visibleCells.forEach({
        ($0 as? Themeable)?.applyTheme()
        ($0 as? ThemeApplicable)?.applyTheme(theme: theme)
    })
    collection.backgroundColor = theme.colors.ecosia.modalBackground
    updateBarAppearance()

    if traitCollection.userInterfaceIdiom == .pad {

Copy link

github-actions bot commented Jan 30, 2025

PR Code Suggestions ✨

Latest suggestions up to f00805b
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Validate theme resolution to avoid crashes

Validate that themeManager.getCurrentTheme(for:) returns a non-nil theme to prevent
potential crashes when accessing theme properties.

firefox-ios/Client/Ecosia/Bookmarks/BookmarksExchange.swift [169-172]

 private func themeFromView(view: UIView) -> Theme {
     let themeManager: ThemeManager = AppContainer.shared.resolve()
-    return themeManager.getCurrentTheme(for: view.currentWindowUUID)
+    guard let theme = themeManager.getCurrentTheme(for: view.currentWindowUUID) else {
+        fatalError("Theme could not be resolved for the given window UUID.")
+    }
+    return theme
 }
Suggestion importance[1-10]: 9

Why: The suggestion addresses a critical issue by ensuring that themeManager.getCurrentTheme(for:) returns a non-nil value, preventing potential crashes when accessing theme properties. The improved_code is accurate and directly improves the robustness of the function.

9
Add fallback for theme resolution failure

Add a fallback mechanism in the update method to handle cases where
themeManager.getCurrentTheme(for:) fails to resolve a theme.

firefox-ios/Client/Ecosia/UI/EcosiaPrimaryButton.swift [36-40]

 private func update() {
     let themeManager: ThemeManager = AppContainer.shared.resolve()
-    let theme = themeManager.getCurrentTheme(for: windowUUID)
+    guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+        backgroundColor = .gray // Fallback color
+        return
+    }
     backgroundColor = (isSelected || isHighlighted) ? theme.colors.ecosia.buttonBackgroundPrimaryActive : theme.colors.ecosia.buttonBackgroundPrimary
 }
Suggestion importance[1-10]: 9

Why: The suggestion adds a fallback mechanism in the update method to handle cases where the theme resolution fails, ensuring the application does not crash and providing a default behavior. The improved_code is well-implemented and enhances the reliability of the code.

9
Handle nil theme in applyTheme

Ensure that the applyTheme method properly handles cases where
themeManager.getCurrentTheme(for:) returns nil to prevent crashes.

firefox-ios/Client/Ecosia/UI/FilterController.swift [81-91]

 func applyTheme() {
-    let theme = themeManager.getCurrentTheme(for: windowUUID)
+    guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+        return // Log or handle theme resolution failure
+    }
     table.visibleCells.forEach {
         ($0 as? ThemeApplicable)?.applyTheme(theme: theme)
     }
     view.backgroundColor = theme.colors.ecosia.ntpBackground
     table.tintColor = theme.colors.ecosia.brandPrimary
     table.separatorColor = theme.colors.ecosia.borderDecorative
     table.backgroundColor = theme.colors.ecosia.ntpBackground
 }
Suggestion importance[1-10]: 9

Why: The suggestion ensures that the applyTheme method handles cases where themeManager.getCurrentTheme(for:) returns nil, preventing potential crashes. The improved_code is accurate and effectively improves the function's robustness.

9
Validate currentWindowUUID initialization

Ensure that currentWindowUUID is properly initialized and non-nil before being
passed to NewSeedCollectedCircleView to avoid runtime errors.

firefox-ios/Client/Ecosia/UI/NTP/ClimateImpactCounter/NTPSeedCounterCell.swift [128]

-let newSeedView = NewSeedCollectedCircleView(windowUUID: currentWindowUUID, seedsCollected: 1)
+guard let windowUUID = currentWindowUUID else {
+    fatalError("currentWindowUUID is nil")
+}
+let newSeedView = NewSeedCollectedCircleView(windowUUID: windowUUID, seedsCollected: 1)
Suggestion importance[1-10]: 9

Why: Ensuring currentWindowUUID is non-nil before using it in NewSeedCollectedCircleView is a critical improvement. This validation prevents potential runtime errors and ensures the application functions correctly.

9
Add safety checks for theme retrieval

Ensure that themeManager.getCurrentTheme(for: windowUUID) does not return nil or an
unexpected value, as this could cause runtime crashes or incorrect theming behavior.

firefox-ios/Client/Ecosia/UI/LoadingScreen.swift [37-38]

-let theme = themeManager.getCurrentTheme(for: windowUUID)
+guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+    fatalError("Failed to retrieve theme for windowUUID")
+}
 view.backgroundColor = theme.colors.ecosia.backgroundPrimary
Suggestion importance[1-10]: 8

Why: Adding a guard statement to ensure themeManager.getCurrentTheme(for: windowUUID) does not return nil is a critical improvement. It prevents potential runtime crashes and ensures the application behaves predictably when the theme cannot be resolved.

8
Validate theme resolution for safety

Verify that themeManager.getCurrentTheme(for: windowUUID) is always returning a
valid Theme object to prevent potential crashes or undefined behavior.

firefox-ios/Client/Ecosia/UI/MultiplyImpact/MultiplyImpact.swift [425-427]

-let theme = themeManager.getCurrentTheme(for: windowUUID)
+guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+    fatalError("Theme could not be resolved for windowUUID")
+}
 view.backgroundColor = theme.colors.ecosia.modalBackground
Suggestion importance[1-10]: 8

Why: The suggestion to validate the theme resolution with a guard statement is highly relevant. It mitigates the risk of runtime crashes or undefined behavior if the theme cannot be resolved, which is crucial for application stability.

8
Add safety checks for nil items

Validate that the configure method handles cases where actions.items.first is nil to
avoid potential crashes or unexpected behavior.

firefox-ios/Client/Ecosia/UI/PageAction/PageActionMenuCell.swift [177-184]

 func configure(with viewModel: PhotonActionSheetViewModel,
                at indexPath: IndexPath,
                theme: Theme) {
     let actions = viewModel.actions[indexPath.section][indexPath.row]
-    guard let item = actions.items.first else { return }
+    guard let item = actions.items.first else {
+        assertionFailure("Expected at least one action item.")
+        return
+    }
Suggestion importance[1-10]: 8

Why: Adding an assertionFailure improves debugging and ensures that unexpected nil values in actions.items.first are flagged during development, reducing the risk of silent failures or crashes.

8
General
Add fallback for text color

Add a fallback mechanism in setLinkTitle to handle cases where
theme.colors.ecosia.textInversePrimary is unavailable or invalid.

firefox-ios/Client/Ecosia/UI/NTP/Tooltip/NTPTooltip.swift [152]

-.foregroundColor: theme.colors.ecosia.textInversePrimary
+.foregroundColor: theme.colors.ecosia.textInversePrimary ?? UIColor.black
Suggestion importance[1-10]: 7

Why: Adding a fallback mechanism for theme.colors.ecosia.textInversePrimary ensures that the application does not crash or display incorrect UI elements if the theme color is unavailable. This is a valuable enhancement for robustness.

7
Ensure UI updates reflect selection states

Ensure that the hover method properly updates the background.backgroundColor when
isSelected or isHighlighted changes, as there might be cases where these states are
not correctly reflected in the UI.

firefox-ios/Client/Ecosia/UI/NTP/News/NTPNewsCell.swift [210-212]

 private func hover() {
-    background.backgroundColor = isSelected || isHighlighted ? selectedBackgroundColor : defaultBackgroundColor
+    background.backgroundColor = (isSelected || isHighlighted) ? selectedBackgroundColor : defaultBackgroundColor
+    setNeedsLayout() // Ensure UI updates are reflected
 }
Suggestion importance[1-10]: 7

Why: Adding setNeedsLayout() ensures that UI updates are reflected properly when isSelected or isHighlighted changes. This is a minor but useful improvement to ensure the UI behaves as expected.

7
Ensure consistent theme application

Ensure that the applyThemeToMonthView method is called consistently to avoid missing
theme updates for the monthView and monthViewLabel.

firefox-ios/Client/Ecosia/UI/Onboarding/WelcomeTourTransparent.swift [55-59]

 func applyTheme(theme: Theme) {
     stack.arrangedSubviews.forEach { view in
         (view as? ThemeApplicable)?.applyTheme(theme: theme)
     }
-    applyThemeToMonthView(theme: theme)
+    if let monthView = monthView {
+        applyThemeToMonthView(theme: theme)
+    }
 }
Suggestion importance[1-10]: 6

Why: Adding a check for monthView before calling applyThemeToMonthView ensures that the method does not fail when monthView is nil. This is a minor improvement for robustness.

6

Previous suggestions

Suggestions up to commit 8ceb257
CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure UI updates occur on main thread

Ensure applyTheme(theme:) is called on the main thread since it modifies UI
elements.

firefox-ios/Client/Ecosia/UI/NTP/ClimateImpactCounter/SeedCounterView.swift [63-64]

-self.theme.backgroundColor = Color(theme.colors.ecosia.backgroundPrimary)
-self.theme.progressColor = Color(theme.colors.ecosia.buttonBackgroundPrimaryActive)
+DispatchQueue.main.async {
+    self.theme.backgroundColor = Color(theme.colors.ecosia.backgroundPrimary)
+    self.theme.progressColor = Color(theme.colors.ecosia.buttonBackgroundPrimaryActive)
+}
Suggestion importance[1-10]: 10

Why: Ensuring that UI updates are performed on the main thread is critical for maintaining application stability and avoiding undefined behavior. This suggestion addresses a fundamental requirement for UI modifications in iOS development.

10
Ensure theme resolution robustness

Verify that themeFromView(view:) correctly resolves the theme for the provided view
to avoid potential nil or incorrect theme assignments.

firefox-ios/Client/Ecosia/Bookmarks/BookmarksExchange.swift [169-172]

 private func themeFromView(view: UIView) -> Theme {
     let themeManager: ThemeManager = AppContainer.shared.resolve()
-    return themeManager.getCurrentTheme(for: view.currentWindowUUID)
+    guard let theme = themeManager.getCurrentTheme(for: view.currentWindowUUID) else {
+        fatalError("Failed to resolve theme for the provided view.")
+    }
+    return theme
 }
Suggestion importance[1-10]: 9

Why: The suggestion improves robustness by adding a guard statement to handle cases where the theme resolution fails, preventing potential crashes. This is a significant enhancement to the code's reliability.

9
Add fallback for theme resolution

Add error handling or fallback logic in update() to ensure
themeManager.getCurrentTheme does not return nil, which could lead to crashes when
accessing theme.colors.

firefox-ios/Client/Ecosia/UI/EcosiaPrimaryButton.swift [37-40]

 private func update() {
     let themeManager: ThemeManager = AppContainer.shared.resolve()
-    let theme = themeManager.getCurrentTheme(for: windowUUID)
+    guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+        backgroundColor = .gray // Fallback color
+        return
+    }
     backgroundColor = (isSelected || isHighlighted) ? theme.colors.ecosia.buttonBackgroundPrimaryActive : theme.colors.ecosia.buttonBackgroundPrimary
 }
Suggestion importance[1-10]: 9

Why: The suggestion adds a fallback mechanism for cases where the theme resolution fails, ensuring the application does not crash and providing a default behavior. This is a valuable improvement for stability.

9
Handle unresolved theme gracefully

Ensure that the themeManager.getCurrentTheme call in viewDidLoad handles cases where
the theme might not be resolved, to prevent potential crashes.

firefox-ios/Client/Ecosia/UI/LoadingScreen.swift [37-38]

 override func viewDidLoad() {
     super.viewDidLoad()
-    let theme = themeManager.getCurrentTheme(for: windowUUID)
+    guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+        view.backgroundColor = .white // Default fallback color
+        return
+    }
     view.backgroundColor = theme.colors.ecosia.backgroundPrimary
 }
Suggestion importance[1-10]: 9

Why: The suggestion introduces a fallback color in case the theme resolution fails, which is a practical and effective way to prevent crashes and maintain functionality. This enhances the code's robustness.

9
Add error handling for theme retrieval

Ensure that themeManager.getCurrentTheme(for: windowUUID) handles cases where the
windowUUID is invalid or not found to avoid potential runtime errors.

firefox-ios/Client/Ecosia/UI/MultiplyImpact/MultiplyImpact.swift [425]

-let theme = themeManager.getCurrentTheme(for: windowUUID)
+guard let theme = themeManager.getCurrentTheme(for: windowUUID) else {
+    // Handle error or fallback theme
+    return
+}
Suggestion importance[1-10]: 9

Why: Adding error handling for theme retrieval is crucial to prevent runtime crashes when the windowUUID is invalid or not found. This suggestion directly addresses a potential issue and provides a robust fallback mechanism, significantly improving the code's reliability.

9
Add nil-check for theme usage

Validate that currentTheme is not nil before applying it to UI elements to avoid
potential crashes.

firefox-ios/Client/Ecosia/UI/PageAction/PageActionMenu.swift [208]

-header.applyTheme(theme: currentTheme)
+if let theme = currentTheme {
+    header.applyTheme(theme: theme)
+} else {
+    // Handle missing theme case
+}
Suggestion importance[1-10]: 9

Why: Adding a nil-check for currentTheme before applying it to UI elements prevents potential crashes due to unwrapping a nil value. This suggestion enhances the code's robustness and ensures safer execution.

9
Handle potential nil theme scenarios

Add a fallback mechanism in applyTheme() to handle cases where
themeManager.getCurrentTheme(for:) might return nil.

firefox-ios/Client/Ecosia/UI/WhatsNew/WhatsNewCell.swift [87-93]

-let theme = themeManager.getCurrentTheme(for: currentWindowUUID)
+guard let theme = themeManager.getCurrentTheme(for: currentWindowUUID) else { return }
 if #available(iOS 14, *) {
     guard var updatedConfiguration = contentConfigurationToUpdate as? UIListContentConfiguration else { return }
     updatedConfiguration.image = item.image?.tinted(withColor: theme.colors.ecosia.iconSecondary)
     contentConfiguration = updatedConfiguration
 } else {
     imageView?.image = item.image?.tinted(withColor: theme.colors.ecosia.iconSecondary)
 }
Suggestion importance[1-10]: 9

Why: Adding a fallback mechanism for a nil theme prevents potential crashes and ensures the method operates safely, significantly enhancing the code's reliability.

9
Prevent retain cycles in closures

Verify that defaultBackgroundColor closure does not cause retain cycles by capturing
self weakly.

firefox-ios/Client/Ecosia/UI/NTP/News/NTPNewsCell.swift [93-95]

-lazy var defaultBackgroundColor: (() -> UIColor) = {
+lazy var defaultBackgroundColor: (() -> UIColor) = { [weak self] in
+    guard let self = self else { return .clear }
     let theme = self.themeManager.getCurrentTheme(for: self.currentWindowUUID)
     return theme.colors.ecosia.ntpCellBackground
 }
Suggestion importance[1-10]: 8

Why: Capturing self weakly in the closure prevents potential retain cycles, which is a common issue in Swift. This suggestion is highly relevant and improves memory management, ensuring the closure does not unintentionally retain the parent object.

8
Safeguard against nil badge references

Ensure that the applyTheme(theme:) method safely handles cases where badge might be
nil to avoid potential runtime crashes.

firefox-ios/Client/Ecosia/UI/PageAction/PageActionMenuCell.swift [71-73]

 func applyTheme(theme: Theme) {
-    badge?.backgroundColor = theme.colors.ecosia.brandPrimary
+    guard let badge = badge else { return }
+    badge.backgroundColor = theme.colors.ecosia.brandPrimary
 }
Suggestion importance[1-10]: 8

Why: The suggestion ensures that the applyTheme method does not attempt to access a nil badge, preventing potential runtime crashes. This is a significant improvement in terms of code safety and robustness.

8
General
Validate ecosia color availability

Ensure that applyUIMode(isPrivate:theme:) properly handles cases where
theme.colors.ecosia might not contain expected color values.

firefox-ios/Client/Frontend/Browser/TabTrayButtonExtensions.swift [29-38]

-tintColor = isPrivate ? theme.colors.ecosia.backgroundPrimary : theme.colors.ecosia.textPrimary
+guard let ecosiaColors = theme.colors.ecosia else { return }
+tintColor = isPrivate ? ecosiaColors.backgroundPrimary : ecosiaColors.textPrimary
 imageView?.tintColor = tintColor
 backgroundLayer.backgroundColor = isPrivate
-    ? theme.colors.ecosia.privateButtonBackground.cgColor
+    ? ecosiaColors.privateButtonBackground.cgColor
     : UIColor.clear.cgColor
Suggestion importance[1-10]: 8

Why: The suggestion ensures that the ecosia color palette is available before accessing its properties, preventing potential runtime errors and improving code robustness.

8
Fix mismatched color assignments

Address the mismatched color assignments in EcosiaDarkSemanticColors to ensure
consistency with the design specifications.

firefox-ios/Client/Ecosia/UI/Theme/EcosiaDarkTheme.swift [45-53]

-var buttonBackgroundPrimaryActive: UIColor = EcosiaColor.Green50 // ⚠️ Mismatch
-var buttonBackgroundSecondary: UIColor = EcosiaColor.Gray70 // ⚠️ Mismatch
-var iconDecorative: UIColor = EcosiaColor.Gray40 // ⚠️ Mismatch
+var buttonBackgroundPrimaryActive: UIColor = EcosiaColor.Green60
+var buttonBackgroundSecondary: UIColor = EcosiaColor.Gray60
+var iconDecorative: UIColor = EcosiaColor.Gray50
Suggestion importance[1-10]: 7

Why: Correcting mismatched color assignments aligns the code with design specifications, improving consistency and visual accuracy. However, the impact is limited to aesthetic consistency.

7

@lucaschifino
Copy link
Collaborator Author

  • Reacting to appearance changes with the app open is actually not properly working ⚠️ Looking into that 👀

Turns out this was related to an old issue we had fixed in previous versions for theme changes with the app on foreground -> #712

Given Firefox still works like that and it's still a very specific case, leaving it to be properly QAed and handled separately. I also had some issues when trying to replicate the fix on the upgraded version, namely traitCollectionDidChange not getting properly triggered when toggling the appearance via the Simulator.

@lucaschifino lucaschifino marked this pull request as ready for review January 31, 2025 14:42
Copy link

Persistent review updated to latest commit f00805b

@lucaschifino lucaschifino requested a review from a team January 31, 2025 14:44
Copy link
Member

@d4r1091 d4r1091 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such great work! 🎨
And indeed a great step forward towards a better design system integration!
On top of that, no more "legacy theme manager". Such a relief!

Lets bring it in so that I can align the opened Ecosia Framework and solve conflicts!
It's interesting your comment regarding the issue being only when the app is in "foreground".

Annoying.
For full visibility, I wanted to mention that what's fixed as part of #712 included the state when the automatic dark/light mode was triggered while the app was in the background state.
Upon reopening, you'd still see the appearance as it was before the switch.
The Foreground state mentioned in the PR included the scenario where we toggled the appearance forcely via the developer tools (CMD+SHIFT+A).

Hope that clarifies it too 👍

@lucaschifino
Copy link
Collaborator Author

For full visibility, I wanted to mention that what's fixed as part of #712 included the state when the automatic dark/light mode was triggered while the app was in the background state.
Upon reopening, you'd still see the appearance as it was before the switch.
The Foreground state mentioned in the PR included the scenario where we toggled the appearance forcely via the developer tools (CMD+SHIFT+A).

@d4r1091 Interesting, thanks for clarifying! Right now this is less of a problem then, since as soon as the app is reopened from background it updates correctly. It's only a problem if the theme changes when on foreground (e.g. becoming nightime with auto theme) which is a very specific use case (maybe not worth spending more time to investigate indeed).

@lucaschifino lucaschifino merged commit 1b6d6e1 into mob-3113-firefox-upgrade-133 Feb 4, 2025
1 of 2 checks passed
@lucaschifino lucaschifino deleted the ls-mob-3152-refactore-ecosia-theming branch February 4, 2025 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants