Fix #1493: Themes / Dark Mode Support #1324

jhreis · 2019-08-02T13:47:41Z

Summary of Changes

This pull request fixes issue #1493

This ticket impacts a good bit, so tried to include a good bit of code comments, as well as PR comments too.

Make sure to squash this!! Lots of commits 😄

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

A few smaller views had elements styled.

…nt colors.

jhreis · 2019-09-06T04:44:39Z

Client/Frontend/Browser/TabManager.swift

@@ -243,13 +243,6 @@ class TabManager: NSObject {
        }
        if let tab = selectedTab {
            TabEvent.post(.didGainFocus, for: tab)
-
-            switch tab.type {


Moved this to AppearanceExtensions.swift

jhreis · 2019-09-06T04:46:57Z

Client/Frontend/Browser/Toolbars/BottomToolbar/BottomToolbarView.swift

-    let shareButton = ToolbarButton()
-    let addTabButton = ToolbarButton()
-    let menuButton = ToolbarButton()
+    let tabsButton = TabsButton(top: false)


This is a bit annoying, but needed some way to differentiate when TabsButton or ToolbarButton required certain tint elements.

This is largely caused by these buttons handling their own internal theming, which is not great.

Filed a follow up ticket for this: #1494

At first I thought it was initial work to support fully custom toolbars, and its placement

jhreis · 2019-09-06T04:47:51Z

Client/Frontend/Browser/Toolbars/BottomToolbar/Menu/MenuViewController.swift

-            case .add: return #imageLiteral(resourceName: "menu-add-bookmark")
-            case .share: return #imageLiteral(resourceName: "nav-share")
-            case .downloads: return #imageLiteral(resourceName: "menu-downloads")
+            case .bookmarks: return #imageLiteral(resourceName: "menu_bookmarks").template


.template is required to properly style these buttons via tintColor or similar.

jhreis · 2019-09-06T04:51:03Z

Client/Frontend/Settings/SettingsViewController.swift

+    }
+
+    private var theme: Theme {
+        Theme.of(tabManager.selectedTab)


A bit annoying, but necessary since theme can dynamically change while settings is up, either by theme change or by toggling Private Browsing Only.

jhreis · 2019-09-06T04:51:50Z

Client/Frontend/Settings/SettingsViewController.swift

@@ -167,6 +170,24 @@ class SettingsViewController: TableViewController {
            general.rows.append(row)
        }

+        let themeSubtitle = Theme.DefaultTheme(rawValue: Preferences.General.themeNormalMode.value)?.displayString


Adding the theme option to settings. Currently only manipulates the normal theme option.

jhreis · 2019-09-06T04:52:16Z

Client/Frontend/Settings/SettingsViewController.swift

+                onValueChange: {
+                    Preferences.Privacy.privateBrowsingOnly.value = $0
+
+                    // Need to flush the table, hacky, but works consistenly and well


When switching into PrivateBrowsingOnly, the settings UI needs to be updated, this is where that happens.

jhreis · 2019-09-06T04:54:35Z

Client/Frontend/Widgets/Theme.swift

@@ -1,57 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


This was all moved to child folder Theme: https://github.com/brave/brave-ios/pull/1324/files#diff-341a31b4ae4d39e8a2ab9390b46afd7e

jhreis · 2019-09-06T04:55:12Z

Client/Frontend/Widgets/Themes/Theme.swift

+    }
+}
+
+fileprivate enum ThemeCodingKeys: String, CodingKey {


For JSON mappings.

jhreis · 2019-09-06T04:56:25Z

Client/Frontend/Widgets/Themes/Theme.swift

@@ -0,0 +1,336 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


Big class. Takes and decodes the JSON files, and utilizes these for theming. Currently a lot of the heavy lifting is done via: DefaultTheme, this was mostly done for time, but there are some complexities involved with fully dynamic themes (e.g. localizations), that still need to be figured out.

So should be a thorough enough solution for any "hardcoded" / hard-included themes we ship in the binary.

jhreis · 2019-09-06T05:26:50Z

Client/Frontend/Browser/BrowserViewController.swift

-                let theme = Theme.of(tab)
-                applyTheme(theme)
+            let newTheme = Theme.of(tab)
+            if previous == nil || newTheme != Theme.of(previous) {


Switched from using tab.type to using theme change detection. Also, will "force" update the theme if the previous tab didn't exist.

iccub

Code looks good overall, few small changes and questions.
I also have two big-picture concerns:

My first feeling was that there is not enough UX constants in the json file to support all views.
This leads to:

Let’s say I create a new view, how do I know what value to use for my views?
Look at this picture for example

One view uses some magic value header, the other one home while both views have tint color taken from header.
These values don't make much sense, we would need to use trail and error to figure out correct values, or our design team has to tell us to use header, home instead of hard color values.

If we start supporting custom themes this can also lead to unexpected colors. If the header color looks good in view X, it might not look good in view Y, and we won't know that unless we test for all themes including the custom ones

EDIT: This point will apply later after we start adding custom themes with the new tab experience etc.
2. I feel like we should have separate values for new tab page UI, and for general components. I supposed some people/publishers might want to have a cool new tab experience with pink colors and other bling bling, but for regular browsing you don't necessarily want to have that pink tint everywhere.
This would also help with my concern from point number 1

These 2 points are not blockers by any means, it's just what came up to my mind while reviewing the code

iccub · 2019-09-06T09:00:44Z

BraveShared/BraveStrings.swift

+
+    // MARK: - Themes
+
+    public static let ThemesDisplayBrightness = NSLocalizedString("ThemesDisplayBrightness", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Display & Brightness", comment: "Setting to choose the user interface theme for normal browsing mode, contains choices like 'light' or 'dark' themes")


I remember Sriram talking about changing this setting title, have we decided on it?

Haven't heard anything, we're going with this for now, can adjust in future 🤷‍♂

iccub · 2019-09-06T09:03:14Z

Client/Extensions/AppearanceExtensions.swift

+        UINavigationBar.appearance().tintColor = colors.accent
+        UINavigationBar.appearance().appearanceBarTintColor = colors.header
+
+//        UISwitch.appearance().tintColor = colors.accent


iccub · 2019-09-06T09:27:27Z

Client/Frontend/Browser/Onboarding/OnboardingNavigationController.swift

@@ -47,12 +47,12 @@ class OnboardingNavigationController: UINavigationController {
        case shieldsInfo

        /// Returns new ViewController associated with the screen type
-        func viewController(with profile: Profile) -> OnboardingViewController {
+        func viewController(with profile: Profile, theme: Theme) -> OnboardingViewController {


Are we passing Theme here because selectedTab is not available yet?
Could we use theme.of nil so it infers the theme based on preferences/privacy mode

No, mostly just no reason for on-boarding to get a tab, just giving it the minimal of what it needs. At some point it may not even need that.

We may be refactoring on-boarding to be light/dark vs being created from a theme file.

iccub · 2019-09-06T09:32:04Z

Client/Application/AppDelegate.swift

+        // In order to properly apply background and align this with the rest of the UI (keyboard / header)
+        // this needs to be called. UI could be handled internally to view systems,
+        // but then keyboard may misalign with Brave selected theme override
+        Theme.of(nil).applyAppearanceProperties()


I feel like we could have another Theme method for getting theme by preference/privacy mode, Theme.of(nil) isn't readable, you need to dig into the method to see what is gonna happen

something like Theme.default or Theme.byPrivacyMode

The main thing is ideally this is quite rare. I ma primary hesitant to use a .default or .mode since it should be so rarely used I don't want to make it super convenient. Otherwise, it may just be used naively.

iccub · 2019-09-06T09:37:15Z

Client/Frontend/Browser/ReaderModeBarView.swift

@@ -89,7 +89,9 @@ class ReaderModeBarView: UIView {
 extension ReaderModeBarView: Themeable {

    func applyTheme(_ theme: Theme) {
-        backgroundColor = UIColor.Browser.Background.colorFor(theme)
-        buttonTintColor = UIColor.Browser.Tint.colorFor(theme)
+        styleChildren(theme: theme)


note to myself: there are no themable children in this view. I think it's still a good practice to always call styleChildren

I added a comment to the protocol with this info, although not technically enforceable. Similar to calling super on overridden methods I suppose.

iccub · 2019-09-06T09:39:53Z

Client/Frontend/Browser/TabTrayController.swift

            tabDataSource.tabs = tabManager.tabsForCurrentMode
-            applyTheme(privateMode ? .private : .regular)
+            /*


Convert to /// comment lines. Emojis can stay 🤣

Moved to using // as /// is generally used for method / class documentation. but got rid of the /* */ thing 😄

iccub · 2019-09-06T09:49:25Z

Client/Frontend/Browser/Toolbars/BottomToolbar/BottomToolbarView.swift

-    let shareButton = ToolbarButton()
-    let addTabButton = ToolbarButton()
-    let menuButton = ToolbarButton()
+    let tabsButton = TabsButton(top: false)


At first I thought it was initial work to support fully custom toolbars, and its placement

iccub · 2019-09-06T10:02:52Z

Client/Frontend/Shields/ShieldsViewController.swift

@@ -144,6 +144,7 @@ class ShieldsViewController: UIViewController, PopoverContentComponent {

    override func loadView() {
        view = View()
+        shieldsView.set(theme: Theme.of(tab))


we can't use applyTheme/styleChildren here?

We can, fixed.

iccub · 2019-09-06T10:16:33Z

Client/Frontend/Widgets/Themes/Theme.swift

+        case `private` = "C5CB0D9A-5467-432C-AB35-1A78C55CFB41"
+
+        // TODO: Theme: Remove with `rawValue`
+        var id: String {


Can be replaced with rawValue as the comment says

jhreis · 2019-09-06T15:17:12Z

Yes, this is a pretty serious issue. I think we would generally be able to define themes that work for our current decisions, but it will break if people use "unusual" theming choices (e.g. exact same header / footer / home colors). You also mention the user expectation for themes, and this is a real thing too. Setting the header color adjusted the settings table view.

It was a hard balance to identify some theme file that would work relatively well with fewer items.

One approach that we may take in the future is defining not specific colors but genre's of colors, like primary, secondary, etc...

Something that would require some more serious thought and be planned out in conjunction with Design. The current system basically requires us to audit themes pretty closely

Agreed, potentially related to point # 1.

iccub

Approved. It must have been a lot of effort to make the app look good for all 3 themes, good job on that :)
I ran some tests on my devices and haven't noticed any bad UI, we should be good to go

jhreis force-pushed the themes branch 2 times, most recently from 2f80463 to 1aa1f3e Compare August 10, 2019 01:06

jhreis added 8 commits August 9, 2019 16:21

Added new theming mechanism built off of JSON files.

09c10ab

Added Transparencies to theme file.

c5fe2cc

Added private theme.

da616c3

Added styleChildren call to all Themeable children.

08d688d

A few smaller views had elements styled.

Added some clarifying comments.

6760afb

Fixed theming issue with home page and tabs.

f357eb1

Unselected TabBarCell now follow addressBar color for deselection.

5c9a457

Fixed up URL / Address bar coloring.

20ae592

jhreis force-pushed the themes branch from 1aa1f3e to 20ae592 Compare August 10, 2019 02:21

jhreis and others added 19 commits August 9, 2019 16:26

Added accent color to theming file.

f4eaed8

Updated more attributes throughout, removing some now unneeded consta…

cb924f4

…nt colors.

Removed all BrowserColor code.

517f002

Fixed up tab tray to respect better color theming.

0abb459

Removed TabTray color constants.

b8def89

Using Theme.of in more necessary places.

77d2807

Refactored ToolbarButton to be more stylistically flexible.

b4611aa

Fixed up TabsButton for theming.

b1c9c52

Fixed reader mode button to use accent color for selection.

3b7c2e9

migrate to Swift5/Xcode 11

c93646e

Update Alamofire to beta 7.

afbc261

Update tests to swift5, fix SearchSuggestion Alamofire change.

a960475

Cleaned up TabsButton creation.

e4fc23c

Added theming comment to TabTray for unconventional logic.

b1539b2

Fixed up the private mode toggle button.

1c25db3

Updated main menu to support themes.

979d5e6

Relocated theming json files.

58b21ae

Update bootstrap script and KIF framework.

616effa

Added theme file navigation for identifying desired themes.

705f07e

jhreis added 17 commits August 29, 2019 16:33

Updated dark and private theme files.

b391bfc

Updated all theme copy and made it translatable.

d35ad4b

Merge branch 'development' into themes

2db24bb

Merge branch 'development' into themes

f6f1f86

Fixed up theme for search suggestions.

82d429e

Updated Light and Dark metadata theme files.

0877f49

Merge branch 'development' into themes

30a917e

Merge branch 'development' into themes

aac7073

Private mode button now themed correctly when opening menu.

5e85e5c

Merge branch 'development' into themes

1672219

Updated iOS 12 theming to better support upgrading to iOS 13.

7e0d126

Merge branch 'development' into themes

e9de252

Merge branch 'development' into themes

cc80f41

Fix #1488: Changing Private Browsing Only, adjusts theme correctly.

b0ee471

Cleaned up a bunch of comments and cleaned up code.

21b880f

Switched from using tab.type to tab.theme for theme change detection.

bc537a8

Removed unneeded comment.

38f5e5d

jhreis mentioned this pull request Sep 6, 2019

Toolbar Theming Cleanup #1494

Closed

6 tasks

jhreis commented Sep 6, 2019

View reviewed changes

jhreis changed the title ~~Themes~~ Fix #1493: Themes / Dark Mode Support Sep 6, 2019

jhreis requested a review from iccub September 6, 2019 05:44

jhreis marked this pull request as ready for review September 6, 2019 05:44

iccub suggested changes Sep 6, 2019

View reviewed changes

jhreis added 3 commits September 6, 2019 04:52

Cleaned up comments.

c8b5721

Improved ShieldView API for themeable interface.

50fb67f

Removed DefaultTheme's id prop (use rawValue).

4c90ca8

iccub approved these changes Sep 6, 2019

View reviewed changes

jhreis merged commit 45787a9 into development Sep 6, 2019

jhreis deleted the themes branch September 6, 2019 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1493: Themes / Dark Mode Support #1324

Fix #1493: Themes / Dark Mode Support #1324

jhreis commented Aug 2, 2019 •

edited

Loading

jhreis Sep 6, 2019

jhreis Sep 6, 2019

iccub Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

jhreis Sep 6, 2019

iccub left a comment •

edited

Loading

iccub Sep 6, 2019

jhreis Sep 6, 2019

iccub Sep 6, 2019

iccub Sep 6, 2019

jhreis Sep 6, 2019

iccub Sep 6, 2019

jhreis Sep 6, 2019

iccub Sep 6, 2019 •

edited

Loading

jhreis Sep 6, 2019

iccub Sep 6, 2019

jhreis Sep 6, 2019

iccub Sep 6, 2019

iccub Sep 6, 2019

jhreis Sep 6, 2019

iccub Sep 6, 2019

jhreis commented Sep 6, 2019 •

edited

Loading

iccub left a comment

		@@ -1,57 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -0,0 +1,336 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public


		// MARK: - Themes

		public static let ThemesDisplayBrightness = NSLocalizedString("ThemesDisplayBrightness", tableName: "BraveShared", bundle: Bundle.braveShared, value: "Display & Brightness", comment: "Setting to choose the user interface theme for normal browsing mode, contains choices like 'light' or 'dark' themes")

Fix #1493: Themes / Dark Mode Support #1324

Fix #1493: Themes / Dark Mode Support #1324

Conversation

jhreis commented Aug 2, 2019 • edited Loading

Summary of Changes

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iccub Sep 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhreis commented Sep 6, 2019 • edited Loading

iccub left a comment

Choose a reason for hiding this comment

jhreis commented Aug 2, 2019 •

edited

Loading

iccub left a comment •

edited

Loading

iccub Sep 6, 2019 •

edited

Loading

jhreis commented Sep 6, 2019 •

edited

Loading