TabViewItem: Respect Background & Foreground APIs by Felix-Dev · Pull Request #3769 · microsoft/microsoft-ui-xaml

Felix-Dev · 2020-12-04T19:09:53Z

Description

This PR supersedes PR #3216. The main difference between these two PRs is the following: We no longer retire the existing TabViewItemIcon[...] theme resources. Instead, we follow the API design set by the InfoBar which ships theme resources to provide fine-grained customization options yet also utilizes the Foreground API to provide a "customize-all-at-once" option. The relevant discussion leading to this change in API design (and thus this new PR) can be found starting here.

Example: Setting the TabViewItem.Foreground brush will change the foreground of the TabViewItem's icon and header accordingly. If, however, customers want to individually style the icon and header foreground in rest mode, they can use the existing TabViewItemIconForeground and TabViewItemHeaderForeground theme resources instead. See the XAML markup below and the resulting UI.

<muxc:TabView>
    
    <muxc:TabView.Resources>
        <SolidColorBrush x:Key="TabViewItemIconForeground" Color="Turquoise" />
        <SolidColorBrush x:Key="TabViewItemHeaderForeground" Color="Green" />
    </muxc:TabView.Resources>

    <muxc:TabView.TabItems>
        <muxc:TabViewItem Header="Home">
            <muxc:TabViewItem.IconSource>
                <muxc:SymbolIconSource Symbol="Home"/>
            </muxc:TabViewItem.IconSource>
        </muxc:TabViewItem>
        
        <!-- Set foreground using the Foreground API-->
        <muxc:TabViewItem Header="Second tab" Foreground="Yellow">
            <muxc:TabViewItem.IconSource>
                <muxc:SymbolIconSource Symbol="Shop"/>
            </muxc:TabViewItem.IconSource>
        </muxc:TabViewItem>
        
        <muxc:TabViewItem Header="Third tab">
            <muxc:TabViewItem.IconSource>
                <muxc:SymbolIconSource Symbol="Document"/>
            </muxc:TabViewItem.IconSource>
        </muxc:TabViewItem>
    </muxc:TabView.TabItems>
    
</muxc:TabView>

And here is an additional GIF showcasing the behavior:

Note that as with the InfoBar control, the Foreground API takes precendence over the theme resources mentioned above. If the Foreground property is not set (or has been cleared) then the theme resources mentioned above are utilized.

Last but not least, the TabViewItemHeaderBackground theme resource is used by default to set the header background of a TabViewItem, but customers can assign a new brush to the TabViewItem.Background property which will then be used instead.

Additional remarks

As part of this PR, the existing interaction test VerifyTabViewItemHeaderForegroundResource() (alongside the relevant elements in the test UI) has been removed as it is now covered by the newly added TabViewItemForegroundTest() API test.
The TabView API test project was added to the innerloop solution (currently missing).
For reviewers of the previous PR: I have re-written the API tests (Background & Foreground testing)

@stmoy @StephenLPeters @DHowett FYI.

Motivation and Context

Fixes #2653

How Has This Been Tested?

Tested manually and added API tests.

marcelwgn · 2020-12-04T19:15:54Z

MUXControlsInnerLoop.sln

+Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "TabView_APITests", "dev\TabView\APITests\TabView_APITests.shproj", "{2F4E95E9-F729-481C-B9AA-C9BEC91AE395}"
+EndProject


Aren't those changes that I would revert anytime I am using the innerloop without TabView?

This in a permanent change which adds the TabView's API test project to the innerloop solution (missing yet in VS).

On that note, it might be better to make this a separate commit to the master branch depending on how long it will take to merge this PR into master. Otherwise, we could end up seeing multiple TabView PRs which modify the API test project and each would then have the to make the same changes -- if the contributor uses the innerloop.

I think that that change makes more sense to have given that this PR might take some time and contributors would be left without those changes until then. In addition to that, it's also only related to the bug you are fixing in the sense that it is the same control, not much more.

We already have at least two outstanding TabView PRs which add the missing API test project to the innerloop solution so the situation is already less than ideal. I will create a standalone PR for this in the next few minutes.

Standalone PR #3770 has been created.

StephenLPeters · 2020-12-04T19:30:05Z

/azp run

azure-pipelines · 2020-12-04T19:30:21Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2020-12-04T21:24:32Z

dev/TabView/TabViewItem.cpp

+        // If Foreground is set, then change icon and header foreground to match.
+        winrt::VisualStateManager::GoToState(
+            *this,
+            ReadLocalValue(winrt::Control::ForegroundProperty()) == winrt::DependencyProperty::UnsetValue() ? L"ForegroundNotSet" : L"ForegroundSet",


@jevansaks how do we feel about this these days?

@jevansaks or @MikeHillberg Wanting to clear some of these old PRs out. Is DP::UnsetValue acceptable to use? I know there was some contention on this in the past.

Properties in general don't have an "is set" concept. Properties that are implemented with a DependencyProperty appear to have one, do to DP.UnsetValue. But this code example is only seeing if the local value is set or not, rather than checking of the property is being set by any means. For example, maybe the property is "set", but it's set by a Style.

For properties we want to have some kind of unset state we usually talk about sentinel values. For reference types that's usually null, for value types we frequently use Nullable to make it a reference type, and sometimes we use magic values like -1, min/max-infinity, or Nan. (Nullable is the better API choice.)

Is there some other condition we can use here?

StephenLPeters · 2021-07-20T19:47:04Z

/azp run

azure-pipelines · 2021-07-20T19:47:16Z

Azure Pipelines successfully started running 1 pipeline(s).

josephnarai · 2021-08-07T08:28:27Z

Is there a timetable for this bug to be fixed and released? Currently using tabs is difficult - I have not found any methods that allow the background color of the tab header to be set.

The tab header is always this gray (#282828) in dark and a white in white and is completely unable to be changed. Furthermore, I can't find where #282828 is defined? It's not in any of the standard system files.

DHowett · 2021-08-20T17:51:13Z

Please, is there anything Terminal can do to help move this along? What's remaining? We have so much code that manually patches resources on TabViewItems because we cannot do this. 😄

zadjii-msft · 2021-09-02T13:15:40Z

Just to clarify, will this PR make it so the Terminal doesn't need to do this crazy stuff

https://github.com/microsoft/terminal/blob/7908164f9d74b2ecfd899a7bba09f19ee9ac1c83/src/cascadia/TerminalApp/TerminalTab.cpp#L1378-L1388

to change the background color of a TabViewItem anymore? We'll be able to just set TabViewItem().Background() instead?

…ound-api-support

StephenLPeters · 2021-09-02T22:55:16Z

/azp run

azure-pipelines · 2021-09-02T22:55:28Z

Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft · 2021-09-13T14:58:53Z

Related question: when setting the background with these APIs, will it also update the rounded corners on the bottoms, like in microsoft/terminal#7133?

StephenLPeters · 2021-09-13T20:14:06Z

/azp run

azure-pipelines · 2021-09-13T20:14:22Z

Azure Pipelines successfully started running 1 pipeline(s).

zadjii-msft · 2021-09-15T15:13:40Z

Hey I'm trying to ingest this over on the Terminal side, and just wanted to make sure I wasn't doing anything wrong. You can see where I replaced our manual color replacement here: microsoft/terminal@d112bd9

Which ends up looking like this:

TabViewItems seem to only use their Background when they're not selected
Setting the Background on a selected tab will update the visible background, until it's hovered, then the tab will correct itself to the focused color it should use
- While it's in this bugged state (with the Background visible instead of the white it otherwise uses), the bottom corners weren't updated to match (though, arguably they're technically correct at this moment)
The tab hover color is always gray, regardless of what the tab's background color is
Setting a background to a dark color leaves the tab foreground color unchanged. For example, the last tab opened in this gif has a dark purple background, which makes the black text unreadable on it
- admittedly we could fix this on our end, we've already got code to pick a light color if the background is dark (and vice versa), though presumably anyone setting just backgrounds would need to do the same

Is this all expected?

ghost · 2021-09-16T12:55:01Z

I think it is. Brushes for each state need to be set manually, and this API only does it for the unselected state.
Ideally you would set a single color for a tab, and you would automatically generate brushes for all states, just like how accent buttons work.

This is also why the foreground remains black on dark backgrounds. There isn't really something to handle that. There would need to be a style that would take the tab color and create a foreground brush.

* this is the same thing as #10996, but with the fix that caused us to #11031 * This includes microsoft/microsoft-ui-xaml#3769, so we had to make some adjustments to how we handle tab colors. It works the same as before. * Should enable #11231 to be started * [x] Closes #10508 * [x] Closes #7133 * [x] Closes #8948 * [ ] I need to finish letting my 19H1 VM boot to make sure unpackaged still works

DESPITE the fact that there's a `Background()` API that we could just call like: ```c++ TabViewItem().Background(deselectedTabBrush); ``` We actually can't, because it will make the part of the tab that doesn't contain the text totally transparent to hit tests. So we actually _do_ still need to set `TabViewItemHeaderBackground` manually. * Regressed in #11240 * Root cause up in microsoft/microsoft-ui-xaml#3769 * [x] closes #11294

Felix-Dev added 8 commits December 1, 2020 18:02

inner loop VS automatic updates

c171238

fix merge conflicts

7cbf0d4

inner loop VS automatic update

f0b5130

Merge branch 'master' of https://github.com/microsoft/microsoft-ui-xaml

4c9c46e

Respect Background/Foreground API.

bcdb3f4

Add API test project to innerloop solution.

75aca61

Add API tests.

6fdaa78

Remove interaction test which is now covered by an API test.

87525ce

ghost added the needs-triage Issue needs to be triaged by the area owners label Dec 4, 2020

Felix-Dev mentioned this pull request Dec 4, 2020

TabViewItem: Background/Foreground APIs are now respected #3216

Closed

marcelwgn reviewed Dec 4, 2020

View reviewed changes

StephenLPeters added area-TabView team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Dec 4, 2020

StephenLPeters reviewed Dec 4, 2020

View reviewed changes

StephenLPeters added 2 commits July 20, 2021 12:44

Merge master and add color testing buttons

1dfa3f1

add colors testing buttons

978fa9a

Merge branch 'main' into user/Felix-Dev/tabviewitem-background-foregr…

93f31f4

…ound-api-support

zadjii-msft mentioned this pull request Sep 13, 2021

Update to WinUI 2.6 (prerelease) microsoft/terminal#10508

Closed

StephenLPeters approved these changes Sep 13, 2021

View reviewed changes

StephenLPeters merged commit 27052f7 into microsoft:main Sep 13, 2021

zadjii-msft mentioned this pull request Sep 15, 2021

Update to MUX 2.7 microsoft/terminal#11240

Merged

4 tasks

zadjii-msft mentioned this pull request Sep 29, 2021

Allow the entire Tab to be hit testable again microsoft/terminal#11369

Merged

1 task

zadjii-msft mentioned this pull request Aug 2, 2022

Update MUX to 2.8 microsoft/terminal#13657

Closed

7 tasks

		Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "TabView_APITests", "dev\TabView\APITests\TabView_APITests.shproj", "{2F4E95E9-F729-481C-B9AA-C9BEC91AE395}"
		EndProject

Conversation

Felix-Dev commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional remarks

Motivation and Context

How Has This Been Tested?

Uh oh!

marcelwgn Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Felix-Dev Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcelwgn Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Felix-Dev Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Felix-Dev Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

StephenLPeters commented Dec 4, 2020

Uh oh!

azure-pipelines bot commented Dec 4, 2020

Uh oh!

StephenLPeters Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

StephenLPeters Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

MikeHillberg Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

StephenLPeters commented Jul 20, 2021

Uh oh!

azure-pipelines bot commented Jul 20, 2021

Uh oh!

josephnarai commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Aug 20, 2021

Uh oh!

zadjii-msft commented Sep 2, 2021

Uh oh!

StephenLPeters commented Sep 2, 2021

Uh oh!

azure-pipelines bot commented Sep 2, 2021

Uh oh!

zadjii-msft commented Sep 13, 2021

Uh oh!

StephenLPeters commented Sep 13, 2021

Uh oh!

azure-pipelines bot commented Sep 13, 2021

Uh oh!

zadjii-msft commented Sep 15, 2021

Uh oh!

ghost commented Sep 16, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Felix-Dev commented Dec 4, 2020 •

edited

Loading

marcelwgn Dec 4, 2020 •

edited

Loading

Felix-Dev Dec 4, 2020 •

edited

Loading

josephnarai commented Aug 7, 2021 •

edited

Loading

ghost commented Sep 16, 2021 •

edited by ghost

Loading