From bb0e1d3979ef60aa7e925f31a522c587d27ea9a4 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 19 Feb 2021 10:20:04 -0800 Subject: [PATCH] Redesign color schemes page (#9196) This PR performs a large overall polish of the color schemes page: - Ensures keyboard navigation is holistically improved (i.e. fully accessible, no lost focus, etc...) - Adds tooltips and automation properties to all controls - Redesigns the page according to @mdtauk's approved design ([link](https://github.com/microsoft/terminal/pull/8997#issuecomment-771623842)). Note, there are some minor modifications to the design that were approved by @cinnamon-msft. - Automatically reflow's the color buttons when they do not fit in horizontal mode ## Detailed Description of the Pull Request / Additional comments - Redesign - a data template was introduced to make color representation consistent and straightforward - `ContentControl` is used to hold a reference to the `ColorTableEntry` and represent it properly using the aforementioned data template. - The design is mainly a StackPanel holding two grids: color table & functional colors. - The color table is populated via code. After much thought, this seems to be the easiest way to correctly bind 16 controls that are very similar. - The functional colors are populated via XAML manually. - We need a grid to separate the text and the buttons. This allows for scenarios like "selection background is the longest string" to force the buttons and text to be aligned. - Reflow - A `VisualStateManager` uses an `AdaptiveTrigger` to change the orientation of the color tables' stack panel. The adaptive trigger was carefully calculated to align with the navigation view's breakpoint. - Keyboard Navigation - (a lesson from `SettingContainer`) `ContentControl` can be focused as opposed to the content inside of it. We don't want that, so we set `IsTabStop` to false on it. That basically fixes all of our keyboard navigation issues in this new design. - Automation Properties and ToolTips - As in my previous PRs, I can't seem to figure out how to bind to a control's automation property to its own tooltip. So I had to do this in the code and add a few `x:Name` around. ## Validation Steps Performed - Manually tested... - tab navigation - accessibility insights - NVDA - changing color schemes updates color table - specific scenario: - change a color table color and a functional color - navigate to a different color scheme - navigate back to the first color scheme - if the colors persist, the changes were propagated to the settings model References #8997 - Based on the work from @Chips1234 References #6800 - Settings UI Epic Closes #8765 - Polish Color Schemes page Closes #8899 - Automation Properties Closes #8768 - Keyboard Navigation --- .../TerminalSettingsEditor/ColorSchemes.cpp | 142 ++++++- .../TerminalSettingsEditor/ColorSchemes.h | 12 +- .../TerminalSettingsEditor/ColorSchemes.idl | 15 +- .../TerminalSettingsEditor/ColorSchemes.xaml | 361 ++++++++---------- .../Resources/en-US/Resources.resw | 62 +-- 5 files changed, 353 insertions(+), 239 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp b/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp index 4aa21a1062f..01325767f02 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp @@ -20,6 +20,14 @@ using namespace winrt::Windows::Foundation::Collections; namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { + // The first 8 entries of the color table are non-bright colors, whereas the rest are bright. + static constexpr uint8_t ColorTableDivider{ 8 }; + + static constexpr std::wstring_view ForegroundColorTag{ L"Foreground" }; + static constexpr std::wstring_view BackgroundColorTag{ L"Background" }; + static constexpr std::wstring_view CursorColorTag{ L"CursorColor" }; + static constexpr std::wstring_view SelectionBackgroundColorTag{ L"SelectionBackground" }; + static const std::array TableColorNames = { RS_(L"ColorScheme_Black/Header"), RS_(L"ColorScheme_Red/Header"), @@ -53,9 +61,25 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation ColorSchemes::ColorSchemes() : _ColorSchemeList{ single_threaded_observable_vector() }, - _CurrentColorTable{ single_threaded_observable_vector() } + _CurrentNonBrightColorTable{ single_threaded_observable_vector() }, + _CurrentBrightColorTable{ single_threaded_observable_vector() } { InitializeComponent(); + + Automation::AutomationProperties::SetName(ColorSchemeComboBox(), RS_(L"ColorScheme_Name/Header")); + Automation::AutomationProperties::SetFullDescription(ColorSchemeComboBox(), RS_(L"ColorScheme_Name/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip")); + ToolTipService::SetToolTip(ColorSchemeComboBox(), box_value(RS_(L"ColorScheme_Name/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip"))); + + Automation::AutomationProperties::SetName(RenameButton(), RS_(L"Rename/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip")); + + Automation::AutomationProperties::SetName(NameBox(), RS_(L"ColorScheme_Name/Header")); + Automation::AutomationProperties::SetFullDescription(NameBox(), RS_(L"ColorScheme_Name/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip")); + ToolTipService::SetToolTip(NameBox(), box_value(RS_(L"ColorScheme_Name/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip"))); + + Automation::AutomationProperties::SetName(RenameAcceptButton(), RS_(L"RenameAccept/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip")); + Automation::AutomationProperties::SetName(RenameCancelButton(), RS_(L"RenameCancel/[using:Windows.UI.Xaml.Controls]ToolTipService/ToolTip")); + Automation::AutomationProperties::SetName(AddNewButton(), RS_(L"ColorScheme_AddNewButton/Text")); + Automation::AutomationProperties::SetName(DeleteButton(), RS_(L"ColorScheme_DeleteButton/Text")); } void ColorSchemes::OnNavigatedTo(const NavigationEventArgs& e) @@ -70,9 +94,20 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // very accurately. for (uint8_t i = 0; i < TableColorNames.size(); ++i) { - auto entry = winrt::make(i, Windows::UI::Color{ 0, 0, 0, 0 }); - _CurrentColorTable.Append(entry); + const auto& entry{ winrt::make(i, Windows::UI::Color{ 0, 0, 0, 0 }) }; + if (i < ColorTableDivider) + { + _CurrentNonBrightColorTable.Append(entry); + } + else + { + _CurrentBrightColorTable.Append(entry); + } } + _CurrentForegroundColor = winrt::make(ForegroundColorTag, Windows::UI::Color{ 0, 0, 0, 0 }); + _CurrentBackgroundColor = winrt::make(BackgroundColorTag, Windows::UI::Color{ 0, 0, 0, 0 }); + _CurrentCursorColor = winrt::make(CursorColorTag, Windows::UI::Color{ 0, 0, 0, 0 }); + _CurrentSelectionBackgroundColor = winrt::make(SelectionBackgroundColorTag, Windows::UI::Color{ 0, 0, 0, 0 }); // Try to look up the scheme that was navigated to. If we find it, immediately select it. const std::wstring lastNameFromNav{ _State.LastSelectedScheme() }; @@ -85,6 +120,41 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation auto scheme = *it; ColorSchemeComboBox().SelectedItem(scheme); } + + // populate color table grid + const auto colorLabelStyle{ Resources().Lookup(winrt::box_value(L"ColorLabelStyle")).as() }; + const auto colorControlStyle{ Resources().Lookup(winrt::box_value(L"ColorControlStyle")).as() }; + const auto colorTableEntryTemplate{ Resources().Lookup(winrt::box_value(L"ColorTableEntryTemplate")).as() }; + auto setupColorControl = [colorTableEntryTemplate, colorControlStyle, colorTableGrid{ ColorTableGrid() }](const auto&& colorRef, const uint32_t& row, const uint32_t& col) { + ContentControl colorControl{}; + colorControl.ContentTemplate(colorTableEntryTemplate); + colorControl.Style(colorControlStyle); + + Data::Binding binding{}; + binding.Source(colorRef); + binding.Mode(Data::BindingMode::TwoWay); + colorControl.SetBinding(ContentControl::ContentProperty(), binding); + + colorTableGrid.Children().Append(colorControl); + Grid::SetRow(colorControl, row); + Grid::SetColumn(colorControl, col); + }; + for (uint32_t row = 0; row < ColorTableGrid().RowDefinitions().Size(); ++row) + { + // color label + TextBlock label{}; + label.Text(TableColorNames[row]); + label.Style(colorLabelStyle); + ColorTableGrid().Children().Append(label); + Grid::SetRow(label, row); + Grid::SetColumn(label, 0); + + // regular color + setupColorControl(_CurrentNonBrightColorTable.GetAt(row), row, 1); + + // bright color + setupColorControl(_CurrentBrightColorTable.GetAt(row), row, 2); + } } // Function Description: @@ -149,20 +219,52 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void ColorSchemes::ColorPickerChanged(IInspectable const& sender, ColorChangedEventArgs const& args) { - if (auto picker = sender.try_as()) + if (const auto& picker{ sender.try_as() }) { - if (auto tag = picker.Tag()) + if (const auto& tag{ picker.Tag() }) { - auto index = winrt::unbox_value(tag); - CurrentColorScheme().SetColorTableEntry(index, args.NewColor()); - _CurrentColorTable.GetAt(index).Color(args.NewColor()); + if (const auto index{ tag.try_as() }) + { + CurrentColorScheme().SetColorTableEntry(*index, args.NewColor()); + if (index < ColorTableDivider) + { + _CurrentNonBrightColorTable.GetAt(*index).Color(args.NewColor()); + } + else + { + _CurrentBrightColorTable.GetAt(*index - ColorTableDivider).Color(args.NewColor()); + } + } + else if (const auto stringTag{ tag.try_as() }) + { + if (stringTag == ForegroundColorTag) + { + CurrentColorScheme().Foreground(args.NewColor()); + _CurrentForegroundColor.Color(args.NewColor()); + } + else if (stringTag == BackgroundColorTag) + { + CurrentColorScheme().Background(args.NewColor()); + _CurrentBackgroundColor.Color(args.NewColor()); + } + else if (stringTag == CursorColorTag) + { + CurrentColorScheme().CursorColor(args.NewColor()); + _CurrentCursorColor.Color(args.NewColor()); + } + else if (stringTag == SelectionBackgroundColorTag) + { + CurrentColorScheme().SelectionBackground(args.NewColor()); + _CurrentSelectionBackgroundColor.Color(args.NewColor()); + } + } } } } bool ColorSchemes::CanDeleteCurrentScheme() const { - if (const auto scheme{ CurrentColorScheme() }) + if (const auto& scheme{ CurrentColorScheme() }) { // Only allow this color scheme to be deleted if it's not provided in-box const std::wstring myName{ scheme.Name() }; @@ -295,14 +397,32 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { for (uint8_t i = 0; i < TableColorNames.size(); ++i) { - _CurrentColorTable.GetAt(i).Color(colorScheme.Table()[i]); + if (i < ColorTableDivider) + { + _CurrentNonBrightColorTable.GetAt(i).Color(colorScheme.Table()[i]); + } + else + { + _CurrentBrightColorTable.GetAt(i - ColorTableDivider).Color(colorScheme.Table()[i]); + } } + _CurrentForegroundColor.Color(colorScheme.Foreground()); + _CurrentBackgroundColor.Color(colorScheme.Background()); + _CurrentCursorColor.Color(colorScheme.CursorColor()); + _CurrentSelectionBackgroundColor.Color(colorScheme.SelectionBackground()); } ColorTableEntry::ColorTableEntry(uint8_t index, Windows::UI::Color color) { Name(TableColorNames[index]); - Index(winrt::box_value(index)); + Tag(winrt::box_value(index)); + Color(color); + } + + ColorTableEntry::ColorTableEntry(std::wstring_view tag, Windows::UI::Color color) + { + Name(LocalizedNameForEnumName(L"ColorScheme_", tag, L"Text")); + Tag(winrt::box_value(tag)); Color(color); } } diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.h b/src/cascadia/TerminalSettingsEditor/ColorSchemes.h index 6f053e99591..e46d80b05b9 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.h +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.h @@ -39,12 +39,17 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void DeleteConfirmation_Click(winrt::Windows::Foundation::IInspectable const& sender, winrt::Windows::UI::Xaml::RoutedEventArgs const& e); GETSET_PROPERTY(Editor::ColorSchemesPageNavigationState, State, nullptr); - GETSET_PROPERTY(Windows::Foundation::Collections::IObservableVector, CurrentColorTable, nullptr); + GETSET_PROPERTY(Model::ColorScheme, CurrentColorScheme, nullptr); + GETSET_PROPERTY(Windows::Foundation::Collections::IVector, CurrentNonBrightColorTable, nullptr); + GETSET_PROPERTY(Windows::Foundation::Collections::IVector, CurrentBrightColorTable, nullptr); GETSET_PROPERTY(Windows::Foundation::Collections::IObservableVector, ColorSchemeList, nullptr); WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); - OBSERVABLE_GETSET_PROPERTY(winrt::Microsoft::Terminal::Settings::Model::ColorScheme, CurrentColorScheme, _PropertyChangedHandlers, nullptr); OBSERVABLE_GETSET_PROPERTY(bool, IsRenaming, _PropertyChangedHandlers, nullptr); + OBSERVABLE_GETSET_PROPERTY(Editor::ColorTableEntry, CurrentForegroundColor, _PropertyChangedHandlers, nullptr); + OBSERVABLE_GETSET_PROPERTY(Editor::ColorTableEntry, CurrentBackgroundColor, _PropertyChangedHandlers, nullptr); + OBSERVABLE_GETSET_PROPERTY(Editor::ColorTableEntry, CurrentCursorColor, _PropertyChangedHandlers, nullptr); + OBSERVABLE_GETSET_PROPERTY(Editor::ColorTableEntry, CurrentSelectionBackgroundColor, _PropertyChangedHandlers, nullptr); private: void _UpdateColorTable(const winrt::Microsoft::Terminal::Settings::Model::ColorScheme& colorScheme); @@ -56,10 +61,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { public: ColorTableEntry(uint8_t index, Windows::UI::Color color); + ColorTableEntry(std::wstring_view tag, Windows::UI::Color color); WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); OBSERVABLE_GETSET_PROPERTY(winrt::hstring, Name, _PropertyChangedHandlers); - OBSERVABLE_GETSET_PROPERTY(IInspectable, Index, _PropertyChangedHandlers); + OBSERVABLE_GETSET_PROPERTY(IInspectable, Tag, _PropertyChangedHandlers); OBSERVABLE_GETSET_PROPERTY(Windows::UI::Color, Color, _PropertyChangedHandlers); }; } diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl b/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl index 21c80c2b466..c446502ce4b 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.idl @@ -18,15 +18,24 @@ namespace Microsoft.Terminal.Settings.Editor Boolean CanDeleteCurrentScheme { get; }; Boolean IsRenaming { get; }; - Microsoft.Terminal.Settings.Model.ColorScheme CurrentColorScheme { get; }; - Windows.Foundation.Collections.IObservableVector CurrentColorTable; + // Terminal Colors + Windows.Foundation.Collections.IVector CurrentNonBrightColorTable { get; }; + Windows.Foundation.Collections.IVector CurrentBrightColorTable { get; }; + + // System Colors + ColorTableEntry CurrentForegroundColor; + ColorTableEntry CurrentBackgroundColor; + ColorTableEntry CurrentCursorColor; + ColorTableEntry CurrentSelectionBackgroundColor; + + Windows.Foundation.Collections.IObservableVector ColorSchemeList { get; }; } [default_interface] runtimeclass ColorTableEntry : Windows.UI.Xaml.Data.INotifyPropertyChanged { String Name { get; }; - IInspectable Index; + IInspectable Tag; Windows.UI.Color Color; } } diff --git a/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml b/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml index f8b76846358..df291904742 100644 --- a/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml +++ b/src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml @@ -17,18 +17,25 @@ the MIT License. See LICENSE in the project root for license information. --> - 0,0,10,20 + + + - - - + + + + @@ -51,22 +88,27 @@ the MIT License. See LICENSE in the project root for license information. --> - - - - - - - - - - + + + + + + + + + + + + + + + + - + @@ -111,6 +153,7 @@ the MIT License. See LICENSE in the project root for license information. --> - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - + - +