Add ColorPickerButton

[robloo]

Add ColorPickerButton to the Toolkit

Closes #3363

PR Type

What kind of change does this PR introduce?

Feature
Documentation content changes
Sample app changes

What is the current behavior?

The toolkit, and WinUI/UWP, has no flyout ColorPicker. The color picker that currently exists is a large canvas-type control (that is incorrectly named). This existing control has lots of usability concerns discussed in #3363 and the WinUI repository.

What is the new behavior?

A brand-new control is added to allow selecting and editing a color within a flyout. The selected color is always visible in the content area of a drop-down button as a preview.

The new properties for this picker are below. Note that these are purposely name 'CustomPalette...' instead of 'Palette...' as multiple palettes may be supported in the future (windows colors, recent colors, custom palette all in different sections visible at the same time).

/// <summary>
/// Gets the list of custom palette colors.
/// </summary>
public ObservableCollection<Windows.UI.Color> CustomPaletteColors { get; }

/// <summary>
/// Gets or sets the number of colors in each row (section) of the custom color palette.
/// A section is the number of columns within an entire row in the palette.
/// Within a standard palette, rows are shades and columns are unique colors.
/// </summary>
public int CustomPaletteColumns { get; set; }

/// <summary>
/// Gets or sets a custom IColorPalette .
/// This will automatically set <see cref="CustomPaletteColors"/> and <see cref="CustomPaletteSectionCount"/> 
/// overwriting any existing values.
/// </summary>
public IColorPalette CustomPalette { get; set; }

/// <summary>
/// Gets or sets a value indicating whether the color palette is visible.
/// </summary>
public bool IsColorPaletteVisible { get; set; }

Making all of this easier is a new IColorPalette interface. Any class can implement this interface to provide a custom color palette such as windows colors (the default), RYB colors, Flat UI colors, etc. Colors could also be algorithmically generated.

/// <summary>
/// Interface to define a color palette.
/// </summary>
public interface IColorPalette
{
    /// <summary>
    /// Gets the total number of colors in this palette.
    /// A color is not necessarily a single value and may be composed of several shades.
    /// </summary>
    int ColorCount { get; }

    /// <summary>
    /// Gets the total number of shades for each color in this palette.
    /// Shades are usually a variation of the color lightening or darkening it.
    /// </summary>
    int ShadeCount { get; }

    /// <summary>
    /// Gets a color in the palette by index.
    /// </summary>
    /// <param name="colorIndex">The index of the color in the palette.
    /// The index must be between zero and <see cref="ColorCount"/>.</param>
    /// <param name="shadeIndex">The index of the color shade in the palette.
    /// The index must be between zero and <see cref="ShadeCount"/>.</param>
    /// <returns>The color at the specified index or an exception.</returns>
    Color GetColor(int colorIndex, int shadeIndex);
}

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Other information

Control

