Improve theming of TabView in High Contrast mode

[jtippet]

TabView is ugly when High Contrast (HC) mode is enabled. This repros with Windows Desktop build 18363.476 and MUX compiled from a recent master (1a62591).

I've made a few changes to make the TabView look better, using Edge Chromium as a reference. Here's a hypnotic compilation of screenshots. TabView "now" is the current HEAD, and TabView "new" illustrates my proposed changes.

Summary of changes:

• I removed the background behind the [x] on disabled tabs in all modes
• I added a background behind the [x] on selected & mouseover tabs in HC mode
• I removed the backdrop behind text, icons and button glyphs in HC mode
• I added a mouseover highlight to tabs and the overflow buttons in HC mode
• I added a demo disabled tab to the test app for tinkering purposes

App	Screenshot
	Light theme
TabView now
TabView new
Edge (chromium)
Edge (spartan)
IE
	Dark theme
TabView now
TabView new
Edge (chromium)
Edge (spartan)
	High Contrast White
TabView now
TabView new
Edge (chromium)
Edge (spartan)
IE
	High Contrast Black
TabView now
TabView new
Edge (chromium)
Edge (spartan)
IE

[jevansaks]

Thanks, the GIFs included are awesome! Adding @YuliKl as our resident accessibility person to help review the proposed changes.

[jevansaks]

/azp run

[azure-pipelines]

Pull request contains merge conflicts.

[jtippet]

I rebased on the latest HEAD to fix the merge conflict. Do we need a new /azp run to make the CI go?

Incidentally, I was thinking about this some more, and I wonder if this change is setting ourselves up for possible trouble. The TabView always paints text with a hardcoded foreground color, and for inactive tabs, it's painted over a transparent background color. The hosting app could, in theory, select a poor background color for the tabview, and all the inactive tabs will bleed that color through. Of course, a contentious developer would never select a bad background color while in HC mode, but not all developers do the right thing.

If that's a real concern, I can modify the theming to forcibly put a contrasting background on all tabs/buttons when in HC mode, not just the active tabs. The advantage is that we'd never draw text over an unknown background color. The disadvantage is that developers will lose the ability to customize the background of inactive tabs while in HC mode. (I think we can still give them control over the gutter space, at least -- we never draw text over that, so it doesn't matter.)

To summarize this logic in pseudo-code:

GetTabBackgroundColor(Tab)
{
    if (Tab.IsHovered) return Theme.TabViewItemHeaderBackgroundPointerOver;
    else if (Tab.IsSelected) return Theme.TabViewItemHeaderBackgroundSelected;
    else if (IsHighContrastMode()) return Theme.TabViewItemHeaderBackgroundInactive;
    else return SystemControlTransparentBrush; // sees through to the gutter
}

Or to summarize in pictures, let's say that a developer hardcodes a tasteful LightPink background for a TabView, but forgets to adapt that for HC Dark mode:

	Result
Current code in master
With my original changes
New alternative discussed here

This alternative is extremely easy to implement, so the only question is whether it's worth losing the opportunity to customize the background in HC mode.

We could try to mitigate even that -- we could try sniffing the background color and running some fancy math on it to see if it provides adequate contrast, or just expose a knob to the developer that says YesIPromiseITestedThisOnHighContrastMode=true. (Which I guess is usually spelled HighContrastAdjustment=false).

[YuliKl]

@jtippet thank you for all this work this is awesome!

One high-level piece of feedback - by convention, we usually keep a resource mapped to the same brush in all themes. So for example, I would expect TabViewBackground to map to SystemControlBackgroundBaseLowBrush in all three themes. If SystemControlBackgroundBaseLowBrush isn't the right brush in, for example, HC, I would try to find a different brush that resolved to the right colors in all themes.

I'll try to come up with some concrete suggestions for how to keep this uniform mapping.

[YuliKl]

This option is extremely easy to implement, so the only question is whether it's worth losing the opportunity to customize the background in HC mode.

In a word, yes. HC is all about end-user customization - letting users choose the combination of potentially unconventional colors that works best for their vision. HC overrides the aesthetic choices made by the app developer and puts the user in charge.

We could try to mitigate even that -- we could try sniffing the background color and running some fancy math on it to see if it provides adequate contrast,

I strongly advise against doing anything automatic along these lines. Inevitably the magic is complicated to code and doesn't meet the needs of a sufficient percentage of customers.

or just expose a knob to the developer that says YesIPromiseITestedThisOnHighContrastMode=true. (Which I guess is usually spelled HighContrastAdjustment=false).

Indeed 😊

I vote for the "new option"

[jtippet]

Okay, that makes sense. I've implemented additional code so that, when HC mode is enabled, we forcibly set a contrasting background behind anything that we draw text or glyphs on. When HC is not enabled, we let the app developer shoot themselves in the foot, if they insist. (That much is unchanged from the code currently in master.)

Here's the same example as above, but now with a LightPink background on the TabView:

Mode	Screenshot
Light theme
Dark theme
High Contrast White
High Contrast Black

As you can see, dark theme is a readability disaster, as the developer requested, but we saved the day in HC Black. In HC mode, the LightPink gutter is still visible around the edges, but it doesn't intrude into the tabs or buttons.

by convention, we usually keep a resource mapped to the same brush in all themes.

Once we've settled on the final appearance we want, I'll go shopping for brushes that satisfy this convention while delivering the correct appearance. Let me know if there's any further changes to the appearance we want, e.g., if you told me that the [<] and [>] buttons look a bit out-of-place in HC mode and we should change them too while we're at it... I'd put up very little resistance to that.

HighContrastAdjustment=false

I'm not going to do this in this PR, since that would change the API surface, and I'm trying to have this PR be "merely" theme changes. But if you think this is a worthy feature, let's open a separate issue to track it, and I'd be happy to take a swing at implementing it.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

These GIFs are awesome, thanks for making them it really makes the code much easier to review

[StephenLPeters]

Looks like the change is crashing the test app.

[StephenLPeters]

@jtippet are these test failures something you will be able to take a look at in order to get this change merged in?

[jtippet]

Yes, sorry for going dark over the US holiday. The test failures don't repro on my local machine, and I wasn't able to find any great details in the AZP logs, so my best theory is that they're some sort of problem loading the code downlevel. I'm going to set up a downlevel VM and try to repro them, but that chore is going to consume enough time that I'm going to have to save it for the weekend.

[StephenLPeters]

@jtippet Do you still plan to address the test failures, or would you like to see if someone from the team can help?

[jtippet]

Thanks for the offer of help. I eventually figured out the problem (I was missing some ResourceDictionary entries). I've pushed a new update, and also rebased on HEAD again. Can you re-run the tests?

[jevansaks]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jtippet]

Sigh, well that's progress - we're down to 2 test failures. I'll take another look.

[jtippet]

Okay I went through the TabView test cases, and sorted out the failures. I realized that I had assumed most of the tests were just flaky, but actually they were failing on my machine due to the DPI. Once I toggled back to 96 DPI, I was able to repro the exact same set of failures as AZP. I suggest adding a note to the developer guide that you have to run the tests at 96 DPI, and/or a warning to each test case that's known to be thusly brittle, since this wasn't obvious to me.

• CloseSelectionTest: Failed because I added a DisabledTab to the test app to test/demo the behavior and theming of IsEnabled=false tabs. I fixed the test case by adding a checkbox which can hide the new tab, and modifying the test to hide the tab.
• TabSizeAndScrollButtonsTest: Failed for the same reason. I fixed it by bumping the magic constant embedded in the code from 690 to 740.
• DragBetweenTabViewsTest & DragOutsideTest: These fail because the underlying helper library that synthesizes the input gesture, Microsoft.Windows.Apps.Test, is apparently not DPI aware. So on my machine, it drags the wrong pixels around. I tried fixing it by changing InputHelper.DragDistance() to use SinglePointGesture.PressAndDrag (which would be much cleaner code anyway), but even that is profoundly broken on non-96 DPIs. So I gave up. The ideal fix would have to come from M.W.Apps.Test itself.
• SizingTest: Also fails due to DPI. On my machine, it bails because it calculated a size as 699.5 instead of 700. I didn't attempt to fix the test, since I suspect this might actually be a real (if minor) bug in the UI framework; its RP metrics probably shouldn't change if the DPI changes.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

