From f97204a709aa598c6dd5cbfaed32e6d7b1c800ea Mon Sep 17 00:00:00 2001 From: mattreaganmozilla <145381717+mattreaganmozilla@users.noreply.github.com> Date: Wed, 22 May 2024 12:47:31 -0700 Subject: [PATCH] Bugfix FXIOS-9174 [Multi-window] Fix theme bugs (#20373) * [9174] Fix several bugs with theme changes across multiple iPad windows. * [9174] Comment * [9174] Minor cleanup for readability * Fix missing import * [9174] Cleanup * [9174] Cleanup and comments * [9174] Comments --- .../Common/Theming/DefaultThemeManager.swift | 17 ++++++++-- .../Sources/Common/Theming/Themeable.swift | 17 +++++++--- firefox-ios/Client.xcodeproj/project.pbxproj | 8 ++--- .../Client/AccessoryViewProvider.swift | 2 +- .../Extensions/UIView+Multiwindow.swift | 15 --------- .../UIView+ThemeUUIDIdentifiable.swift | 33 +++++++++++++++++++ .../Views/BrowserViewController.swift | 2 ++ .../Theme/ThemedTableViewController.swift | 2 +- 8 files changed, 69 insertions(+), 27 deletions(-) delete mode 100644 firefox-ios/Client/Extensions/UIView+Multiwindow.swift create mode 100644 firefox-ios/Client/Extensions/UIView+ThemeUUIDIdentifiable.swift diff --git a/BrowserKit/Sources/Common/Theming/DefaultThemeManager.swift b/BrowserKit/Sources/Common/Theming/DefaultThemeManager.swift index 20f416e323b5..826ba21897bb 100644 --- a/BrowserKit/Sources/Common/Theming/DefaultThemeManager.swift +++ b/BrowserKit/Sources/Common/Theming/DefaultThemeManager.swift @@ -116,7 +116,15 @@ public final class DefaultThemeManager: ThemeManager, Notifiable { guard currentTheme(for: window).type != newTheme else { return } updateSavedTheme(to: newTheme) - updateCurrentTheme(to: fetchSavedThemeType(for: window), for: window) + + // Although we may have only explicitly changed the state on one specific window, + // we want to be sure we update all windows in case the Light/Dark theme changed. + allWindowUUIDs.forEach { + updateCurrentTheme(to: fetchSavedThemeType(for: $0), for: $0, notify: false) + } + // After updating all windows, notify (once). We send the UUID of the window for + // which the change originated though more than 1 window may ultimately update its UI. + notifyCurrentThemeDidChange(for: window) } public func reloadTheme(for window: WindowUUID) { @@ -215,7 +223,7 @@ public final class DefaultThemeManager: ThemeManager, Notifiable { userDefaults.set(newTheme.rawValue, forKey: ThemeKeys.themeName) } - private func updateCurrentTheme(to newTheme: ThemeType, for window: WindowUUID) { + private func updateCurrentTheme(to newTheme: ThemeType, for window: WindowUUID, notify: Bool = true) { windowThemeState[window] = newThemeForType(newTheme) // Overwrite the user interface style on the window attached to our scene @@ -223,7 +231,12 @@ public final class DefaultThemeManager: ThemeManager, Notifiable { let style = self.currentTheme(for: window).type.getInterfaceStyle() self.windows[window]?.overrideUserInterfaceStyle = style + if notify { + notifyCurrentThemeDidChange(for: window) + } + } + private func notifyCurrentThemeDidChange(for window: WindowUUID) { mainQueue.ensureMainThread { [weak self] in self?.notificationCenter.post(name: .ThemeDidChange, withUserInfo: window.userInfo) } diff --git a/BrowserKit/Sources/Common/Theming/Themeable.swift b/BrowserKit/Sources/Common/Theming/Themeable.swift index d8925b25e4eb..51483711c961 100644 --- a/BrowserKit/Sources/Common/Theming/Themeable.swift +++ b/BrowserKit/Sources/Common/Theming/Themeable.swift @@ -13,23 +13,32 @@ public protocol Themeable: ThemeUUIDIdentifiable, AnyObject { func applyTheme() } +/// Protocol for views to identify which iPad window (UUID) the view is associated with. +/// By default, all UIViews conform to this automatically. See: UIView+ThemeUUIDIdentifiable.swift. public protocol ThemeUUIDIdentifiable: AnyObject { var currentWindowUUID: WindowUUID? { get } } +/// Protocol that views or controllers may optionally adopt when they provide an explicit (typically, injected) +/// window UUID. This is used by our convenience extensions to allow UIViews to automatically detect their +/// associated iPad window UUID, even if the view is not immediately installed in a window or view hierarchy. +public protocol InjectedThemeUUIDIdentifiable: AnyObject { + var windowUUID: WindowUUID { get } +} + extension Themeable { public func listenForThemeChange(_ subview: UIView) { let mainQueue = OperationQueue.main themeObserver = notificationCenter.addObserver(name: .ThemeDidChange, queue: mainQueue) { [weak self] _ in self?.applyTheme() - guard let uuidIdentifiable = subview as? ThemeUUIDIdentifiable else { return } - self?.updateThemeApplicableSubviews(subview, for: uuidIdentifiable.currentWindowUUID) + self?.updateThemeApplicableSubviews(subview) } } - public func updateThemeApplicableSubviews(_ view: UIView, for window: WindowUUID?) { - guard let window else { return } + public func updateThemeApplicableSubviews(_ view: UIView) { + guard let window = (view as? ThemeUUIDIdentifiable)?.currentWindowUUID else { return } + assert(window != .unavailable, "Theme applicable view has `unavailable` window UUID. Unexpected.") let theme = themeManager.currentTheme(for: window) let themeViews = getAllSubviews(for: view, ofType: ThemeApplicable.self) themeViews.forEach { $0.applyTheme(theme: theme) } diff --git a/firefox-ios/Client.xcodeproj/project.pbxproj b/firefox-ios/Client.xcodeproj/project.pbxproj index 348394ab256a..8a6b9b3fc100 100644 --- a/firefox-ios/Client.xcodeproj/project.pbxproj +++ b/firefox-ios/Client.xcodeproj/project.pbxproj @@ -54,7 +54,7 @@ 1D2F68AD2ACB266300524B92 /* RemoteTabsPanelState.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D2F68AC2ACB266300524B92 /* RemoteTabsPanelState.swift */; }; 1D2F68AF2ACB272500524B92 /* RemoteTabsTableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D2F68AE2ACB272500524B92 /* RemoteTabsTableViewController.swift */; }; 1D2F68B12ACCA22000524B92 /* RemoteTabsEmptyView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D2F68B02ACCA22000524B92 /* RemoteTabsEmptyView.swift */; }; - 1D3822E92BAB99250046BC5E /* UIView+Multiwindow.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D3822E82BAB99250046BC5E /* UIView+Multiwindow.swift */; }; + 1D3822E92BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D3822E82BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift */; }; 1D3C90882ACE1AF400304C87 /* RemoteTabPanelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1D3C90872ACE1AF400304C87 /* RemoteTabPanelTests.swift */; }; 1D4D79462BF2F4E7007C6796 /* SimpleTab.swift in Sources */ = {isa = PBXBuildFile; fileRef = 43E69EAF254D064E00B591C2 /* SimpleTab.swift */; }; 1D4D79472BF2F4FD007C6796 /* Throttler.swift in Sources */ = {isa = PBXBuildFile; fileRef = 96D95015270238500079D39D /* Throttler.swift */; }; @@ -2263,7 +2263,7 @@ 1D2F68AC2ACB266300524B92 /* RemoteTabsPanelState.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabsPanelState.swift; sourceTree = ""; }; 1D2F68AE2ACB272500524B92 /* RemoteTabsTableViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabsTableViewController.swift; sourceTree = ""; }; 1D2F68B02ACCA22000524B92 /* RemoteTabsEmptyView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabsEmptyView.swift; sourceTree = ""; }; - 1D3822E82BAB99250046BC5E /* UIView+Multiwindow.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIView+Multiwindow.swift"; sourceTree = ""; }; + 1D3822E82BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "UIView+ThemeUUIDIdentifiable.swift"; sourceTree = ""; }; 1D3C90872ACE1AF400304C87 /* RemoteTabPanelTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RemoteTabPanelTests.swift; sourceTree = ""; }; 1D558A562BED7ECB001EF527 /* MockWindowManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockWindowManager.swift; sourceTree = ""; }; 1D558A592BEE7D07001EF527 /* WindowSimpleTabsCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WindowSimpleTabsCoordinator.swift; sourceTree = ""; }; @@ -8696,7 +8696,7 @@ E1442FCB294782D8003680B0 /* UIView+Extension.swift */, E1442FC4294782D7003680B0 /* UIView+Screenshot.swift */, E1442FCA294782D8003680B0 /* UIView+SnapKit.swift */, - 1D3822E82BAB99250046BC5E /* UIView+Multiwindow.swift */, + 1D3822E82BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift */, E1442FC7294782D7003680B0 /* UIViewController+Extension.swift */, C81A8F2426D3ED1900EBA539 /* UIWindow+Extension.swift */, E1442FC9294782D8003680B0 /* URL+Mail.swift */, @@ -14465,7 +14465,7 @@ 2137785F297F3B1B00D01309 /* DownloadsPanelViewModel.swift in Sources */, EBE26B57220C959D00D1D99A /* BrowserViewController+TabToolbarDelegate.swift in Sources */, 8A3EF7F02A2FCF3100796E3A /* HiddenSettings.swift in Sources */, - 1D3822E92BAB99250046BC5E /* UIView+Multiwindow.swift in Sources */, + 1D3822E92BAB99250046BC5E /* UIView+ThemeUUIDIdentifiable.swift in Sources */, B2FEA6912B4661BE0058E616 /* AddressAutofillToggle.swift in Sources */, B2FEA68F2B460D9E0058E616 /* AddressAutofillSettingsViewModel.swift in Sources */, AB03032B2AB47AF300DCD8EF /* FakespotOptInCardView.swift in Sources */, diff --git a/firefox-ios/Client/AccessoryViewProvider.swift b/firefox-ios/Client/AccessoryViewProvider.swift index 945a8402c5d6..7c827c70c3f9 100644 --- a/firefox-ios/Client/AccessoryViewProvider.swift +++ b/firefox-ios/Client/AccessoryViewProvider.swift @@ -10,7 +10,7 @@ enum AccessoryType { case standard, creditCard, address, login } -class AccessoryViewProvider: UIView, Themeable { +class AccessoryViewProvider: UIView, Themeable, InjectedThemeUUIDIdentifiable { // MARK: - Constants private struct UX { static let toolbarHeight: CGFloat = 50 diff --git a/firefox-ios/Client/Extensions/UIView+Multiwindow.swift b/firefox-ios/Client/Extensions/UIView+Multiwindow.swift deleted file mode 100644 index 557806353806..000000000000 --- a/firefox-ios/Client/Extensions/UIView+Multiwindow.swift +++ /dev/null @@ -1,15 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at http://mozilla.org/MPL/2.0/ - -import UIKit -import Common -import Shared - -extension UIView: ThemeUUIDIdentifiable { - /// If the view is installed in a view hierarchy that is part of a window, - /// returns the window UUID. If not, returns nil. - public var currentWindowUUID: WindowUUID? { - return (self.window as? BrowserWindow)?.uuid - } -} diff --git a/firefox-ios/Client/Extensions/UIView+ThemeUUIDIdentifiable.swift b/firefox-ios/Client/Extensions/UIView+ThemeUUIDIdentifiable.swift new file mode 100644 index 000000000000..182d816646c7 --- /dev/null +++ b/firefox-ios/Client/Extensions/UIView+ThemeUUIDIdentifiable.swift @@ -0,0 +1,33 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/ + +import UIKit +import Common +import Shared + +extension UIView: ThemeUUIDIdentifiable { + /// Convenience that allows most UIViews to automatically reference their parent window UUID. + /// If the view is installed in a view hierarchy that is part of a window, or is a type + /// that has its UUID injected, this returns the window UUID. If not, returns nil. + public var currentWindowUUID: WindowUUID? { + // If the view has opted in to InjectedThemeUUIDIdentifiable, we prefer that UUID + // since it is injected and should always be correct even if the view isn't installed + // in a window or view hierarchy. + if let injectedUUID = (self as? InjectedThemeUUIDIdentifiable)?.windowUUID { + return injectedUUID + } + + // Edge-case: if the view is a UITableView, allow delegates to opt-in to InjectedThemeUUIDIdentifiable. + if let tableView = self as? UITableView, + let uuidProvider = tableView.delegate as? InjectedThemeUUIDIdentifiable { + return uuidProvider.windowUUID + } + + // Finally, for most views, we can simply return the associated UUID of the window the + // view is installed in. This covers 99% of use cases, however care needs to be used with + // UI that is not always inserted into a view hierarchy. In those situations, the view + // can opt-in to `InjectedThemeUUIDIdentifiable`. + return (self.window as? BrowserWindow)?.uuid + } +} diff --git a/firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift b/firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift index ede634ff0f5a..43759c91bab1 100644 --- a/firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift +++ b/firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift @@ -2364,6 +2364,8 @@ class BrowserViewController: UIViewController, urlBar.applyUIMode(isPrivate: isPrivate, theme: currentTheme) } + toolbar.applyTheme(theme: currentTheme) + guard let contentScript = tabManager.selectedTab?.getContentScript(name: ReaderMode.name()) else { return } applyThemeForPreferences(profile.prefs, contentScript: contentScript) } diff --git a/firefox-ios/Client/Frontend/Theme/ThemedTableViewController.swift b/firefox-ios/Client/Frontend/Theme/ThemedTableViewController.swift index 407e796e2328..6e3f947fa733 100644 --- a/firefox-ios/Client/Frontend/Theme/ThemedTableViewController.swift +++ b/firefox-ios/Client/Frontend/Theme/ThemedTableViewController.swift @@ -6,7 +6,7 @@ import UIKit import Shared import Common -class ThemedTableViewController: UITableViewController, Themeable { +class ThemedTableViewController: UITableViewController, Themeable, InjectedThemeUUIDIdentifiable { var themeManager: ThemeManager @objc var notificationCenter: NotificationProtocol var themeObserver: NSObjectProtocol?