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

TabView doesn't correctly respect OverlayCornerRadius not specified in App.xaml #3083

Closed
marcelwgn opened this issue Aug 9, 2020 · 19 comments
Labels
area-Resources area-TabView needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) no-issue-activity team-Controls Issue for the Controls team

Comments

@marcelwgn
Copy link
Collaborator

marcelwgn commented Aug 9, 2020

Describe the bug

Paste the following code into an app:

<muxc:TabView>
    <muxc:TabView.TabItems>
        <muxc:TabViewItem Header="Text">
            <muxc:TabViewItem.Resources>
                <CornerRadius x:Key="OverlayCornerRadius">0,0,0,0</CornerRadius>
            </muxc:TabViewItem.Resources>
        </muxc:TabViewItem>
    </muxc:TabView.TabItems>
</muxc:TabView>

The expected result would be:
image

The actual result is:

image

I think this is caused by the fact that bindings inside resources dictionaries/templates are only evaluated once. So overriding the resource inside the app does not have any effect since the binding has already evaluated with the value picked up from the WinUI resources, not the app overriding it.

Steps to reproduce the bug

Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'

Expected behavior

Screenshots

Version Info

NuGet package version:

Windows 10 version Saw the problem?
Insider Build (20185)
May 2020 Update (19041)
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop
Xbox
Surface Hub
IoT

Additional context

Pasting the above code into the MuxControlsTestApp yields:

image

which is also not correct in my opinion.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 9, 2020
@StephenLPeters 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 Aug 10, 2020
@MelbourneDeveloper
Copy link

@chingucoding I'm experiencing a problem that is probably related.

When you try to implement a vanilla TabView, you get an error saying that the app doesn't have this resource.

InnerException = {"The text associated with this error code could not be found.\r\n\r\nCannot find a Resource with the Name/Key OverlayCornerRadius [Line: 25 Position: 39]"}

The XamlControlsGallery doesn't have this problem. It is picking the resources up from somewhere, but I can't figure out where it gets the resource from. Clearly the TabView control doesn't have the correct default resources by default which is a bug, but what is special about the sample that pulls in this resource?

@marcelwgn
Copy link
Collaborator Author

Usually, the OverlayCornerRadius themeresource is shipped with WinUI (see this file). Are you using WinUI and have also added the following reference:

<XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>

@MelbourneDeveloper
Copy link

@chingucoding

image

Nuget:

    <PackageReference Include="Microsoft.UI.Xaml">
      <Version>2.4.3</Version>
    </PackageReference>

XAML:

<Page
    x:Class="WCTTabs.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:WCTTabs"
    xmlns:controls="using:Microsoft.UI.Xaml.Controls"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    
    mc:Ignorable="d"
    
    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
    
    <Grid>
        <controls:TabView />
    </Grid>
    
</Page>

When you say "also added the following reference" do you mean drop the resources somewhere in the resources collection? I tried that and got the same problem.

<Page
    x:Class="WCTTabs.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:WCTTabs"
    xmlns:controls="using:Microsoft.UI.Xaml.Controls"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    
    mc:Ignorable="d"
    
    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">

    <Page.Resources>
        <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>
    </Page.Resources>
    
    <Grid>
        <controls:TabView />
    </Grid>
    
</Page>

Is there are quickstart guide that explains how to get this working from a blank project?

@Felix-Dev
Copy link
Contributor

@MelbourneDeveloper You should add the WinUI reference on the application level:

<Application.Resources>
    <ResourceDictionary>
        <ResourceDictionary.MergedDictionaries>
            <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls" />

            <!-- Other merged dictionaries here -->
        </ResourceDictionary.MergedDictionaries>

        <!-- Other app resources here -->
    </ResourceDictionary>
</Application.Resources>

(When you add a WinUI nuget package to your project, Visual Studio should open a readme.txt file which contains instructions how to add the WinUI resources to your app.)

@marcelwgn
Copy link
Collaborator Author

I think referencing the WinUI resources from the Page.Resources should work, the XAML Controls Gallery references it in the App.xaml though. There is a guide on how to get started with WinUI 2: https://docs.microsoft.com/en-us/windows/apps/winui/winui2/getting-started

@MelbourneDeveloper
Copy link

@chingucoding @Felix-Dev got it. Thanks!

The secret was adding it to the app resources. I think this is going to be a stumbling block for developers new to the toolkit. Generally speaking, controls shouldn't depend on resources that they have to add separately.

Here is what worked:

<Application
    x:Class="WCTTabs.App"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:WCTTabs">

    <Application.Resources>
        <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls" />
    </Application.Resources>

</Application>

@marcelwgn
Copy link
Collaborator Author

marcelwgn commented Sep 28, 2020

The secret was adding it to the app resources. I think this is going to be a stumbling block for developers new to the toolkit.

When installing the WinUI nuget package, Visual Studio opens the packages readme file that explains how to add the WinUI resources. That step is also outlined in the guide I linked. If you have suggests on how to minimize this stumbling block, feel free to comment them here, so we can make adoption of WinUI easier for developers.

Generally speaking, controls shouldn't depend on resources that they have to add separately.

I can see what you mean, however that is kind of the point of lightweight styling. Lighweight styling allows you to override single resource in order to change visual aspects of templates easily without having to retemplate the control. Without resources that templates depend on.


Missclicked and accidently closed the issue, sorry about that.

@marcelwgn marcelwgn reopened this Sep 28, 2020
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Sep 28, 2020
@MelbourneDeveloper
Copy link

@chingucoding hmmm... Well, I can see some merit to that. But this a break from the design of any of the other XAML frameworks I've come across.

People just expect that they can leverage a control without it depending on anything and when it throws errors because resources are missing, people will probably just give up. I almost did.

@marcelwgn
Copy link
Collaborator Author

The lightweight styling is not only present in WinUI but also in other XAML libraries/frameworks such as Uno platform, Avalonia and the Windows Community toolkit. The issue you encountered is that for some reasons, including the WinUI resources did not work, which is not really an issue caused by lightweight styling but rather faulty resource lookup.

@StephenLPeters Is this expected to happen? Shouldn't the XAML resource lookup still pick up the appropriate resource if you include the resources in a page and not on the app level?

@MelbourneDeveloper
Copy link

@chingucoding that doesn't sound right to me. I work with Uno all the time and I don't have to explicitly drag in any extra resources for the controls to work.

I haven't needed to with Avalonia either.

@marcelwgn
Copy link
Collaborator Author

The issue with WinUI 2 is that you need to add the templates and resources consumed by the WinUI controls. By the way, if you don't add the WinUI resources, the WinUI tabview wouldn't work too as it wouldn't be able to find it's template. The advantage of Avalonia and Uno is that they own the complete experience, WinUI 2 is only a library of controls. It's not the complete UWP XAML framework but only an addition to the existing UWP controls, the same is true for the Windows Community Toolkit. The references that you "needed to pull in" where not outside of WinUI, they are all contained in WinUI or the OS XAML. Again the issue you encountered is not an issue with that WinUI expects you to provide resources that are not present in WinUI but rather that the way you included the WinUI resources failed in that case.

@marcelwgn
Copy link
Collaborator Author

Also the following can't work since TabView is not part of the standard UWP controls, but rather is an addition WinUI 2 delivers to customers. When the control tries to look up the template for the control, it asks the XAML resource system where to provide the resource. The XAML resource system however looks at:
a) the standard templates provided through UWP XAML
b) any resource dictionaries on the way from the XAML root to the control (simplifying here)

So if you don't add the resource dictionaries for WinUI to your app, the templates for the control won't be loaded and thus it can't find the template. With WinUI 3, this won't be an issue, for the time being though, WinUI 2 needs this right now because of the way XAML resources work.

<Page
    x:Class="WCTTabs.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:WCTTabs"
    xmlns:controls="using:Microsoft.UI.Xaml.Controls"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    
    mc:Ignorable="d"
    
    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">
    
    <Grid>
        <controls:TabView />
    </Grid>
    
</Page>

@StephenLPeters
Copy link
Contributor

The lightweight styling is not only present in WinUI but also in other XAML libraries/frameworks such as Uno platform, Avalonia and the Windows Community toolkit. The issue you encountered is that for some reasons, including the WinUI resources did not work, which is not really an issue caused by lightweight styling but rather faulty resource lookup.

@StephenLPeters Is this expected to happen? Shouldn't the XAML resource lookup still pick up the appropriate resource if you include the resources in a page and not on the app level?

I think I would expect this code to work generally.. however App.xaml resource dictionary gets special treatment in the resource lookup which is probably why it works and the page dictionary doesn't. resource tree lookup never restarts the process, so I suspect what is happening is we are looking for a template, find it in the page resources, load the template, start the lookup process for the resources that were in the template, from the point at which we found the template. And in this particular case that point is beyond the light weight styles.

App.xaml and generic.xaml are unique in that they gets searched in there entirety for every lookup (I think). @MikeHillberg @kikisaints @kmahone or maybe @jevansaks can confirm or specify further.

I think this does indicate that there might be a better way to set up the projects resources so that templates are always found before resources.

<Page
    x:Class="WCTTabs.MainPage"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
    xmlns:local="using:WCTTabs"
    xmlns:controls="using:Microsoft.UI.Xaml.Controls"
    xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
    
    mc:Ignorable="d"
    
    Background="{ThemeResource ApplicationPageBackgroundThemeBrush}">

    <Page.Resources>
        <XamlControlsResources xmlns="using:Microsoft.UI.Xaml.Controls"/>
    </Page.Resources>
    
    <Grid>
        <controls:TabView />
    </Grid>
    
</Page>

@StephenLPeters
Copy link
Contributor

@ranjeshj as well

@ranjeshj
Copy link
Contributor

ranjeshj commented Sep 28, 2020

I'm curious if this is specific to WinUI2.x and the way we are including the styles in app.xaml. I wonder if there is special handling for styles in generic.xaml which makes it work for OS Xaml and WinUI3. Does this also repro in WinUI3 ?

@ranjeshj ranjeshj added the needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) label Sep 28, 2020
@StephenLPeters
Copy link
Contributor

Luke says we add the templates and the resources separately. I wonder if we change the order we add those in if we could alleviate this issue now.

@StephenLPeters
Copy link
Contributor

here is the code that does it: https://github.com/microsoft/microsoft-ui-xaml/blob/de788345659ba319597161149843504fbe686659/dev/dll/XamlControlsResources.cpp looking closer I'm not sure there is anything we can do.

@StephenLPeters StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Sep 30, 2020
@marcelwgn
Copy link
Collaborator Author

It doesn't seem like there is any code in there that has any say in this. I'm inclined to say that this issue is because of the way resources work which would need to wait until WinUI 3 to be fixed.

@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Resources area-TabView needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) no-issue-activity team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

6 participants