Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ColorPicker Bottom Shade Adjuster Resets Color to White #4208

Open
1 of 2 tasks
zateutsch opened this issue Aug 31, 2021 · 5 comments · May be fixed by #4502
Open
1 of 2 tasks

ColorPicker Bottom Shade Adjuster Resets Color to White #4208

zateutsch opened this issue Aug 31, 2021 · 5 comments · May be fixed by #4502
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ controls 🎛️ In-PR 🚀
Milestone

Comments

@zateutsch
Copy link
Contributor

Describe the bug

Using the bottom shade adjuster while on the darkest shade resets the selected color to white.

  • Is this bug a regression in the toolkit? If so, what toolkit version did you last see it work:

Steps to Reproduce

  • Can this be reproduced in the Sample App? (Either in a sample as-is or with new XAML pasted in the editor.) If so, please provide custom XAML or steps to reproduce. If not, let us know why it can't be reproduced (e.g. more complex setup, environment, dependencies, etc...)

Steps to reproduce the behavior:

  1. Reproduced in the Sample App as-is
  2. Using box-shaped spectrum color picker, select any color on the spectrum
  3. Lower shade value to darkest shade using either bottom or left shade control
  4. Update to lighter shade value using bottom control.
  5. Color resets to white.

Expected behavior

Updating to lighter shade using the bottom control maintains the latest used color, instead of reverting to white.

Screenshots

image
image
image

@zateutsch zateutsch added the bug 🐛 An unexpected issue that highlights incorrect behavior label Aug 31, 2021
@ghost ghost added the needs triage 🔍 label Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

Hello zateutsch, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@michael-hawker
Copy link
Member

Thanks @zateutsch, I had mentioned this to @robloo before during the initial review. I don't think the memory tracking is on his feature board in #3643, so good to have an issue tracking it. Putting out to next milestone for now.

@robloo
Copy link
Contributor

robloo commented Sep 1, 2021

Yea, this was called out before. Thanks for pointing this out in real-world usage.

I did initially think this needs to be fixed by remembering the last color before white. However, just operating in HSV should be enough (and a lot easier / more elegant). The Hue doesn't change in the bottom preview and accent colors area (it behaves just like a Value-channel slider with 5 discrete steps). This means the information is preserved if only HSV was being used instead of RBG.

I will have to change the converter to operate in HSV and this could be a larger tear up in this area. But all this information is internal to the ColorPicker anyway.

Bottom line: It should be fixable without remembering the color: Just switching from RGB to HSV color representation should be enough.

@robloo
Copy link
Contributor

robloo commented Feb 27, 2022

This was fixed in an internal version of the control.

  1. A new ColorPreviewer control was added to handle the bottom color preview area as well as the accent colors (this helps clean up and simplify ColorPicker code while making feature additions simpler)
  2. As mentioned above, the ColorPreviewer now works in HSVA color representation instead of RGBA. That preserves Hue information even in min/max Saturation. In practice that means the slider never gets reset to white/black and you can always step back into the color hue selected in the spectrum.

It's only a matter of copying over the code so it shouldn't be too much trouble to fix.

@robloo robloo linked a pull request Mar 5, 2022 that will close this issue
13 tasks
@ghost ghost added the In-PR 🚀 label Mar 5, 2022
@robloo
Copy link
Contributor

robloo commented Mar 5, 2022

Fixed in #4502

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ controls 🎛️ In-PR 🚀
Projects
Features 8.0
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants