Repository: microsoft/terminal (language: cpp)

Issue:
Panel split background ignores `requestedTheme`, follows system app mode
# Environment

```none
Windows build number: 10.0.18362.476
Windows Terminal version (if applicable): 0.7.3382.0
```

# Steps to reproduce

Have Windows configured in "light mode", and configure Terminal with `requestedTheme` set to `dark`. Split a tab in two or more panels.

# Expected behavior

The split background should be dark, matching the tabs. The current panel is highlighted with the accent colour.

# Actual behavior

The split background is light, matching the system theme (in particular the "app mode" setting), and ignores the `requestedTheme` Terminal setting. Next to a light grey line, the accent colour is less visible.

The opposite is also true: set windows to "dark mode", Terminal to "light mode", and the splits will follow the system settings.

![image](https://user-images.githubusercontent.com/584584/70645288-9c0dff80-1c3c-11ea-948a-785e9c30e5c8.png)

Panel split background ignores `requestedTheme`, follows system app mode
# Environment

```none
Windows build number: 10.0.18362.476
Windows Terminal version (if applicable): 0.7.3382.0
```

# Steps to reproduce

Have Windows configured in "light mode", and configure Terminal with `requestedTheme` set to `dark`. Split a tab in two or more panels.

# Expected behavior

The split background should be dark, matching the tabs. The current panel is highlighted with the accent colour.

# Actual behavior

The split background is light, matching the system theme (in particular the "app mode" setting), and ignores the `requestedTheme` Terminal setting. Next to a light grey line, the accent colour is less visible.

The opposite is also true: set windows to "dark mode", Terminal to "light mode", and the splits will follow the system settings.

![image](https://user-images.githubusercontent.com/584584/70645288-9c0dff80-1c3c-11ea-948a-785e9c30e5c8.png)



Discussion / hints:
Wow, yea it sure does. Thanks for the report!
For reference:
- colors are defined in [`App.xml > ThemeDictionaries`, ~line 41](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalApp/App.xaml#L41)
- pane borders are assigned in [`Pane.cpp#Pane::_SetupResources`, ~line 1510](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalApp/Pane.cpp#L1510)
I am assigning myself as a reminder: i have a long and sad e-mail thread with the Xaml team about this, and I need to summarize it to explain why we have so much trouble fixing this bug. 
@sweihub peddle your wares elsewhere.
This is the e-mail I sent to the Xaml folks (April 2, 2020)

## Requested Themes, a saga

Windows Terminal is built as an application hosting a Xaml Island, with its own Windows.UI.Xaml.Application composed subclass.

Terminal *also* allows its users to select a light or a dark application theme independent of the active system theme.

The most straightforward way to do this and have it apply globally to the application is to set **Application.RequestedTheme** on startup.

### Application.RequestedTheme(Dark)

Per the [documentation], it is impossible to set an application’s requested theme after it’s been initialized. This means that Terminal will have to set RequestedTheme in its App’s constructor.

This works perfectly for applications that undergo normal CoreApplication activation: the composing parent class of App is initialized, registers itself with the framework, and RequestedTheme ends up being set before initialization has completed (and it’s locked out.)

In a Xaml island, however, one of two things happen:

1.  The App is initialized before the Xaml Island boots up its CoreWindow/Application integration
    
    1.  The call to **put\_RequestedTheme** fails with an access violation.
    
    2.  This causes a crash.

2.  The App is initialized **after** Xaml Island startup
    
    1.  WindowsXamlManager creates its own FrameworkApplication
    
    2.  That application loads **App.xbf**
    
    3.  The loading of an App.xbf locks out RequestedTheme()
    
    4.  This causes an invalid argument HR/exception (depending on language)

**Conclusion**: It is impossible to set RequestedTheme in a Xaml Islands application.

## How else could we set the theme?

### ElementThemes

It is possible to set an ElementTheme on the root Page of an application. This works “alright” for Terminal, save for a couple issues ranging from minor to significant:

  - Dialogs do not live under the root Page of the application; they are presented in a different visual tree by the dialog API.
      - We try to set the ElementTheme on our dialogs, and this works:
      - Our dialogs mostly follow the ElementTheme we requested, except their *primary buttons* and *context menus* reflect the system/application theme, which we couldn’t change (#5195, #3654)
	  - ![image](https://user-images.githubusercontent.com/189190/100683824-f9a7ca00-332d-11eb-807a-7ab52a85cee5.png)
	  - ![image](https://user-images.githubusercontent.com/189190/100683856-0a584000-332e-11eb-9334-5b3959c87002.png)
	      - The primary button of the dialog is in that circle.
  - Terminal implements split panes as Grids containing Borders.
      - Those borders are either Accent or SystemChromeLow color.
      - We’re loading SystemChromeLow at runtime(1), because panes are not Xaml controls.
      - It appears that we’re unable to get the active ElementTheme version of SystemChromeLow because we’re using the Application’s resource dictionary, and that reflects the system/application theme (which we couldn’t change) (#3917)
	  - ![image](https://user-images.githubusercontent.com/189190/100683868-117f4e00-332e-11eb-9aa5-bd9b4a955618.png)
	      - To the left of the purple line should be a white line, not a gray one

1: what I wanted to ask was “is it possible for me to create a brush, or otherwise get something that will change color based on the theme of the element it’s placed inside, **at runtime**?”

 

Maybe we can fix the pane one by making it a Xaml control with visual states and theme resource dictionaries? It seemed like it would be a good idea to use visual states to indicate which pane was active and let Xaml handle switching the resources off for us anyway.

We ended up writing a Xaml fragment for the Border inside the pane, giving it theme resources and visual states, and letting it go.

*It turns out that a Border cannot move between visual states because you cannot call GoToState on something that isn’t a Control.*

We ended up putting the Grid in the Xaml fragment containing the Border as well.

*It turns out that a **Grid** cannot move between visual states because you cannot call GoToState on something that isn’t a Control.*

We ended up putting a UserControl around the entire thing, because that’s the only empty Control we could create, to house the Grid and the border. It’s now more complex than just making the controls at runtime.

*And*, after all that the issue still remains: the border has the wrong color when the application theme doesn’t match the system theme.

**Conclusion**: This is actually impossible.

[documentation]: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.application.requestedtheme?view=winrt-19041

The response:

> So there may in fact be no answer here now for WinUI 2/XAML Islands v1.
https://github.com/microsoft/terminal/issues/12356 has a variation of this bug. That one's a hotter fix, so I went with a hack to fix it. However, the branches for that issue have a lot of history that might be valuable. 
IF ONLY WE HAD A HELPER FOR GETTING A RESOURCE OUT OF THE RESOURCES FOR A SPECIFIC THEME
Wow, yea it sure does. Thanks for the report!
For reference:
- colors are defined in [`App.xml > ThemeDictionaries`, ~line 41](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalApp/App.xaml#L41)
- pane borders are assigned in [`Pane.cpp#Pane::_SetupResources`, ~line 1510](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalApp/Pane.cpp#L1510)
I am assigning myself as a reminder: i have a long and sad e-mail thread with the Xaml team about this, and I need to summarize it to explain why we have so much trouble fixing this bug. 
@sweihub peddle your wares elsewhere.
This is the e-mail I sent to the Xaml folks (April 2, 2020)

## Requested Themes, a saga

Windows Terminal is built as an application hosting a Xaml Island, with its own Windows.UI.Xaml.Application composed subclass.

Terminal *also* allows its users to select a light or a dark application theme independent of the active system theme.

The most straightforward way to do this and have it apply globally to the application is to set **Application.RequestedTheme** on startup.

### Application.RequestedTheme(Dark)

Per the [documentation], it is impossible to set an application’s requested theme after it’s been initialized. This means that Terminal will have to set RequestedTheme in its App’s constructor.

This works perfectly for applications that undergo normal CoreApplication activation: the composing parent class of App is initialized, registers itself with the framework, and RequestedTheme ends up being set before initialization has completed (and it’s locked out.)

In a Xaml island, however, one of two things happen:

1.  The App is initialized before the Xaml Island boots up its CoreWindow/Application integration
    
    1.  The call to **put\_RequestedTheme** fails with an access violation.
    
    2.  This causes a crash.

2.  The App is initialized **after** Xaml Island startup
    
    1.  WindowsXamlManager creates its own FrameworkApplication
    
    2.  That application loads **App.xbf**
    
    3.  The loading of an App.xbf locks out RequestedTheme()
    
    4.  This causes an invalid argument HR/exception (depending on language)

**Conclusion**: It is impossible to set RequestedTheme in a Xaml Islands application.

## How else could we set the theme?

### ElementThemes

It is possible to set an ElementTheme on the root Page of an application. This works “alright” for Terminal, save for a couple issues ranging from minor to significant:

  - Dialogs do not live under the root Page of the application; they are presented in a different visual tree by the dialog API.
      - We try to set the ElementTheme on our dialogs, and this works:
      - Our dialogs mostly follow the ElementTheme we requested, except their *primary buttons* and *context menus* reflect the system/application theme, which we couldn’t change (#5195, #3654)
	  - ![image](https://user-images.githubusercontent.com/189190/100683824-f9a7ca00-332d-11eb-807a-7ab52a85cee5.png)
	  - ![image](https://user-images.githubusercontent.com/189190/100683856-0a584000-332e-11eb-9334-5b3959c87002.png)
	      - The primary button of the dialog is in that circle.
  - Terminal implements split panes as Grids containing Borders.
      - Those borders are either Accent or SystemChromeLow color.
      - We’re loading SystemChromeLow at runtime(1), because panes are not Xaml controls.
      - It appears that we’re unable to get the active ElementTheme version of SystemChromeLow because we’re using the Application’s resource dictionary, and that reflects the system/application theme (which we couldn’t change) (#3917)
	  - ![image](https://user-images.githubusercontent.com/189190/100683868-117f4e00-332e-11eb-9aa5-bd9b4a955618.png)
	      - To the left of the purple line should be a white line, not a gray one

1: what I wanted to ask was “is it possible for me to create a brush, or otherwise get something that will change color based on the theme of the element it’s placed inside, **at runtime**?”

 

Maybe we can fix the pane one by making it a Xaml control with visual states and theme resource dictionaries? It seemed like it would be a good idea to use visual states to indicate which pane was active and let Xaml handle switching the resources off for us anyway.

We ended up writing a Xaml fragment for the Border inside the pane, giving it theme resources and visual states, and letting it go.

*It turns out that a Border cannot move between visual states because you cannot call GoToState on something that isn’t a Control.*

We ended up putting the Grid in the Xaml fragment containing the Border as well.

*It turns out that a **Grid** cannot move between visual states because you cannot call GoToState on something that isn’t a Control.*

We ended up putting a UserControl around the entire thing, because that’s the only empty Control we could create, to house the Grid and the border. It’s now more complex than just making the controls at runtime.

*And*, after all that the issue still remains: the border has the wrong color when the application theme doesn’t match the system theme.

**Conclusion**: This is actually impossible.

[documentation]: https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.application.requestedtheme?view=winrt-19041

The response:

> So there may in fact be no answer here now for WinUI 2/XAML Islands v1.
https://github.com/microsoft/terminal/issues/12356 has a variation of this bug. That one's a hotter fix, so I went with a hack to fix it. However, the branches for that issue have a lot of history that might be valuable. 
IF ONLY WE HAD A HELPER FOR GETTING A RESOURCE OUT OF THE RESOURCES FOR A SPECIFIC THEME

Fix patch:
diff --git a/src/cascadia/TerminalApp/App.xaml b/src/cascadia/TerminalApp/App.xaml
index 24573ba6198..3da02dff31a 100644
--- a/src/cascadia/TerminalApp/App.xaml
+++ b/src/cascadia/TerminalApp/App.xaml
@@ -153,7 +153,7 @@
                                              Color="#2e2e2e" />
 
                             <StaticResource x:Key="UnfocusedBorderBrush"
-                                            ResourceKey="ApplicationPageBackgroundThemeBrush" />
+                                            ResourceKey="TabViewBackground" />
 
                             <SolidColorBrush x:Key="SettingsUiTabBrush"
                                              Color="#0c0c0c" />
@@ -169,7 +169,7 @@
                                              Color="#e8e8e8" />
 
                             <StaticResource x:Key="UnfocusedBorderBrush"
-                                            ResourceKey="ApplicationPageBackgroundThemeBrush" />
+                                            ResourceKey="TabViewBackground" />
 
                             <SolidColorBrush x:Key="SettingsUiTabBrush"
                                              Color="#ffffff" />
diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp
index b1060590d4e..0f1b115bb07 100644
--- a/src/cascadia/TerminalApp/Pane.cpp
+++ b/src/cascadia/TerminalApp/Pane.cpp
@@ -5,6 +5,8 @@
 #include "Pane.h"
 #include "AppLogic.h"
 
+#include "Utils.h"
+
 #include <Mmsystem.h>
 
 using namespace winrt::Windows::Foundation;
@@ -45,14 +47,6 @@ Pane::Pane(const Profile& profile, const TermControl& control, const bool lastFo
     _connectionStateChangedToken = _control.ConnectionStateChanged({ this, &Pane::_ControlConnectionStateChangedHandler });
     _warningBellToken = _control.WarningBell({ this, &Pane::_ControlWarningBellHandler });
 
-    // On the first Pane's creation, lookup resources we'll use to theme the
-    // Pane, including the brushed to use for the focused/unfocused border
-    // color.
-    if (s_focusedBorderBrush == nullptr || s_unfocusedBorderBrush == nullptr)
-    {
-        _SetupResources();
-    }
-
     // Register an event with the control to have it inform us when it gains focus.
     _gotFocusRevoker = _control.GotFocus(winrt::auto_revoke, { this, &Pane::_ControlGotFocusHandler });
     _lostFocusRevoker = _control.LostFocus(winrt::auto_revoke, { this, &Pane::_ControlLostFocusHandler });
@@ -3077,16 +3071,16 @@ float Pane::_ClampSplitPosition(const bool widthOrHeight, const float requestedV
 //   * The Brush we'll use for inactive Panes - TabViewBackground (to match the
 //     color of the titlebar)
 // Arguments:
-// - <none>
+// - requestedTheme: this should be the currently active Theme for the app
 // Return Value:
 // - <none>
-void Pane::_SetupResources()
+void Pane::SetupResources(const winrt::Windows::UI::Xaml::ElementTheme& requestedTheme)
 {
     const auto res = Application::Current().Resources();
     const auto accentColorKey = winrt::box_value(L"SystemAccentColor");
     if (res.HasKey(accentColorKey))
     {
-        const auto colorFromResources = res.Lookup(accentColorKey);
+        const auto colorFromResources = ThemeLookup(res, requestedTheme, accentColorKey);
         // If SystemAccentColor is _not_ a Color for some reason, use
         // Transparent as the color, so we don't do this process again on
         // the next pane (by leaving s_focusedBorderBrush nullptr)
@@ -3104,7 +3098,10 @@ void Pane::_SetupResources()
     const auto unfocusedBorderBrushKey = winrt::box_value(L"UnfocusedBorderBrush");
     if (res.HasKey(unfocusedBorderBrushKey))
     {
-        auto obj = res.Lookup(unfocusedBorderBrushKey);
+        // MAKE SURE TO USE ThemeLookup, so that we get the correct resource for
+        // the requestedTheme, not just the value from the resources (which
+        // might not respect the settings' requested theme)
+        auto obj = ThemeLookup(res, requestedTheme, unfocusedBorderBrushKey);
         s_unfocusedBorderBrush = obj.try_as<winrt::Windows::UI::Xaml::Media::SolidColorBrush>();
     }
     else
diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h
index c81bd3fd096..3f72d13e49b 100644
--- a/src/cascadia/TerminalApp/Pane.h
+++ b/src/cascadia/TerminalApp/Pane.h
@@ -136,6 +136,8 @@ class Pane : public std::enable_shared_from_this<Pane>
 
     bool ContainsReadOnly() const;
 
+    static void SetupResources(const winrt::Windows::UI::Xaml::ElementTheme& requestedTheme);
+
     // Method Description:
     // - A helper method for ad-hoc recursion on a pane tree. Walks the pane
     //   tree, calling a function on each pane in a depth-first pattern.
@@ -337,8 +339,6 @@ class Pane : public std::enable_shared_from_this<Pane>
         return false;
     }
 
-    static void _SetupResources();
-
     struct PanePoint
     {
         float x;
diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp
index 327347f5caf..564f99379df 100644
--- a/src/cascadia/TerminalApp/TerminalPage.cpp
+++ b/src/cascadia/TerminalApp/TerminalPage.cpp
@@ -4175,6 +4175,22 @@ namespace winrt::TerminalApp::implementation
         const auto theme = _settings.GlobalSettings().CurrentTheme();
         auto requestedTheme{ theme.RequestedTheme() };
 
+        {
+            // Update the brushes that Pane's use...
+            Pane::SetupResources(requestedTheme);
+            // ... then trigger a visual update for all the pane borders to
+            // apply the new ones.
+            for (const auto& tab : _tabs)
+            {
+                if (auto terminalTab{ _GetTerminalTabImpl(tab) })
+                {
+                    terminalTab->GetRootPane()->WalkTree([&](auto&& pane) {
+                        pane->UpdateVisuals();
+                    });
+                }
+            }
+        }
+
         const auto res = Application::Current().Resources();
 
         // Use our helper to lookup the theme-aware version of the resource.


Test patch: