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

Setting TabViewItem resources manually regressed in 2.8.x? #8260

Closed
zadjii-msft opened this issue Mar 7, 2023 · 5 comments
Closed

Setting TabViewItem resources manually regressed in 2.8.x? #8260

zadjii-msft opened this issue Mar 7, 2023 · 5 comments

Comments

@zadjii-msft
Copy link
Member

Filing this as a part of investigating updating the Terminal from v2.7.3-prerelease.220816001 to WinUI v2.8.2-prerelease.220830001. I'm on 25310.rs_we at the moment, though I noticed similar behavior the last time I tried to update to 2.8 last August. My original PR and notes are in microsoft/terminal#13657

The Terminal colorizes its tabs by updating the brushes in the Resources for individual tab items...
https://github.com/microsoft/terminal/blob/fe2220e07b3b46482a34f29cc2dabea9286fd02c/src/cascadia/TerminalApp/TabBase.cpp#L465-L468

... then makes a pair of GoToState calls to actually update the tab's visuals.
https://github.com/microsoft/terminal/blob/fe2220e07b3b46482a34f29cc2dabea9286fd02c/src/cascadia/TerminalApp/TabBase.cpp#L552-L564

In my branch where I'm trying to update to 2.8, this seemingly no longer works. The tab foregrounds always seem to be #282828, the deselected colors never seem to update.

Furthermore, I tried taking TabView_themeresources.xaml and dropping it wholesale into a resource dictionary, renaming the tab style to MyTabViewStyle and manually setting our own tabview's style to that one, so I could try and wholly retemplate it. I could not get it to work the same as before at all.

Seemingly, the TabViewItem's VisualState's seemed like they would evaluate the {ThemeResource TabViewItemHeaderBackgroundSelected} when the tab was instantiated (before we could set the resources on it). And it seemingly never re-evaluated those resources - you'd always get #282828 colored selected tabs, regardless of switching into/out of the selected state. Looking at the diff between 2.7 and 2.8, it looks like there was always a Setter for TabContainer.Background, so if my presumption is correct, then I have no idea how this ever worked.

Can someone help me figure out what I did wrong here, or what changed in WinUI? This is a hard block from us adopting WinUI 2.8.

Notes

@gabbybilka gabbybilka added the needs-triage Issue needs to be triaged by the area owners label Mar 13, 2023
@bpulliam bpulliam added team-Controls Issue for the Controls team needs-review 👀 product-winui2 and removed needs-triage Issue needs to be triaged by the area owners labels Mar 13, 2023
@zadjii-msft
Copy link
Member Author

A thought - maybe by defining my own style in my own resources, I broke the thing that enabled us to hot-swap the resource values like that. I'm gonna test to see if manually copying the 2.7 TabView style into our project and setting it as a custom style still works. That might help narrow the problem space.

@zadjii-msft
Copy link
Member Author

zadjii-msft commented Mar 17, 2023

maybe by defining my own style in my own resources, I broke the thing that enabled us to hot-swap the resource values like that

After some testing - nah, that wasn't it. On 2.7, putting a copy of the 2.7 tab view styles into our project still works exactly as expected. I'll see if I can build some hybrid 2.7 styles + 2.8 code chimera and see if that even compiles.


update: The chimera (2.8 MUX with the 2.7 styles) builds and runs, but the tab colors are definitely still broken. So that makes me think it's not something that changed in TabView.xaml, at least. Maybe code behind? Maybe the themeresources? Hmm.

@zadjii-msft
Copy link
Member Author

@karenbtlai Could it be:

void TabViewItem::UpdateTabGeometry()
{
auto const templateSettings = winrt::get_self<TabViewItemTemplateSettings>(TabViewTemplateSettings());
auto const scaleFactor = SharedHelpers::Is19H1OrHigher() ?
static_cast<float>(XamlRoot().RasterizationScale()) :
static_cast<float>(winrt::DisplayInformation::GetForCurrentView().RawPixelsPerViewPixel());
auto const height = ActualHeight();
auto const popupRadius = unbox_value<winrt::CornerRadius>(ResourceAccessor::ResourceLookup(*this, box_value(c_overlayCornerRadiusKey)));
auto const leftCorner = popupRadius.TopLeft;
auto const rightCorner = popupRadius.TopRight;
// Assumes 4px curving-out corners, which are hardcoded in the markup
auto data = L"<Geometry xmlns='http://schemas.microsoft.com/winfx/2006/xaml/presentation'>F1 M0,%f a 4,4 0 0 0 4,-4 L 4,%f a %f,%f 0 0 1 %f,-%f l %f,0 a %f,%f 0 0 1 %f,%f l 0,%f a 4,4 0 0 0 4,4 Z</Geometry>";
WCHAR strOut[1024];
StringCchPrintf(strOut, ARRAYSIZE(strOut), data,
height - 1.0,
leftCorner, leftCorner, leftCorner, leftCorner, leftCorner,
ActualWidth() - (leftCorner + rightCorner + 1.0f / scaleFactor),
rightCorner, rightCorner, rightCorner, rightCorner,
height - (5.0 + rightCorner));
const auto geometry = winrt::XamlReader::Load(strOut).try_as<winrt::Geometry>();
templateSettings->TabGeometry(geometry);
}

I have no idea what setting the TabViewItemTemplateSettings::TabGeometry on a TabViewItem actually does, but that block does seem entirely new in 2.8 and something that wasn't just introduced in the xaml styling.

Though, that seems like it's only used in the 2.8 xaml, so theoretically, by not having the SelectedBackgroundPath in my custom style, that shouldn't actually be used anywhere, right?

@karkarl
Copy link
Contributor

karkarl commented Mar 30, 2023

Hey @zadjii-msft sorry for the late response.

In this specific commit the colouring for TabViewItemBackground selected is updated such that SelectedBackgroundPath handles the selection colours, and TabViewItem.Background is set to transparent when selected. This is to avoid double opacities for the two elements for the mica scenario.

Due to this, you will have to set SelectedBackgroundPath.Fill and overwrite its resources for:

  • Selected: <Setter Target="SelectedBackgroundPath.Fill" Value="{ThemeResource TabViewItemHeaderBackgroundSelected}"/>
  • PointerOverSelected: <Setter Target="SelectedBackgroundPath.Fill" Value="{ThemeResource TabViewItemHeaderBackgroundSelected}"/>
  • PressedSelected: <Setter Target="SelectedBackgroundPath.Fill" Value="{ThemeResource TabViewItemHeaderBackgroundSelected}"/>

@zadjii-msft
Copy link
Member Author

For future readers: We're closing this as resolved, despite not really knowing what happened here. You can see our PR to update the Terminal here: microsoft/terminal#15078.

tl;dr: In >=MUX 2.7, we were updating our tab colors by doing a "Visual State Dance", as I called it. We'd manually change the TabViewItem's VisualState to one that it wasn't in, then change it back to the one it should be in. This seemingly re-applied the new values of the brushes. However in 2.8, this seemingly didn't work anymore!

So instead, we do a "Theme Dance", like so:

const auto& reqTheme = TabViewItem().RequestedTheme();
TabViewItem().RequestedTheme(ElementTheme::Light);
TabViewItem().RequestedTheme(ElementTheme::Dark);
TabViewItem().RequestedTheme(reqTheme);

This causes the ThemeResources to be re-evaluated to the new values.

We never got to the root cause of why this seems different in 2.8. It literally makes no sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants