-
Notifications
You must be signed in to change notification settings - Fork 677
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
TabViewItem: Background/Foreground APIs are now respected #3216
TabViewItem: Background/Foreground APIs are now respected #3216
Conversation
Also replaced the TabViewItemIconForeground* theme resources in use with the matching TabViewItemHeaderForeground* theme resources.
…wly created resource dictionary "Deprecated_themeresources.xaml".
MUXControlsInnerLoop.sln
Outdated
@@ -1007,7 +1011,6 @@ Global | |||
{8B056B8F-C1AB-4A80-BD17-DEACE9897E6A} = {2165C785-9365-4615-B227-8E9C7444ECE1} | |||
{AE308818-AF18-48BA-BF33-89779083D297} = {2165C785-9365-4615-B227-8E9C7444ECE1} | |||
{74D18B1B-5F6B-4534-945B-131E8E3206FB} = {2165C785-9365-4615-B227-8E9C7444ECE1} | |||
{80AF98CA-BC1D-4011-8460-5671799EC419} = {E95C2CA1-FF23-47CC-A896-CC5175B37890} |
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.
This is a duplicated entry...
|
||
<ResourceDictionary.ThemeDictionaries> | ||
<ResourceDictionary x:Key="Light"> | ||
<StaticResource x:Key="TabViewItemIconForeground" ResourceKey="SystemControlForegroundBaseMediumBrush" /> |
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.
TabViewItemIconForeground [](start = 35, length = 25)
Can we verify that this change doesn't break apps? Basically that an app that references this resource continues to work after this change. I suspect it does but we should confirm
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.
I tested that locally here and and apps still work fine using these resources. Can create a test for this if required.
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.
@stmoy would it be possible to make XCG reference one of these resources so that we can validate that it still works when we update the release version?
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.
@Felix-Dev I think better than using the xcg would be to add something to the IXMP test app that references these moved resources.
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.
@StephenLPeters I'm having troubled adding the IXMP test projects to my VS solution. While I can add the IXMPTestApp
and IXMPTestApp.TAEF
projects seemingly fine, adding the shared IXMPTestApp.Shared
project seems to work not correctly:
These two top-level folders (3c184...., da296....) - I assume - should not be shown in the Solution explorer and I have no idea why VS just decides to add them. They do not exist on the file system.
I suspect these are causing issues here because while I can build the IXMPTestApp
project, I cannot run it. The run process is terminated with error message
The program '[0x5548] IXMPTestApp.exe: Program Trace' has exited with code 0 (0x0).
The program '[0x5548] IXMPTestApp.exe' has exited with code -1073740791 (0xc0000409).
before even the Application constructor is called (no breakpoint hit).
Thoughts here?
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.
@StephenLPeters Would something like this look okay for now?
(I am using a 2px gray border around the theme resource value here to act as a visual border in case the page background has the same color as the theme resource value.)
This is a new page in the MUXControlsReleaseTest apps which for now tests one of the now deprecated tabview theme resources (so this single resource test is meant to act as a test for all theme resources in question here).
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.
What exactly are we testing through this? What would happen if the resource disappears/breaks, would we be able to notice that through the CI?
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.
We are testing here that moving the resources into the new deprecated theme resource file will not break apps. It is my understanding that the CI will then run the tests (launch the app and navigate to the different test pages,...) on the newest available WinUI nuget package and that a missing resource will thus be noticed because the app will crash after navigating to the page consuming said resource.
@StephenLPeters to confirm though.
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.
I think that is more than enough, I wouldn't really expect anyone to look at it so as long as the page load the resource I think its sufficient.
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.
Added the proposed new page to the MUXControlsReleaseTest app now (and updated the WinUI placeholder version).
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…into user/Felix-Dev/tabview-foreground-background-api-support
…ource dictionary for deprecated resources.
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.
🕐
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.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Looks like I missed something when upgrading the placeholder package? |
I am so excited for this (/cc @ranjeshj) |
@Felix-Dev Does this update the close button glyph to be contrast-appropriate for the lightness of the background? |
@DHowett No, this PR does not touch the default color settings for the TabView. I would suggest opening a new issue for the color contrast issue you are seeing. Speaking of color contrast for the TabViewItem's close button, I did a quick test with the Accessibility Insights tool and we seem to meet the contrast ratio of 3:1 for the button glyph and related TabViewItem background colors (button background/tab background) in light/dark mode. I did notice, however, that the close button's PointerOver background is barely discernible when the "parent" tab is selectedin dark mode and Accessibility Insights game me an estimated ratio of 1.194:1 in that case. This might be something to look at at one point though that could, depending on the chosen solution, require additional changes to the internal TabViewItem logic (see PR description #2823 - Additional Information 2). |
@Felix-Dev Sorry, allow me to clarify: If I change the background and foreground color using this new API, does the close button match the foreground color? |
The screenshots indicate that it does not. |
@DHowett Ah I see. Yep, the foreground/background color of the close button is not affected by the Foreground/Background APIs currently. To set the foreground color of the close button, the theme resource Does that work for the Terminal team or do we need to think about more solutions here? For example, we currently do not expose a public TabViewItemCloseButtonStyle. Would that be useful to have instead of having to deal with theme resources directly to control the foreground/background of the close button in its different visual states? |
So, funny you should ask. We got a pull request recently (and an associated issue) that fixed (or complained about) the close button not being updated properly. microsoft/terminal#8046, microsoft/terminal#8209 (PR) I think the crux of the matter is that the close button must maintain contrast with the tabviewitem background, in all states. |
@stmoy @StephenLPeters On a related note, this PR in its current form retires the existing Your thoughts? |
@DHowett We could include the foregound of the tab's close button in the TabViewItem.Foreground API, but that leaves the close button's background untouched (not really relevant in the current default design since it defaults to transparent) and the close button's PointerOver/Pressed visual state foreground/background default colors are also untouched by that. And those would likely need updating as well to provide a visually compelling tab design. So, looking at it from an implementation perspective, customers would likely need to use theme resources anyway for the close button if the tab background can be updated. We also have new controls like the InfoBar, which do not include the close button in the Foreground/Background APIs: <muxc:InfoBar Title="Title"
Message="Message"
Foreground="Yellow"
IsIconVisible="True"
IsClosable="True">
</muxc:InfoBar> Customers can use the InfoBar.Background API to change the background of the info bar as well, so we essentially have the same situation there as with the TabViewItem: <muxc:InfoBar Title="Title"
Message="Message"
Foreground="DarkBlue"
Background="LightBlue"
IsIconVisible="True"
IsClosable="True">
</muxc:InfoBar>
One difference is that the InfoBar exposes a |
@DHowett - when apps start customizing colors, the framework doesn't really have many "smarts" to ensure contrast. It's up to the app once custom colors start being used. That said...
I think this is likely the way to handle this situation. Simply adding a "CloseButtonForeground/Background" wouldn't help support contrast in hover/pressed states - we need something more robust. Since InfoBar already has such an API, maybe we could introduce a similar API here as well. Even so, the experience is no worse than before and since there are workarounds using lightweight styling, I don't think the introduction of TabViewCloseButtonStyle should block this PR, especially since this PR has been sitting in limbo for such a long time. @Felix-Dev @StephenLPeters - what is left to do before getting this pulled in? |
@stmoy Some pipeline failures currently block merging this test. Apparently while upgrading the WinUI placeholder version for the MUXControlsReleaseTest app, I missed something which caused these errors. That said, this step was only done by us because we deprecate theme resources (the different TabViewItemIconForeground[...] theme resources) as part of this PR. For test purposes, it was decided to create a test page consuming these deprecated resources in the MUXControlsReleaseTest app which lead to me wanting to upgrade the local WinUI placeholder version used by that app to allow local testing. However, it might not be best to retire these TabViewItemIconForeground[...] theme resources after all but instead keep them. Please see this question above. If the team agrees that this sounds like a reasonable API design here, we can keep these currently slated-to-be-deprecated theme resources. Which in turn would make the updates to the MUXControlsReleaseTest app unnecessary and will remove that PR blocker. |
I think it's reasonable to keep both brushes as customization options but having Foreground take precedence over both. |
@StephenLPeters @stmoy Since we will now keep the existing TabViewItem* theme resources and don't deprecate them, it might be easier to just start a new PR instead of reverting most of the existing commits (the bulk of the commits in this PR are about handling the deprecated theme resources - including all the MUXControlsReleaseTest app work). Would you agree with this? |
If you think that would be easier I'd encourage you to do that, just be sure to link to this PR so we can access the conversations that happened here. |
Closing this PR in favor of PR #3769. |
Description
With this PR, the
TabViewItem
's Foreground and Background APIs are now respected. Consistent with theMenuFlyoutItem
andNavigationViewItem
, the foreground applies to both the TabViewItem's Icon and its Header. As such, the existingTabViewItemIconForeground*
theme resources are now obsolete and the existingTabViewItemHeaderForeground*
theme resources will apply to both the header and the icon of the TabViewItem now.To avoid breaking changes, the obsolete
TabViewItemIconForeground*
theme resources have been moved into a WinUI-wideDeprecrated_themeresources
file (in thedev/CommonStyles
directory). I've opted for a project-wide file here instead of a per-control file as the shared theme of all deprecated theme resources is that those have only been retained for backward compatibility and will be removed in a future major WinUI version. For example: If a TabView theme resource is deprecated in WinUI 3.1 and a NavigationView theme resource is deprecated in WinUI 3.4, both of these theme resources are slated to be removed in the future major WinUI 4.0 update. As such. it makes sense to group these theme resources in a single file.If we add such "Deprecated_resources" file, we should conisder adding that to an internal checklist for major WinUI releases (based on semver) so that we won't forget to actually remove these deprecated resources in the next major WinUI release.
@stmoy FYI.
This PR also adds the existing TabView
APITests
project to the innerloop solution (it was missing for now).Motivation and Context
Fixes #2653
How Has This Been Tested?
Tested manually and added API tests.
Screenshots: