-
Notifications
You must be signed in to change notification settings - Fork 9k
Add caption button dim out for unfocused windows #19668
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
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
f097c48 to
8d1a4da
Compare
carlos-zamora
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a quick glance, I'm personally not a fan of adding a _focused member to MinMaxCloseControl and TitlebarControl. The concern being that at some point these two may go out of sync (not necessarily in this PR, but in the future).
Since TitlebarControl doesn't really use _focused other than passing it down to MinMaxCloseControl, how about this:
- remove
_focusedfromTitlebarControl - when
_titlebar.Focused()is called, instead set it on theMinMaxCloseControl(either replace the code directly or haveTitlebarControl::Focused()just set it on theMinMaxCloseControlthen) - then, you won't need to pass
_focusedeach time you interact with the control.MinMaxCloseControlshould then be keeping track of the state directly and make changes appropriately
That should end up reducing the complexity by a bit, and you'll actually use the MinMaxCloseControl::_focused member you added 😉.
Aside from that, I tested the changes locally and it looks/feels great! Thanks for doing this, I'm excited to see it land soon. 😊
Thanks! I've removed |
Summary of the Pull Request
This PR closes #12632 by dimming the caption controls in the titlebar when unfocused. Leaves them intact when in high contrast mode. The color used is borrowed from PowerToy's unfocused color: https://github.com/microsoft/PowerToys/blob/37bd24db365d7bfc3e27fc77d0ac3c0948ffa189/src/settings-ui/Settings.UI/SettingsXAML/Controls/TitleBar/TitleBar.xaml#L179
References and Relevant Issues
#12632
#10401
Detailed Description of the Pull Request / Additional comments
Introduces a
_focusedboolean toTitlebarControlwhich is passed intoMinMaxCloseControl. Instead of using a new VisualStateGroup, I opted to slot the Unfocused state into CommonStates, this way we prevent any conflicts with Visual Setters, and can preserve animations.Validation Steps Performed
Tested by hovering and unhovering in focused and unfocused states, as well as in high contrast mode. States are consistent and applied correctly. I did notice that clicking on the titlebar wouldn't put it into focus unless I also moved my mouse though, but that is also present in the release branch.
PR Checklist