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

Impossible to set custom tab color after setting it to black #7948

Closed
ItsMeSousuke opened this issue Oct 16, 2020 · 15 comments · Fixed by #7963
Closed

Impossible to set custom tab color after setting it to black #7948

ItsMeSousuke opened this issue Oct 16, 2020 · 15 comments · Fixed by #7963
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@ItsMeSousuke
Copy link

ItsMeSousuke commented Oct 16, 2020

Environment

Windows build number: Win32NT, 10.0.18362.0 Microsoft Windows NT 10.0.18362.0
Windows Terminal version (if applicable): 1.3.2651.0

Steps to reproduce

  1. Right click on tab and select Color...
  2. Click Custom
  3. Click More
  4. Remove value from hex code field, tab background is set to black
  5. Try to pick any color from color picker

Expected behavior

User should be able to pick new color from color picker.

Actual behavior

Tab color is black and can't be change by picking color from color picker, it can be only change by selecting one from predefined list or by setting any other color hex code other than black
image

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 16, 2020
@ghost
Copy link

ghost commented Oct 16, 2020

I can repro it:

image

@ghost
Copy link

ghost commented Oct 17, 2020

I think this is an issue with the XAML ColorPicker control. Because I was able to reproduce this issue in XAML Controls Gallery

image

And a little more closer look:

image

@ghost
Copy link

ghost commented Oct 17, 2020

This is what I think is the reason why this is not working

Look at this

Look at the slider. It is set to black. So if you change the color using that rainbow like thing, that slider is still at black.
You have to move the slider in order for it to work (and I have confirmed it).

This also explains why the color changes successfully when you choose a preset color or you give another hex value.

The funny thing is that Windows Terminal does not have this slider

This is probably because

The one with the slider:

image

The one that Windows Terminal be likely be using:

image

@ghost
Copy link

ghost commented Oct 17, 2020

cc @DHowett @zadjii-msft

@ghost
Copy link

ghost commented Oct 19, 2020

GIFing this problem:

(Sorry for my extremely bad GIFs. Especially the ColorPicker control 🤢)

The problem:

The problem

Demonstrating the problem and a possible fix in XAML Studio:

potential-fix

I think we can fix this by enabling Color Slider in the ColorPicker control.

I would like to make a PR on this if you agree on this fix (only if this fix is going to be as easy as I did in XAML Studio)

@WSLUser
Copy link
Contributor

WSLUser commented Oct 19, 2020

Is there an existing issue in https://github.com/microsoft/microsoft-ui-xaml/issues? This seems like a good workaround to add but ultimately this is a bug with the XAML framework and should be addressed by the WINUI team.

@zadjii-msft
Copy link
Member

Yea, I think it's fine to fix locally for the Terminal with enabling the color slider, but I agree, we should file a follow-up on the WinUI repo that WSLUser linked.

@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 19, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 19, 2020
@zadjii-msft zadjii-msft added good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Needs-Tag-Fix Doesn't match tag requirements labels Oct 19, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Oct 19, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Oct 19, 2020
@ghost
Copy link

ghost commented Oct 19, 2020

@zadjii-msft I will take up this issue

@ghost
Copy link

ghost commented Oct 19, 2020

@zadjii-msft
Just point me to the right XAML file.

@zadjii-msft zadjii-msft assigned ghost Oct 19, 2020
@ghost ghost mentioned this issue Oct 19, 2020
6 tasks
@ghost ghost added the In-PR This issue has a related PR label Oct 19, 2020
@ghost
Copy link

ghost commented Oct 19, 2020

@zadjii-msft

PR ready to be reviewed -> #7963

@WSLUser
Copy link
Contributor

WSLUser commented Oct 19, 2020

So I did check and there isn't an existing issue on the WinUI repo. Please create a ticket and link to it here. The PR should be considered a workaround, not a "fix" and so this issue should be closed when we pick up a version of MUX with the fix. We should add the upstream-issue and workaround-available tags for tracking/visibility.

@ghost ghost removed the In-PR This issue has a related PR label Oct 19, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 19, 2020
@ghost ghost added the In-PR This issue has a related PR label Oct 20, 2020
@ghost ghost closed this as completed in #7963 Oct 23, 2020
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Oct 23, 2020
ghost pushed a commit that referenced this issue Oct 23, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
Adds the color slider to the tab color picker

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #7948 
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] ~Tests added/passed~
* [ ] ~Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx~
* [ ] ~Schema updated.~
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #7948 

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->


<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

*Not required*
@zadjii-msft
Copy link
Member

I've filed microsoft/microsoft-ui-xaml#3476 upstream, but I'm okay leaving this issue closed, because frankly, the picker is better off with the color slider visible. That makes it possible to pick both a lightness and a hue, which greatly increases the range of easily accessible tab colors.

Thanks for the investigation @alannt777!

DHowett pushed a commit that referenced this issue Oct 27, 2020
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
Adds the color slider to the tab color picker

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #7948
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] ~Tests added/passed~
* [ ] ~Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx~
* [ ] ~Schema updated.~
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #7948

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

*Not required*

(cherry picked from commit 4a95d94)
@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7963, which has now been successfully released as Windows Terminal v1.4.3141.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Nov 11, 2020

🎉This issue was addressed in #7963, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal good first issue This is a fix that might be easier for someone to do as a first contribution Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants