Repository: microsoft/terminal (language: cpp)

Issue:
nits from themes PR
see #12992 

________________________________________
* [ ] In `src/cascadia/TerminalSettingsModel/Theme.idl`:
```diff
> +        Color,
+        TerminalBackground
+    };
+
+    runtimeclass ThemeColor
+    {
+        ThemeColor();
+        static ThemeColor FromColor(Microsoft.Terminal.Core.Color color);
+        static ThemeColor FromAccent();
+        static ThemeColor FromTerminalBackground();
+
+        Microsoft.Terminal.Core.Color Color { get; };
+        ThemeColorType ColorType;
+
+        static Microsoft.Terminal.Core.Color ColorFromBrush(Windows.UI.Xaml.Media.Brush brush);
+        Windows.UI.Xaml.Media.Brush Evaluate(Windows.UI.Xaml.ResourceDictionary res,
```
XAML in the settings model -- this will cause trouble for a consumer like the shell extension in the future.

* [ ] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp`:
```diff
> +        // Surprisingly, though this is called every time we navigate to the page,
+        // the list does not keep growing on each navigation.
```
This comment has a VERY threatening aura! I am almost certain it will break at some point...
_____________
* [x] In `src/cascadia/TerminalApp/AppLogic.cpp`:
```diff
> @@ -964,7 +960,7 @@ namespace winrt::TerminalApp::implementation
 
     void AppLogic::_RefreshThemeRoutine()
     {
-        _ApplyTheme(_settings.GlobalSettings().Theme());
+        _ApplyTheme(GetRequestedTheme());
```
I feel like ApplyTheme should take a Theme ;P
________________________________________
* [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`:
```diff
> +        //
+        // This helper allows us to instead lookup the value of a resource
+        // specified by `key` for the given `requestedTheme`, from the
+        // dictionaries in App.xaml. Make sure the value is actually there!
+        // Otherwise this'll throw like any other Lookup for a resource that
+        // isn't there.
+        static const auto lookup = [](auto& res, auto& requestedTheme, auto& key) {
+            // You want the Default version of the resource? Great, the App is
+            // always in the OS theme. Just look it up and be done.
+            if (requestedTheme == ElementTheme::Default)
+            {
+                return res.Lookup(key);
+            }
+            static const auto lightKey = winrt::box_value(L"Light");
+            static const auto darkKey = winrt::box_value(L"Dark");
+            // There isn't an ElementTheme::HighContrast.
```
so... what exactly happens in HC then?
________________________________________
* [ ] In `src/cascadia/TerminalSettingsModel/Theme.cpp`:
```diff
> +        {                                                                                      \
+            return json.isObject();                                                            \
+        }                                                                                      \
+                                                                                               \
+        Json::Value ToJson(const nameSpace::name& val)                                         \
+        {                                                                                      \
+            if (val == nullptr)                                                                \
+                return Json::Value::null;                                                      \
+            Json::Value json{ Json::ValueType::objectValue };                                  \
+            macro(THEME_SETTINGS_TO_JSON);                                                     \
+            return json;                                                                       \
+        }                                                                                      \
+                                                                                               \
+        std::string TypeDescription() const                                                    \
+        {                                                                                      \
+            return "name (You should never see this)";                                         \
```
lol this will actually say the word "name" since it's not in a macro-stringify expression


<details>
<summary>resolved</summary>
________________________________________
* [x] Does this work with background images?

Yep, sure does.

________________________________________
* [x] It looks insane to have a light terminal BG titlebar, and in dark OS theme - the caption btns are also white:

![image](https://user-images.githubusercontent.com/18356694/177865865-8c1fc66f-05fd-4680-9c27-93e7f63bfac8.png)

________________________________________
* [x] In `src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp`:
```diff
> +                "background": "#FFFF8800",
+                "unfocusedBackground": "#FF884400"
```
wait but utils parses RGBA
how is this test passing?


wait, i am so confused
___________________________
* [x] In `src/cascadia/TerminalSettingsModel/Theme.h`:
```diff
> +
+        static com_ptr<Theme> FromJson(const Json::Value& json);
+        void LayerJson(const Json::Value& json);
+        Json::Value ToJson() const;
+
+        winrt::Windows::UI::Xaml::ElementTheme RequestedTheme() const noexcept;
+
+        WINRT_PROPERTY(winrt::hstring, Name);
+
+        MTSM_THEME_SETTINGS(THEME_SETTINGS_INITIALIZE)
+
+    private:
+    };
+
+#undef THEME_SETTINGS_INITIALIZE
+#undef THEME_SETTINGS_COPY
```
do we need to undef COPY_THEME...?
________________________________________
* [x] In `src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h`:
```diff
> +        }
+        case winrt::Microsoft::Terminal::Settings::Model::ThemeColorType::Color:
+        {
+            return til::u16u8(til::color{ val.Color() }.ToHexString(false));
+        }
+        case winrt::Microsoft::Terminal::Settings::Model::ThemeColorType::TerminalBackground:
+        {
+            return "terminalBackground";
+        }
+        }
+        return til::u16u8(til::color{ val.Color() }.ToHexString(false));
+    }
+
+    std::string TypeDescription() const
+    {
+        return "ThemeColor (#rrggbb, #rgb, #aarrggbb, accent, terminalBackground)";
```
isn't it rrggbbaa?
________________________________________
* [x] In `src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h`:
```diff
> @@ -547,6 +547,79 @@ JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage)
     };
 };
 
+template<>
+struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt::Microsoft::Terminal::Settings::Model::ThemeColor>
+{
+    winrt::Microsoft::Terminal::Settings::Model::ThemeColor FromJson(const Json::Value& json)
+    {
+        if (json == Json::Value::null)
+        {
+            return nullptr;
+        }
+        const auto string{ Detail::GetStringView(json) };
+        if (string == "accent")
```
nit: prefer declaring these magic constant values in the class body and referencing them later
________________________________________
* [x] In `src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp`:
```diff
> @@ -1092,6 +1113,20 @@ Json::Value CascadiaSettings::ToJson() const
     }
     json[JsonKey(SchemesKey)] = schemes;
 
+    Json::Value themes{ Json::ValueType::arrayValue };
+    for (const auto& entry : _globals->Themes())
+    {
+        // Ignore the built in themes, when serializing the themes back out. We
```
implicit contract: users cannot overwrite system themes
________________________________________
* [x] In `src/cascadia/TerminalApp/AppLogic.h`:
```diff
>          // -------------------------------- WinRT Events ---------------------------------
+        // PropertyChanged is surprisingly not a typed event, so we'll define that one manually.
```
W A T

do we do that everywhere? if not, why not? Should we have done here what we do elsewhere?

________________________________________
* [x] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.idl`:
```diff
> @@ -21,7 +21,8 @@ namespace Microsoft.Terminal.Settings.Editor
         IInspectable CurrentLanguage;
 
         IInspectable CurrentTheme;
-        Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Editor.EnumEntry> ThemeList { get; };
+        static String ThemeNameConverter(Microsoft.Terminal.Settings.Model.Theme theme);
+        Windows.Foundation.Collections.IObservableVector<Microsoft.Terminal.Settings.Model.Theme> ThemeList { get; };
```
@PankajBhojwani FYI since this will impact MVVM -- this is a raw exposure of a settings model object
________________________________________
* [x] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.cpp`:
```diff
> +    constexpr std::wstring_view systemThemeName{ L"system" };
+    constexpr std::wstring_view darkThemeName{ L"dark" };
+    constexpr std::wstring_view lightThemeName{ L"light" };
```
these became global constants in TSE, but not in TSM -- curious!
________________________________________
* [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`:
```diff
> +        _activated = activated;
+        _updateTabRowColors();
+    }
```
so _activated is ignored?
________________________________________
* [x] In `src/cascadia/TerminalSettingsModel/Theme.cpp`:
```diff
> +    HKEY hKey{ nullptr };
+    if (RegOpenKeyEx(HKEY_CURRENT_USER, RegKeyDwm, 0, KEY_READ, &hKey) == ERROR_SUCCESS)
+    {
+        return wil::unique_hkey{ hKey };
+    }
+    return nullptr;
+}
+static DWORD readDwmSubValue(const wil::unique_hkey& dwmRootKey, const wchar_t* key)
+{
+    DWORD val{ 0 };
+    DWORD size{ sizeof(val) };
+    LOG_IF_FAILED(RegQueryValueExW(dwmRootKey.get(), key, nullptr, nullptr, reinterpret_cast<BYTE*>(&val), &size));
+    return val;
+}
+
+static til::color _getAccentColorForTitlebar()
```
do we reload this when the accent color changes?
________________________________________
________________________________________
* [x] In `src/cascadia/TerminalSettingsModel/Theme.cpp`:
```diff
> +    }
+
+    return theme;
+}
+
+// Method Description:
+// - Create a new instance of this class from a serialized JsonObject.
+// Arguments:
+// - json: an object which should be a serialization of a ColorScheme object.
+// Return Value:
+// - Returns nullptr for invalid JSON.
+winrt::com_ptr<Theme> Theme::FromJson(const Json::Value& json)
+{
+    auto result = winrt::make_self<Theme>();
+
+    if (json.isString())
```
we control the entire themes array, from its inception to its death. Who would put a string in it?
________________________________________
* [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`:
```diff
> +                    if (winrt::unbox_value<winrt::hstring>(dictionaryKey) !=
+                        winrt::unbox_value<winrt::hstring>(requestedThemeKey))
```
but do we need users to crash if this somehow happens in production?
________________________________________
* [x] In `src/cascadia/TerminalApp/TerminalPage.cpp`:
```diff
> +                }
+            }
+
+            // We didn't find it in the requested dict, fall back to the default dictionary.
+            return res.Lookup(key);
+        };
+
+        // Use our helper to lookup the theme-aware version of the resource.
+        const auto tabViewBackgroundKey = winrt::box_value(L"TabViewBackground");
+        const auto backgroundSolidBrush = lookup(res, requestedTheme, tabViewBackgroundKey).as<Media::SolidColorBrush>();
+
+        til::color bgColor = backgroundSolidBrush.Color();
+
+        if (_settings.GlobalSettings().UseAcrylicInTabRow())
+        {
+            const til::color backgroundColor = backgroundSolidBrush.Color();
```
You marked it as resolved... but the line of code is still here.


</details>


<details>
<summary>follow ups</summary>
________________________________________
* [ ] In `src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp`:
```diff
> @@ -531,6 +532,25 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
         }
     }
 
+    {
+        for (const auto& themeJson : json.themes)
+        {
+            if (const auto theme = Theme::FromJson(themeJson))
+            {
+                if (origin != OriginTag::InBox &&
```
follow up: we should add an origin tag to themes, after #12800 lands.
________________________________________
* [ ] In `src/cascadia/TerminalSettingsEditor/GlobalAppearance.xaml`:
```diff
>                            ItemsSource="{x:Bind ThemeList, Mode=OneWay}"
                           SelectedItem="{x:Bind CurrentTheme, Mode=TwoWay}"
-                          Style="{StaticResource ComboBoxSettingStyle}" />
+                          Style="{StaticResource ComboBoxSettingStyle}">
+                    <ComboBox.ItemTemplate>
+                        <DataTemplate x:DataType="model:Theme">
+                            <TextBlock Text="{x:Bind local:GlobalAppearance.ThemeNameConverter((model:Theme)), Mode=OneWay}" />
```
@carlos-zamora, can you make sure this is still accessible? we have had issues with putting custom data templates into combo boxes.
________________________________________

* [ ] In `src/types/utils.cpp`:
```diff
>      }
-    else
+    else if (str.size() == 7)
+    {
+        rStr = std::string(&str.at(1), 2);
+        gStr = std::string(&str.at(3), 2);
+        bStr = std::string(&str.at(5), 2);
+        aStr = "ff";
```
I don't love that we force a string parser to parse ff just to get 255... ;P
________________________________________
</details>


Discussion / hints:
> but do we need users to crash if this somehow happens in production?

That one's interesting. I REALLY don't think that should ever be happening - your install has got to be all sorts of fucked up to have a ResourceDictionary, in the Terminal's resources, under a key that's _not_ an hstring. IDK to me, if you're a user that hit that, you were probably playing stupid games.

(inb4 I regret this comment in like, 8 weeks)




Fix patch:
diff --git a/src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp b/src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp
index 25e115a0cd7..4e3bdacc254 100644
--- a/src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/ThemeTests.cpp
@@ -47,6 +47,10 @@ namespace SettingsModelLocalTests
         {
             return Core::Color{ r, g, b, 255 };
         }
+        static Core::Color rgba(uint8_t r, uint8_t g, uint8_t b, uint8_t a) noexcept
+        {
+            return Core::Color{ r, g, b, a };
+        }
     };
 
     void ThemeTests::ParseSimpleTheme()
@@ -56,7 +60,7 @@ namespace SettingsModelLocalTests
             "tabRow":
             {
                 "background": "#FFFF8800",
-                "unfocusedBackground": "#FF884400"
+                "unfocusedBackground": "#FF8844"
             },
             "window":
             {
@@ -72,7 +76,8 @@ namespace SettingsModelLocalTests
         VERIFY_IS_NOT_NULL(theme->TabRow());
         VERIFY_IS_NOT_NULL(theme->TabRow().Background());
         VERIFY_ARE_EQUAL(Settings::Model::ThemeColorType::Color, theme->TabRow().Background().ColorType());
-        VERIFY_ARE_EQUAL(rgb(0xff, 0x88, 0x00), theme->TabRow().Background().Color());
+        VERIFY_ARE_EQUAL(rgba(0xff, 0xff, 0x88, 0x00), theme->TabRow().Background().Color());
+        VERIFY_ARE_EQUAL(rgba(0xff, 0x88, 0x44, 0xff), theme->TabRow().UnfocusedBackground().Color());
 
         VERIFY_IS_NOT_NULL(theme->Window());
         VERIFY_ARE_EQUAL(winrt::Windows::UI::Xaml::ElementTheme::Light, theme->Window().RequestedTheme());
@@ -101,7 +106,7 @@ namespace SettingsModelLocalTests
             "name": "noWindow",
             "tabRow":
             {
-                "background": "#FF112233",
+                "background": "#112233",
                 "unfocusedBackground": "#FF884400"
             },
         })" };
@@ -126,7 +131,7 @@ namespace SettingsModelLocalTests
             "name": "nullWindow",
             "tabRow":
             {
-                "background": "#FF112233",
+                "background": "#112233",
                 "unfocusedBackground": "#FF884400"
             },
             "window": null
diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp
index 1b0787d2538..e85c3cff3fa 100644
--- a/src/cascadia/TerminalApp/AppLogic.cpp
+++ b/src/cascadia/TerminalApp/AppLogic.cpp
@@ -958,9 +958,16 @@ namespace winrt::TerminalApp::implementation
     }
     CATCH_LOG()
 
+    // Method Description:
+    // - Update the current theme of the application. This will trigger our
+    //   RequestedThemeChanged event, to have our host change the theme of the
+    //   root of the application.
+    // Arguments:
+    // - newTheme: The ElementTheme to apply to our elements.
     void AppLogic::_RefreshThemeRoutine()
     {
-        _ApplyTheme(GetRequestedTheme());
+        // Propagate the event to the host layer, so it can update its own UI
+        _RequestedThemeChangedHandlers(*this, Theme());
     }
 
     // Function Description:
@@ -1069,18 +1076,6 @@ namespace winrt::TerminalApp::implementation
         return _settings;
     }
 
-    // Method Description:
-    // - Update the current theme of the application. This will trigger our
-    //   RequestedThemeChanged event, to have our host change the theme of the
-    //   root of the application.
-    // Arguments:
-    // - newTheme: The ElementTheme to apply to our elements.
-    void AppLogic::_ApplyTheme(const Windows::UI::Xaml::ElementTheme& newTheme)
-    {
-        // Propagate the event to the host layer, so it can update its own UI
-        _RequestedThemeChangedHandlers(*this, newTheme);
-    }
-
     UIElement AppLogic::GetRoot() noexcept
     {
         return _root.as<winrt::Windows::UI::Xaml::Controls::Control>();
diff --git a/src/cascadia/TerminalApp/AppLogic.h b/src/cascadia/TerminalApp/AppLogic.h
index 2ba2deaeffb..7abf70f725a 100644
--- a/src/cascadia/TerminalApp/AppLogic.h
+++ b/src/cascadia/TerminalApp/AppLogic.h
@@ -133,10 +133,15 @@ namespace winrt::TerminalApp::implementation
 
         // -------------------------------- WinRT Events ---------------------------------
         // PropertyChanged is surprisingly not a typed event, so we'll define that one manually.
+        // Usually we'd just do
+        //    WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
+        //
+        // But what we're doing here is exposing the Page's PropertyChanged _as
+        // our own event_. It's a FORWARDED_CALLBACK, essentially.
         winrt::event_token PropertyChanged(Windows::UI::Xaml::Data::PropertyChangedEventHandler const& handler) { return _root->PropertyChanged(handler); }
         void PropertyChanged(winrt::event_token const& token) { _root->PropertyChanged(token); }
 
-        TYPED_EVENT(RequestedThemeChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::ElementTheme);
+        TYPED_EVENT(RequestedThemeChanged, winrt::Windows::Foundation::IInspectable, winrt::Microsoft::Terminal::Settings::Model::Theme);
         TYPED_EVENT(SettingsChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable);
         TYPED_EVENT(SystemMenuChangeRequested, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::SystemMenuChangeArgs);
 
@@ -191,8 +196,6 @@ namespace winrt::TerminalApp::implementation
         void _ReloadSettings();
         void _OpenSettingsUI();
 
-        void _ApplyTheme(const Windows::UI::Xaml::ElementTheme& newTheme);
-
         bool _hasCommandLineArguments{ false };
         bool _hasSettingsStartupActions{ false };
         std::vector<Microsoft::Terminal::Settings::Model::SettingsLoadWarnings> _warnings;
diff --git a/src/cascadia/TerminalApp/AppLogic.idl b/src/cascadia/TerminalApp/AppLogic.idl
index 42096bd08d1..7f2f0d15921 100644
--- a/src/cascadia/TerminalApp/AppLogic.idl
+++ b/src/cascadia/TerminalApp/AppLogic.idl
@@ -121,7 +121,7 @@ namespace TerminalApp
         event Windows.Foundation.TypedEventHandler<Object, Windows.UI.Xaml.UIElement> SetTitleBarContent;
         event Windows.Foundation.TypedEventHandler<Object, String> TitleChanged;
         event Windows.Foundation.TypedEventHandler<Object, LastTabClosedEventArgs> LastTabClosed;
-        event Windows.Foundation.TypedEventHandler<Object, Windows.UI.Xaml.ElementTheme> RequestedThemeChanged;
+        event Windows.Foundation.TypedEventHandler<Object, Microsoft.Terminal.Settings.Model.Theme> RequestedThemeChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> FocusModeChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> FullscreenChanged;
         event Windows.Foundation.TypedEventHandler<Object, Object> ChangeMaximizeRequested;
diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
index 9f0c42cf66f..131369d25f4 100644
--- a/src/cascadia/TerminalApp/TerminalPage.cpp
+++ b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -4092,7 +4092,6 @@ namespace winrt::TerminalApp::implementation
 
         if (_settings.GlobalSettings().UseAcrylicInTabRow())
         {
-            const til::color backgroundColor = backgroundSolidBrush.Color();
             const auto acrylicBrush = Media::AcrylicBrush();
             acrylicBrush.BackgroundSource(Media::AcrylicBackgroundSource::HostBackdrop);
             acrylicBrush.FallbackColor(bgColor);
@@ -4101,27 +4100,25 @@ namespace winrt::TerminalApp::implementation
 
             TitlebarBrush(acrylicBrush);
         }
-        else if (theme.TabRow())
+        else if (const auto tabRowBg{ _activated ? theme.TabRow().Background() :
+                                                   theme.TabRow().UnfocusedBackground() };
+                 tabRowBg != nullptr && theme.TabRow() != nullptr)
         {
-            if (const auto tabRowBg{ _activated ? theme.TabRow().Background() :
-                                                  theme.TabRow().UnfocusedBackground() })
-            {
-                const auto terminalBrush = [this]() -> Media::Brush {
-                    if (const auto& control{ _GetActiveControl() })
-                    {
-                        return control.BackgroundBrush();
-                    }
-                    else if (auto settingsTab = _GetFocusedTab().try_as<TerminalApp::SettingsTab>())
-                    {
-                        return settingsTab.Content().try_as<Settings::Editor::MainPage>().BackgroundBrush();
-                    }
-                    return nullptr;
-                }();
+            const auto terminalBrush = [this]() -> Media::Brush {
+                if (const auto& control{ _GetActiveControl() })
+                {
+                    return control.BackgroundBrush();
+                }
+                else if (auto settingsTab = _GetFocusedTab().try_as<TerminalApp::SettingsTab>())
+                {
+                    return settingsTab.Content().try_as<Settings::Editor::MainPage>().BackgroundBrush();
+                }
+                return nullptr;
+            }();
 
-                const auto themeBrush{ tabRowBg.Evaluate(res, terminalBrush, true) };
-                bgColor = ThemeColor::ColorFromBrush(themeBrush);
-                TitlebarBrush(themeBrush);
-            }
+            const auto themeBrush{ tabRowBg.Evaluate(res, terminalBrush, true) };
+            bgColor = ThemeColor::ColorFromBrush(themeBrush);
+            TitlebarBrush(themeBrush);
         }
         else
         {
diff --git a/src/cascadia/TerminalApp/TitlebarControl.cpp b/src/cascadia/TerminalApp/TitlebarControl.cpp
index 011aed7c453..bd5748eb279 100644
--- a/src/cascadia/TerminalApp/TitlebarControl.cpp
+++ b/src/cascadia/TerminalApp/TitlebarControl.cpp
@@ -8,6 +8,8 @@
 
 #include "TitlebarControl.h"
 
+#include "ColorHelper.h"
+
 #include "TitlebarControl.g.cpp"
 
 namespace winrt::TerminalApp::implementation
@@ -21,6 +23,25 @@ namespace winrt::TerminalApp::implementation
         MinMaxCloseControl().MinimizeClick({ this, &TitlebarControl::Minimize_Click });
         MinMaxCloseControl().MaximizeClick({ this, &TitlebarControl::Maximize_Click });
         MinMaxCloseControl().CloseClick({ this, &TitlebarControl::Close_Click });
+
+        // Listen for changes to the Background. If the Background changes,
+        // we'll want to manually adjust the RequestedTheme of our caption
+        // buttons, so the foreground stands out against whatever BG color was
+        // selected for us.
+        //
+        // This is how you register a PropertyChanged event for the Background
+        // property of a Grid. The Background property is defined in the base
+        // class Panel.
+        const auto bgProperty{ winrt::Windows::UI::Xaml::Controls::Panel::BackgroundProperty() };
+        RegisterPropertyChangedCallback(bgProperty, [weakThis = get_weak(), bgProperty](auto& /*sender*/, auto& e) {
+            if (auto self{ weakThis.get() })
+            {
+                if (e == bgProperty)
+                {
+                    self->_backgroundChanged(self->Background());
+                }
+            }
+        });
     }
 
     double TitlebarControl::CaptionButtonWidth()
@@ -144,4 +165,26 @@ namespace winrt::TerminalApp::implementation
         MinMaxCloseControl().ReleaseButtons();
     }
 
+    void TitlebarControl::_backgroundChanged(winrt::Windows::UI::Xaml::Media::Brush brush)
+    {
+        // Loosely cribbed from TerminalPage::_SetNewTabButtonColor
+        til::color c;
+        if (auto acrylic = brush.try_as<winrt::Windows::UI::Xaml::Media::AcrylicBrush>())
+        {
+            c = acrylic.TintColor();
+        }
+        else if (auto solidColor = brush.try_as<winrt::Windows::UI::Xaml::Media::SolidColorBrush>())
+        {
+            c = solidColor.Color();
+        }
+        else
+        {
+            return;
+        }
+
+        const auto isBrightColor = ColorHelper::IsBrightColor(c);
+        MinMaxCloseControl().RequestedTheme(isBrightColor ? winrt::Windows::UI::Xaml::ElementTheme::Light :
+                                                            winrt::Windows::UI::Xaml::ElementTheme::Dark);
+    }
+
 }
diff --git a/src/cascadia/TerminalApp/TitlebarControl.h b/src/cascadia/TerminalApp/TitlebarControl.h
index 5479032a7fa..259361302ad 100644
--- a/src/cascadia/TerminalApp/TitlebarControl.h
+++ b/src/cascadia/TerminalApp/TitlebarControl.h
@@ -31,6 +31,8 @@ namespace winrt::TerminalApp::implementation
     private:
         void _OnMaximizeOrRestore(byte flag);
         HWND _window{ nullptr }; // non-owning handle; should not be freed in the dtor.
+
+        void _backgroundChanged(winrt::Windows::UI::Xaml::Media::Brush brush);
     };
 }
 
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
index f8e4dc4f80f..1d6f71c6344 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -41,6 +41,10 @@ static constexpr std::string_view ProfilesListKey{ "list" };
 static constexpr std::string_view SchemesKey{ "schemes" };
 static constexpr std::string_view ThemesKey{ "themes" };
 
+constexpr std::wstring_view systemThemeName{ L"system" };
+constexpr std::wstring_view darkThemeName{ L"dark" };
+constexpr std::wstring_view lightThemeName{ L"light" };
+
 static constexpr std::wstring_view jsonExtension{ L".json" };
 static constexpr std::wstring_view FragmentsSubDirectory{ L"\\Fragments" };
 static constexpr std::wstring_view FragmentsPath{ L"\\Microsoft\\Windows Terminal\\Fragments" };
@@ -538,7 +542,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
             if (const auto theme = Theme::FromJson(themeJson))
             {
                 if (origin != OriginTag::InBox &&
-                    (theme->Name() == L"system" || theme->Name() == L"light" || theme->Name() == L"dark"))
+                    (theme->Name() == systemThemeName || theme->Name() == lightThemeName || theme->Name() == darkThemeName))
                 {
                     // If the theme didn't come from the in-box themes, and its
                     // name was one of the reserved names, then just ignore it.
@@ -1119,7 +1123,7 @@ Json::Value CascadiaSettings::ToJson() const
         // Ignore the built in themes, when serializing the themes back out. We
         // don't want to re-include them in the user settings file.
         const auto theme{ winrt::get_self<Theme>(entry.Value()) };
-        if (theme->Name() == L"system" || theme->Name() == L"light" || theme->Name() == L"dark")
+        if (theme->Name() == systemThemeName || theme->Name() == lightThemeName || theme->Name() == darkThemeName)
         {
             continue;
         }
diff --git a/src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h b/src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h
index 50a17a5abb2..292ffbbd190 100644
--- a/src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h
+++ b/src/cascadia/TerminalSettingsModel/TerminalSettingsSerializationHelpers.h
@@ -558,6 +558,9 @@ JSON_ENUM_MAPPER(::winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage)
 template<>
 struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt::Microsoft::Terminal::Settings::Model::ThemeColor>
 {
+    static constexpr std::string_view accentString{ "accent" };
+    static constexpr std::string_view terminalBackgroundString{ "terminalBackground" };
+
     winrt::Microsoft::Terminal::Settings::Model::ThemeColor FromJson(const Json::Value& json)
     {
         if (json == Json::Value::null)
@@ -565,11 +568,11 @@ struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt:
             return nullptr;
         }
         const auto string{ Detail::GetStringView(json) };
-        if (string == "accent")
+        if (string == accentString)
         {
             return winrt::Microsoft::Terminal::Settings::Model::ThemeColor::FromAccent();
         }
-        else if (string == "terminalBackground")
+        else if (string == terminalBackgroundString)
         {
             return winrt::Microsoft::Terminal::Settings::Model::ThemeColor::FromTerminalBackground();
         }
@@ -592,8 +595,8 @@ struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt:
 
         const auto string{ Detail::GetStringView(json) };
         const auto isColorSpec = (string.length() == 9 || string.length() == 7 || string.length() == 4) && string.front() == '#';
-        const auto isAccent = string == "accent";
-        const auto isTerminalBackground = string == "terminalBackground";
+        const auto isAccent = string == accentString;
+        const auto isTerminalBackground = string == terminalBackgroundString;
         return isColorSpec || isAccent || isTerminalBackground;
     }
 
@@ -624,7 +627,7 @@ struct ::Microsoft::Terminal::Settings::Model::JsonUtils::ConversionTrait<winrt:
 
     std::string TypeDescription() const
     {
-        return "ThemeColor (#rrggbb, #rgb, #aarrggbb, accent, terminalBackground)";
+        return "ThemeColor (#rrggbb, #rgb, #rrggbbaa, accent, terminalBackground)";
     }
 };
 
diff --git a/src/cascadia/TerminalSettingsModel/Theme.cpp b/src/cascadia/TerminalSettingsModel/Theme.cpp
index 46e7b52f71a..236cb5ab987 100644
--- a/src/cascadia/TerminalSettingsModel/Theme.cpp
+++ b/src/cascadia/TerminalSettingsModel/Theme.cpp
@@ -266,21 +266,6 @@ winrt::com_ptr<Theme> Theme::FromJson(const Json::Value& json)
 {
     auto result = winrt::make_self<Theme>();
 
-    if (json.isString())
-    {
-        // We found a string, not an object. Just secretly promote that string
-        // to a theme object with just the applicationTheme set from that value.
-        JsonUtils::GetValue(json, result->_Name);
-        winrt::WUX::ElementTheme requestedTheme{ winrt::WUX::ElementTheme::Default };
-        JsonUtils::GetValue(json, requestedTheme);
-
-        auto window{ winrt::make_self<implementation::WindowTheme>() };
-        window->RequestedTheme(requestedTheme);
-        result->_Window = *window;
-
-        return result;
-    }
-
     JsonUtils::GetValueForKey(json, NameKey, result->_Name);
 
     // This will use each of the ConversionTrait's from above to quickly parse the sub-objects
diff --git a/src/cascadia/TerminalSettingsModel/Theme.h b/src/cascadia/TerminalSettingsModel/Theme.h
index 27fb8cc3e32..b27e42baa46 100644
--- a/src/cascadia/TerminalSettingsModel/Theme.h
+++ b/src/cascadia/TerminalSettingsModel/Theme.h
@@ -96,6 +96,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
 #undef THEME_SETTINGS_INITIALIZE
 #undef THEME_SETTINGS_COPY
+#undef COPY_THEME_OBJECT
 #undef THEME_OBJECT
 }
 
diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp
index 1f929efda20..31e100e8baa 100644
--- a/src/cascadia/WindowsTerminal/AppHost.cpp
+++ b/src/cascadia/WindowsTerminal/AppHost.cpp
@@ -719,7 +719,8 @@ void AppHost::_UpdateTitleBarContent(const winrt::Windows::Foundation::IInspecta
 // - arg: the ElementTheme to use as the new theme for the UI
 // Return Value:
 // - <none>
-void AppHost::_UpdateTheme(const winrt::Windows::Foundation::IInspectable&, const winrt::Windows::UI::Xaml::ElementTheme& /*arg*/)
+void AppHost::_UpdateTheme(const winrt::Windows::Foundation::IInspectable&,
+                           const winrt::Microsoft::Terminal::Settings::Model::Theme& /*arg*/)
 {
     _updateTheme();
 }
diff --git a/src/cascadia/WindowsTerminal/AppHost.h b/src/cascadia/WindowsTerminal/AppHost.h
index 5c97ed3be57..2de84742189 100644
--- a/src/cascadia/WindowsTerminal/AppHost.h
+++ b/src/cascadia/WindowsTerminal/AppHost.h
@@ -44,7 +44,7 @@ class AppHost
     void _UpdateTitleBarContent(const winrt::Windows::Foundation::IInspectable& sender,
                                 const winrt::Windows::UI::Xaml::UIElement& arg);
     void _UpdateTheme(const winrt::Windows::Foundation::IInspectable&,
-                      const winrt::Windows::UI::Xaml::ElementTheme& arg);
+                      const winrt::Microsoft::Terminal::Settings::Model::Theme& arg);
     void _FocusModeChanged(const winrt::Windows::Foundation::IInspectable& sender,
                            const winrt::Windows::Foundation::IInspectable& arg);
     void _FullscreenChanged(const winrt::Windows::Foundation::IInspectable& sender,


Test patch: