dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.08k stars 1.17k forks source link

Fluent2 TabItem MinWidth Is Too Large #9889

Open robert-abeo opened 1 month ago

robert-abeo commented 1 month ago

Description

A MinWidth for TabItem of 180 is far too large for the Fluent theme. Remember WinUI doesn't have a TabControl, it has a TabView with page tabs that can be torn off and re-ordered, etc. It's a different design/use-case.

For existing WPF apps a MinWidth of 180 breaks the UI and requires re-templating.

Reproduction Steps

https://github.com/dotnet/wpf/blob/673c35418e5416e486e6651b69b89712a36fcf5c/src/Microsoft.DotNet.Wpf/src/Themes/PresentationFramework.Fluent/Styles/TabControl.xaml#L228

Expected behavior

  1. The MinWidth should be much smaller -- in fact removing it is better
  2. There needs to be a lightweight styling resource to set this without re-templating (if it stays)

Note that WinUI doesn't set a value at all here:

https://github.com/microsoft/microsoft-ui-xaml/blob/d74a0332cf0d5e58f12eddce1070fa7a79b4c2db/src/controls/dev/TabView/TabView.xaml#L296

Actual behavior

180 is hard-coded

Regression?

No response

Known Workarounds

No response

Impact

No response

Configuration

No response

Other information

No response

aquinn39 commented 2 days ago

Agreed - one common use case it can break is dialogs like the File Explorer properties dialog for example, which have lots of tabs up the top in a small window so a large size makes them take up way to much space. They should be more like the updated styles for the GDI common controls TabControl I think (like the File Explorer properties dialog one).

And yeah at the very least the MinWidth should be changeable, it doesn't seem to be bound to the MinWidth property of the control.

Only workaround I found was to give them a dummy style, which results in it using the old styles from Aero2 or Classic (in High Contrast mode) Which don't have this issue.

aquinn39 commented 2 days ago

@dipeshmsft Just had a look at your PR. So is the correct way to override the min width to set it in the resources like this <system:Double x:Key="TabItemHeaderMinWidth">0</system:Double>? I gave that a try but it doesn't seem to be working.

miloush commented 1 day ago

I usually don't get involved in the theming PRs but I will just note that I don't think it's reasonable to be turning everything into resource keys. These should be settable using styles. Everything that is set using resource value is losing the affordances of DPs. How do you animate the value? Use it in triggers?

robert-abeo commented 1 day ago

@miloush

I don't think it's reasonable to be turning everything into resource keys.

The XAML space has moved on quite a bit since WPF. UWP and then WinUI introduced the concept of lightweight styling resources and they serve and important and useful purpose. I wouldn't get in the way of this. We should also be matching Fluent UI upstream as much as possible.

miloush commented 1 day ago

And anyone is welcome to move to UWP or WinUI if they value that. Why should we be turning WPF into not-WPF?

robert-abeo commented 1 day ago

Why should we be turning WPF into not-WPF?

... progress and advancement of thinking. Think of the XAML ecosystem as a whole and embrace any needed change.

h3xds1nz commented 1 day ago

As my idol has once said (can't remember that guy's name anymore, sorry):

In a code base this old and so widely used everyone MUST prioritize correctness. We can't risk stability at this point with existing apps.

robert-abeo commented 1 day ago

Sarcasm aside, this is the Fluent theme -- a NEW feature based on WinUI. Obviously, new code isn't looked at the same way as existing code. And I'm also arguing to keep it the same as WinUI. What seems to you as a contradiction is actually a pretty standard approach.

miloush commented 1 day ago

Personally I am willing to entertain justified breaking changes in order to move a product forward. This one in particular will just break people setting these values using styles, for no benefit at all.

aquinn39 commented 1 day ago

I agree with @miloush. I don't think WPF should need to follow what UWP and WinUI are doing. People can use those technologies if they want.

In regards to lightweight styling, I don't think using it in this scenario adds anything of value - why does this control need to do it that way while every other control uses the MinWidth property? I get that a TabItem has a Content property that shows in the actual content area of the TabControl and that this is probably where MinWidth applies, but wouldn't it make more sense to apply it to the actual tab since you can control what is in the Content area - so you can just set the MinWidth of whatever is in there and the MinWidth of the TabControl can apply to the actual tab. Or even have a ContentMinWidth property or something.

But either way, I think this minimum width change on the tab item was unnecessary, and I am wondering if there was some confusion regarding TabItem from WPF and TabViewItem from WinUI; these two controls are different - they have different use cases and functionality. WinUI's is more like the tabs in your browser and in File Explorer, WPF's are more like in the properties dialog for items in File Explorer. These new templates seem to be trying to copy WinUI's.

I'm not a huge fan of many of the changes in the new themes; I feel like a lot of them were unecessary and can break existing UIs. I don't think there was a need to try and copy WinUI, I think WPF should have just done something similar to the GDI Common Controls and just added rounded corners and updated colours and left the layout and features of controls alone. For instance, why do Buttons have their HorizontalAlignment no longer defaulting to Stretch? This breaks countless UIs and all the other WPF themes use Stretch by default. Why was a clear button added to TextBoxes with no easy way to turn it off? Why do SystemColors not support dark mode when the Fluent theme could have very easily overridden these values to support existing UIs that use these specifically so that they DO support the user's colour settings?

Anyway, I'm ranting a bit here, I just think these new themes break a lot of things when in the past, when Aero2 and AeroLite were added (the only other time WPF had new themes added to it), they worked a lot better with existing apps that used the older themes and were enabled by default in Windows 8 and above in .NET Framework 4.0. This is because it didn't try to reinvent the wheel and so allowed apps to easily look modern and match the look and feel of Windows 8 without requiring work from the developer. Microsoft should make it as easy as possible for apps to match the look and feel of Windows 11 if they want to promote it so badly.

dipeshmsft commented 23 hours ago

Agreed - one common use case it can break is dialogs like the File Explorer properties dialog for example, which have lots of tabs up the top in a small window so a large size makes them take up way to much space. They should be more like the updated styles for the GDI common controls TabControl I think (like the File Explorer properties dialog one).

That's right. No objections on that front. I have been working on writing some sample applications using the new theme to understand the gaps that need to be filled and from what I have seen TabControl ( and DataGrid ) need an overhaul.

Regarding the MinWidth thing, although WinUI doesn't set it, but they have functions which recalculate the size of the tab items and adjust accordingly, and the MinWidth resource defined in WinUI was brought here but that is not how TabControl works in WPF.

@dipeshmsft Just had a look at your PR. So is the correct way to override the min width to set it in the resources like this 0</system:Double>? I gave that a try but it doesn't seem to be working.

I guess this should have worked, I must have done some experiment around this but I will take a look again.

I usually don't get involved in the theming PRs but I will just note that I don't think it's reasonable to be turning everything into resource keys. These should be settable using styles. Everything that is set using resource value is losing the affordances of DPs. How do you animate the value? Use it in triggers?

The way that it used to work before will still work, right. Providing resource keys will let users to update these values without touching the style. Can you elaborate this point a bit?

aquinn39 commented 22 hours ago

@dipeshmsft Are the new themes supposed to be in a stable state now that .NET 9 is released? Like are they meant ok to be okay to use in production/release code?

I know the ThemeMode property is still in evaluation state even in .NET 9, but that doesn't seem to be the case for specifying them in Application.xaml, so I assumed they're good to go but things like this and the high contrast mode crash are making me think otherwise.

miloush commented 22 hours ago

As far as I am aware this is all experimental exactly for the purpose of getting more test coverage and finding out issues.

It is not acceptable to me for a theme to be using merged dictionaries. I can tolerate if an assembly also happens to ship with WPF and people decide to merge its resources in at their own risk, but if that remains the case I will be objecting to having a public API doing that.