• Ensure the name ColorPickerButton is accepted
• Rename ColorPickerSlider to ColorPickerButtonSlider. ColorPickerSlider is already used by WinUI.
• Implement all accessibility functionality. Check tab ordering.
• This control originally used NumberBoxes for channel numerical input. Complete porting this functionality over to TextBoxes as the Toolkit does not have a dependency on WinUI.
• Support all visual state groups in the original ColorPicker. Connect all functionality
• Test changing all ColorPicker properties. Determine which ones make sense to ignore
• Trim displayed hex if alpha is not enabled
• Switch to radio buttons to switch between RGB/HSV representation. A ComboBox within a Flyout is generally bad practice and is also limited by some bugs in UWP (Stale issue bot #1467).
• Switch to ToggleButton instead of RadioButton for RGB/HSV representation. Radio buttons don't work in flyouts and controls well due to UWP bugs. ToggleButtons may also visually look better. The design follows a 'segmented control' concept discussed here: Grouped selection support microsoft/microsoft-ui-xaml#2310
• Switch the default color palette to be the windows colors (if possible)
• Align formatting to the toolkit conventions
• Handle CornerRadius in the template. Conditional XAML? Check how other controls do this.
• Rename 'CustomPaletteSectionCount' to 'CustomPaletteWrapCount' or 'CustomPaletteColumns' like UniformGrid
• Update selected palette color data template to provide correct contrast of the border with the selected color to match with windows settings app (check mark overlay) The check mark is not included as the size of swatches in the palette could be smaller than the check mark itself.
• Rename 'CustomPaletteColumns' to 'CustomPaletteColumnCount'
• Improve spacing above/below channel sliders -- try 24px (2 x 12px)
• Do not show the color (set the preview border visibility to collapsed) while the control is initializing. This prevents the default color from briefly appearing during initial load. Instead, the background will just show through.
• Do not change the saturation and value in the hue slider background. Keep SV at maximum. This allows the hue to always be clear to the user and is in line with other implementations (like in Inkscape).
• Rename WindowsColorPalette to FluentColorPalette
• Show palette color display name in a tooltip
• Use the WinUI algorithm for light/dark color switching
• Each tab should have independent visibility using IsColorPaletteVisible (new), IsColorSpectrumVisible (existing) and IsColorChannelTextInputVisible (repurposed to mean the entire tab). The tabs still visible should take up available space remaining. With only one tab visible the tab header should disappear.

Code Review

1. The mini-palette and color preview will be all one color when the selected color is black or white. The calculated accent colors will not change and pressing them will not change the selected color.
   • Requires feedback from Microsoft on how the accent colors are calculated in Windows
   • It is possible to specially handle when the color is as max/min value (black or white) at least the two accent colors to the left/right could be different shades of gray.
2. The mini-palette accent colors are destructive. Pressing back and forward by 1 will not restore the original color.
   • Again, requires feedback from Microsoft on how the accent colors are calculated in Windows.
   • Due to perceptual adjustments it's likely this will always be destructive.
3. The third dimension slider in the spectrum tab will vary the other dimensions in the spectrum. This results in the spectrum cursor jumping around as the slider value changes. This might be related to HSV/RGB conversion but needs to be investigated.
4. Adjust the design of the sample app page to move the ColorPickerButton closer to the description.
5. Check if a ColorToDisplayName converter already exists. Consider adding control specific converters to their own sub-namespace.
6. The ColorPickerButton will derive from DropDownButton and have a ColorPicker property. Dependent on [Discussion] Bump Minimum Version to 1809 #3440
7. The ColorPicker will have the same new style as the ColorPickerButton flyout. While it's a new concept to have tabs in a panel-type control, this is considered acceptable.
8. Merge @michael-hawker branch into this one when given the thumb's up. Both controls will be in the same directory.
9. Remove conditional XAML for corner radius after WinUI 3.0. Conditional XAML doesn't work correctly in resource dictionaries and with setters. See Conditional XAML not working in ResourceDictionaries microsoft/microsoft-ui-xaml#2556, fixed with [Discussion] Bump Minimum Version to 1809 #3440
10. All slider rendering will be broken out and put into a new ColorPickerSlider. This should simplify code-behind and the default template.
11. The Pivot tab width and show/hide of tabs will be controlled in code-behind. use of the pivot itself is somewhat dependent on issues with touch. The pivot, or switching to a new control, will not hold the release.
12. Sliders should vary only 1-channel and keep the other channels at maximum. This ensures the gradient always makes sense and never becomes fully black/white/transparent. This could also be made a configuration of the ColorPickerSlider control. Edit: This does need to be configurable as it's only required in HSV mode.

Other

• Complete integration with the sample app. Add XAML to dynamically change the control.
• Complete documentation of the control

Future

These changes should be considered for future versions of the control. They are recorded here for lack of a better location.

• The mini selected color palette may follow the Windows accent color lighter/darker shades algorithm. However, the implementation of this algorithm is currently unknown. This algorithm may be in the XAML fluent theme editor: https://github.com/microsoft/fluent-xaml-theme-editor
• Switch back to NumberBox for all color channel numerical input. This will require WinUI 3.0 and the corresponding release of the toolkit. When Proposal: Support Hex Display and Input in NumberBox microsoft/microsoft-ui-xaml#2757 is implemented, also use NumberBox for hex color input.
• Remove the custom 'ColorPickerButtonSlider' after switch to WinUI 3.0. This assumes there is no additional control-specific code at that point. (It currently exists just to work-around UWP bugs). This should be turned into it's own full primitive control.
• Update ColorSpectrum_GotFocus event handler (instructions are within code) after/if ColorSpectrum bugs are fixed (likely after WinUI 3.0
• Use Color.IsEmpty if Proposal: Add Color.IsEmpty and Color.Empty microsoft/microsoft-ui-xaml#2826 is implemented
• Ensure PivotItems are properly removed once/if Setting PivotItem.Visiblity to Collapsed Does not Remove It from Pivot microsoft/microsoft-ui-xaml#2952 is closed.
• Add color history palette to the sample app
• Move localizable strings into resources
• Treat negative/zero numbers in CustomPaletteColumnCount as 'auto' and automatically calculate a good column count to keep rows/columns even. Set the default of this property to 0 'auto'.

[ghost]

Thanks robloo for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[net-foundation-cla]

All CLA requirements met.

[net-foundation-cla]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ robloo sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Kyaa-dost]

@robloo Thanks for the PR. Whenever you get a chance if you could sign the license/cla above that would be great😄

[robloo]

@Kyaa-dost

I'm getting a lot of formatting errors. In my opinion the settings are too strict and I would like to override them. For example, I have the following code:

Errors are:

• Code should not contain multiple whitespace characterss in a row
• Statement should not be on a single line

If I break these things up it's going to make the code a lot more difficult to read. The null checks all over the place are required in a control and making them inline is easier to read. Concerning whitespace, I strongly disagree with this requirement as aligning code like this is a pretty standard practice for readability... at least in the embedded world.

[Kyaa-dost]

@Kyaa-dost

I'm getting a lot of formatting errors. In my opinion the settings are too strict and I would like to override them. For example, I have the following code:

Errors are:

• Code should not contain multiple whitespace characterss in a row
• Statement should not be on a single line

If I break these things up it's going to make the code a lot more difficult to read. The null checks all over the place are required in a control and making them inline is easier to read. Concerning whitespace, I strongly disagree with this requirement as aligning code like this is a pretty standard practice for readability... at least in the embedded world.

Thanks for the detailed info and clarification.
Looping in @michael-hawker here

[robloo]

@Kyaa-dost @michael-hawker This is largely implemented now in the toolkit. I don't have all the visual states and properties hooked up but your feedback on overall look of the control would be useful now before all of that is done. (See ToDo list in PR text)

I have added this control to the SampleApp so it is easy to test.

[robloo]

I ran across a bug I would like help diagnosing. These types of bugs can take a while so wonder if anyone has an idea if the problem is with UWP. Maybe it's just too late in the evening :)

In OnApplyTemplate() I'm setting the color representation (RGB/HSV) RadioButton's IsChecked value by calling SetActiveColorRepresentation(). On the first ColorPickerButton loaded in the visual tree it works fine. For all other ColorPickerButton's loaded in the tree the radio button for RGB/HSV is never set. Clicking the radio buttons after initial load everything works fine.

Looking at what is going on inside SetActiveColorRepresentation(), I see that when I call this.RgbRadioButton.IsChecked = true; it's raising the ColorRepRadioButton_CheckedUnchecked event which is calling SetActiveColorRepresentation() again -- but this only occurs once and not an infinite number of times. The interesting thing is I explicitly remove the event handlers to avoid this happening but it happens anyway. I know UWP has re-entracy problems if you try to modify IsChecked within the Checked event handler -- I really do miss WPF sometimes -- however, this even handling code should never be run for this to happen. Any ideas?

[robloo]

It appears there are a few UWP bugs making radio buttons unusable with implicit groups while in controls and especially with flyouts.

microsoft/microsoft-ui-xaml#1299
microsoft/microsoft-ui-xaml#2081

[robloo]

@michael-hawker I merged your branch and closed several of the open items. Several updates were made splitting out the ColorPickerSlider as a separate control. Although more work is required to remove them as template parts, what was done includes:

• Switches the sliders to work entirely within HSV which means its background stops turning red when you get to a minimum value. That should avoid some early issue reports :)
• Exposes properties to fix the Saturation/Value and Alpha channels at maximum when calculating backgrounds. This ensures the background is always visible and aligns with other ColorPickers. It can be disabled to go back to previous (mathematically correct) functionality though. Hopefully this avoids some issue reports as well after release.
• Because each slider background is no longer consistent with the others (since some channel values can now be fixed at max), the slider thumb foreground color has to calculated separately for each slider. This means some thumbs can be white while others are black whereas before they were all the same since the selected color below the thumb was the same in all sliders.
• Moves related slider XAML into separate file
• Moves slider into Primitives namespace

The only new issue I noticed is keyboard navigation doesn't work to change tabs. The new switch presenter is very nice though. In the future it would also be nice to auto-size the tabs to fit available width when some tabs are hidden.

[michael-hawker]

@robloo I was able to navigate the tabs with the keyboard (it's just a listbox for the header). Not sure if the tab order for everything is set, so maybe that's a problem? I did notice we should edit the navigation on the palette grid as it doesn't let you use the arrow keys to move in the actual grid, that may just be a property we can set. We can do some fixes for this after.

[michael-hawker]

Looking good, let's get this in to the next preview and collect some community feedback. 🎉

[ghost]

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[Kyaa-dost]

🚀 🚀