Okay I went through the TabView test cases, and sorted out the failures. I realized that I had assumed most of the tests were just flaky, but actually they were failing on my machine due to the DPI. Once I toggled back to 96 DPI, I was able to repro the exact same set of failures as AZP. I suggest adding a note to the developer guide that you have to run the tests at 96 DPI, and/or a warning to each test case that's known to be thusly brittle, since this wasn't obvious to me.

• CloseSelectionTest: Failed because I added a DisabledTab to the test app to test/demo the behavior and theming of IsEnabled=false tabs. I fixed the test case by adding a checkbox which can hide the new tab, and modifying the test to hide the tab.
• TabSizeAndScrollButtonsTest: Failed for the same reason. I fixed it by bumping the magic constant embedded in the code from 690 to 740.
• DragBetweenTabViewsTest & DragOutsideTest: These fail because the underlying helper library that synthesizes the input gesture, Microsoft.Windows.Apps.Test, is apparently not DPI aware. So on my machine, it drags the wrong pixels around. I tried fixing it by changing InputHelper.DragDistance() to use SinglePointGesture.PressAndDrag (which would be much cleaner code anyway), but even that is profoundly broken on non-96 DPIs. So I gave up. The ideal fix would have to come from M.W.Apps.Test itself.
• SizingTest: Also fails due to DPI. On my machine, it bails because it calculated a size as 699.5 instead of 700. I didn't attempt to fix the test, since I suspect this might actually be a real (if minor) bug in the UI framework; its RP metrics probably shouldn't change if the DPI changes.

Good suggestion! thanks for the work to fix the tests up.

[jtippet]

This exciting new error looks like an infrastructure problem? The test automation script is reporting a powershell syntax error because $versionedMasters is null.

Get-Content : Cannot bind argument to parameter 'Path' because it is null.
At D:\a\1\s\build\Helix\ProcessHelixFiles.ps1:119 char:31
+             $v2 = Get-Content $versionedMasters[$i+1].FullName

The step that had failed before now passes:

All tests eventually passed, but some initially failed.
Finishing: OutputTestResults.ps1

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

The latest failure is due to a newly introduced unreliable test. I've made a PR to disable it #1770. Once that PR is merged in we should pull the changed into your branch and rerun the PR gates. Sorry for the annoyance!

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[StephenLPeters]

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[YuliKl]

I may be overthinking, but the "ActiveTab" part of this resource name doesn't sound quite right to me. @MikeHillberg, what do you think?

[YuliKl]

I'm not a fan of Edge (chromium) approach of having the selected tab look the same as the tab being hovered over. I think Edge (spartan) got the accessibility right in that the hovered tab gains a border but not a full background change.

[jtippet]

That's a fair point, and I'm also slightly uncomfortable with the loss of information of the selected-tab looking the same as a hovered-tab. However, the border is exposed programmatically, so a developer could set one too. It'll take some finesse, I think, to insert a border without messing up apps that also try to set a border.

As to what this should look like, I'll present a few options. In each of these mockups, the first tab is active, the second tab is hovered, and the third is idle.

Border	Mock-up
4px all sides
2px all sides
4px top
4px top and sides
4px bottom

I'm no UI designer, so I don't hold my opinion with any great fondness, but I tend to think that the "4px all sides" option looks the most consistent with the rest of the OS, although the "4px bottom" option is actually kind of cool to interact with.

[YuliKl]

@stmoy, @chigy any thoughts? My personal preference is the "2px all sides" option, which feels consistent with other controls styling and also with Edge (Spartan) UX.

[stmoy]

+1 to 2px on all sides. 4px on all sides seems overly chunky to me, and the other options seem inconsistent with the platform all-up.

[mdtauk]

2px border thickness is being used for the Form controls when focused, so it seems sensible to use that here. perhaps the 4px top border with 2px on the sides could be a good middle ground

[chigy]

Agree with the 2px all sides feeling best.

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).