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

Update colors of our custom NewTab button to match MUX's TabView #6812

Merged
merged 1 commit into from
Jul 7, 2020

Conversation

jtippet
Copy link
Contributor

@jtippet jtippet commented Jul 7, 2020

Update colors of our custom NewTab button to match MUX's TabView button

MUX has a NewTab button, but Terminal uses a homemade lookalike. The
version in Terminal doesn't use the same brush color resources as MUX's
button, so it looks very slightly different. This PR updates Terminal's
button to use the exact same colors that MUX uses. I literally copied
these brush names out of MUX source code.

References

This is the color version of the layout fix #6766
This is a prerequisite for fixing #5360

PR Checklist

Detailed Description of the Pull Request / Additional comments

The real reason that this matters is that once you flip on
ApplicationHighContrastAdjustment::None, the existing colors will not
work at all. The existing brushes are themed to black foreground on a
black background when High Contrast (HC) Black theme is enabled. The
only thing that's saving you is
ApplicationHighContrastAdjustment::Auto is automatically backplating
the glyphs on the buttons, which (by design) hides the fact that the
colors are poor. The backplates are those ugly squares inside the
buttons on the HC themes.

Before I can push a PR that disables automatic backplating (set
ApplicationHighContrastAdjustment to None), we'll need to select
better brushes that work in HC mode. MUX has already selected brushes
that work great in all modes, so it just makes sense to use their
brushes.

If you want to hear some unsubstantiated speculation... read on. I
speculate that when this Terminal code was originally written, the MUX
code was incomplete, so it wasn't possible to copy all of MUX's brushes.
It also may not have been obvious that MUX uses an inconsistent mix of
TabViewButtonXxx and TabViewItemHeaderXxx brushes for this widget, since
the latter is hidden in some styling override code.

Validation Steps Performed

Screenshots! Everybody loves screenshots! These show the button in its
hover state (center button) and normal state (right button).

Theme Old New
Light oldlight newlight
Dark olddark newdark
HC White oldwhite newwhite
HC Black oldblack newblack

The one very subtle difference here is that, for non-HC themes, the
glyph's foreground has a bit more contrast when the button is in
hovered/pressed states. Again this slight difference hardly matters
now, but using the correct brushes will become critical when we try to
remove the HC backplating.

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Jul 7, 2020
@mimi89999
Copy link

I see that the + button will have a bigger contrast that other buttons. Won't it attract more attention than other buttons? I think that it could be distracting. Could you show a screenshot of the full window? Also, how does it look with ApplicationHighContrastAdjustment::None?

@jtippet
Copy link
Contributor Author

jtippet commented Jul 7, 2020

I see that the + button will have a bigger contrast that other buttons. Won't it attract more attention than other buttons? I think that it could be distracting. Could you show a screenshot of the full window?

That's only when the mouse is over it. The screenshot tool I used above doesn't include the mouse cursor. Here's an example with the mouse cursor:

newwhite

It looks fine in actual usage.

Also, how does it look with ApplicationHighContrastAdjustment::None?

This is the same as original screenshots, but also with AHCA::None:

image

Note again that the mouse is actually hovering over the center button, even though my screenshot tool didn't show it.

@DHowett
Copy link
Member

DHowett commented Jul 7, 2020

Thanks so much for doing this.

@DHowett
Copy link
Member

DHowett commented Jul 7, 2020

And, yes, your hunch is correct. We also knew relatively little about XAML when we started. 😄

@DHowett DHowett changed the title Update colors of our custom NewTab button to match MUX's TabView button Update colors of our custom NewTab button to match MUX's TabView Jul 7, 2020
@DHowett
Copy link
Member

DHowett commented Jul 7, 2020

@maftbot merge this in 1 minute

@DHowett DHowett merged commit 4faa104 into microsoft:master Jul 7, 2020
@jtippet jtippet deleted the user/jtippet/newtab_color branch July 7, 2020 20:40
DHowett pushed a commit that referenced this pull request Jul 17, 2020
Update colors of our custom NewTab button to match MUX's TabView button

MUX has a NewTab button, but Terminal uses a homemade lookalike.  The
version in Terminal doesn't use the same brush color resources as MUX's
button, so it looks very slightly different.  This PR updates Terminal's
button to use the exact same colors that MUX uses.  I literally copied
these brush names out of MUX source code.

## References
This is the color version of the layout fix #6766
This is a prerequisite for fixing #5360

## Detailed Description of the Pull Request / Additional comments
The real reason that this matters is that once you flip on
`ApplicationHighContrastAdjustment::None`, the existing colors will not
work at all.  The existing brushes are themed to black foreground on a
black background when High Contrast (HC) Black theme is enabled.  The
only thing that's saving you is
`ApplicationHighContrastAdjustment::Auto` is automatically backplating
the glyphs on the buttons, which (by design) hides the fact that the
colors are poor.  The backplates are those ugly squares inside the
buttons on the HC themes.

Before I can push a PR that disables automatic backplating (set
`ApplicationHighContrastAdjustment` to `None`), we'll need to select
better brushes that work in HC mode.  MUX has already selected brushes
that work great in all modes, so it just makes sense to use their
brushes.

The one very subtle difference here is that, for non-HC themes, the
glyph's foreground has a bit more contrast when the button is in
hovered/pressed states.  Again this slight difference hardly matters
now, but using the correct brushes will become critical when we try to
remove the HC backplating.

Closes #6812

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

ghost commented Jul 21, 2020

🎉Windows Terminal v1.1.2021.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 21, 2020

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

Handy links:

@ghost
Copy link

ghost commented Jul 22, 2020

🎉Windows Terminal v1.1.2021.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 22, 2020

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

Handy links:

@ghost
Copy link

ghost commented Jul 22, 2020

🎉Windows Terminal Preview v1.2.2022.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 22, 2020

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

Handy links:

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 Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants