microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.34k stars 677 forks source link

Content Dialog is not resilient to template changes which remove its template parts. #3751

Open BrianDT opened 3 years ago

BrianDT commented 3 years ago

Describe the bug On calling ShowAsync the following exception is raised

System.AccessViolationException HResult=0x80004003 Message=Attempted to read or write protected memory. This is often an indication that other memory is corrupt. Source= StackTrace:

Steps to reproduce the bug Can be reproduced by running the sample located here: https://github.com/BrianDT/UWPTemplatedContentDialogIssue.git

Expected behavior The content dialog should be displayed

Screenshots

templated content dialog issue

Version Info UWP / Uno platform 3.3.0 app built on Microsoft.NETCore.UniversalWindowsPlatform to 6.2.11

NuGet package version: Unsure as this is probably embedded

Windows app type: UWP Win32
Yes
Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041)
November 2019 Update (18363) Yes
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 Yes
Xbox
Surface Hub
IoT

Additional context Initially reported as a uno.ui issue and was redirected here https://github.com/unoplatform/uno/issues/4639

StephenLPeters commented 3 years ago

@BrianDT have you been able to reproduce this issue in a UWP app? The template that you have provided is missing some template parts that the default template has. Its possible that the Content Dialog's code behind is improperly expecting a template part to be present. Can you try updating your template to include the same named parts that the default template specifies?

BrianDT commented 3 years ago

@StephenLPeters good call, I have added borders and grids with the same names as those in the default template and added matching names to the existing Border and Grid. This does enable the dialog to display. Obviously these extra control layers should not be required and no names should be a hard coded, but it is at least a workaround. The sample has been updated to reflect these additions. Just to be sure I also added a pure UWP app project to the sample, fully separated from the Uno projects. Now if I could figure out get the dialog horizontally centred in the window I would be really happy.

StephenLPeters commented 3 years ago

Awesome, sounds like we've identified a real issue in Content Dialog's code behind. It should be resilient to missing template parts. We can use this issue to track that, however its code wasn't lifted to Winui 2, so we will not be able to fix this until post winui 3.

Content Dialog is supposed to be horizontally centered when it opens. Can you share more info on that? We might need/want to open another issue on that.

BrianDT commented 3 years ago

@StephenLPeters Regarding the horizontal centring. It would seem that the Content Dialog control is positioned with its top left corner at the top right of its parent panel. In the sample app the Content Dialog is defined as a child of the red StackPanel shown in the attached screen shot. If the StackPanel is allowed to be the full width of the screen as was originally intended then the Content Dialog is displayed entirely off screen. I would have expected the Content Dialog to be anchored somewhere within the parent panel, what is the expected functionality and should I be raising a separate issue for this? templated content dialog issue 2

StephenLPeters commented 3 years ago

Hmm, I just made a super simple ReproApp.zip and a content dialog opened parented to a stack panel opens at the center of the app. Can you reproduce this issue with a UWP app?

BrianDT commented 3 years ago

@StephenLPeters Oops I left a HorizontalAlignment="Right" in my sample. However, experimenting with the simple example and comparing results indicates that the issue only occurs when the ContentDialog is templated when the top left corner is centred and I assume the width is ignored. This can clearly be seen if the XAML in the simple example is adjusted to:

    <Grid>
        <Grid.RowDefinitions>
            <RowDefinition Height="*" />
            <RowDefinition Height="Auto" />
        </Grid.RowDefinitions>
        <StackPanel>
            <Button Content="Open Codebehind dialog" Click="Button_Click"/>
            <Button Content="Open xaml dialog" Click="Button_Click_1"/>
        </StackPanel>

        <StackPanel Grid.Row="1" Width="500" Height="200" Background="Orange" Margin="0,0,0,64">
            <ContentDialog x:Name="xamlDialog" CloseButtonText="Close" HorizontalAlignment="Center">
                <ContentDialog.Template>
                    <ControlTemplate>
                        <Border x:Name="Container">
                            <!--SHOULD NOT BE NECESSARY-->
                            <Grid x:Name="LayoutRoot">
                                <!--SHOULD NOT BE NECESSARY-->
                                <Border x:Name="BackgroundElement" BorderBrush="Red" BorderThickness="2" CornerRadius="4" Background="Aquamarine" Padding="8">
                                    <ContentPresenter/>
                                </Border>
                            </Grid>
                        </Border>
                    </ControlTemplate>
                </ContentDialog.Template>
                <StackPanel Orientation="Horizontal">
                    <TextBlock>Hello!!</TextBlock>
                    <Button Margin="24,0,0,0" Click="Button_Close" Content="CLOSE" Foreground="White"/>
                </StackPanel>
            </ContentDialog>
        </StackPanel>
    </Grid>

And in the code behind change

        private async void Button_Click_1(object sender, RoutedEventArgs e)
        {
            var result = await xamlDialog.ShowAsync();
        }

To

        private async void Button_Click_1(object sender, RoutedEventArgs e)
        {
            var result = await xamlDialog.ShowAsync(ContentDialogPlacement.InPlace);
        }

And add

        private void Button_Close(object sender, RoutedEventArgs e)
        {
            xamlDialog.Hide();
        }

templated content dialog issue 3

StephenLPeters commented 3 years ago

@StephenLPeters Oops I left a HorizontalAlignment="Right" in my sample. However, experimenting with the simple example and comparing results indicates that the issue only occurs when the ContentDialog is templated when the top left corner is centred and I assume the width is ignored. This can clearly be seen if the XAML in the simple example is adjusted to:

I'm having trouble understanding what the repro steps you are outlining here are. Content Dialog is retemplated, what does top left corner being centered mean? How is the width ignored?

    private async void Button_Click_1(object sender, RoutedEventArgs e)
    {
        var result = await xamlDialog.ShowAsync(ContentDialogPlacement.InPlace);
    }

Using ContentDialogPlacement.InPlace means the dialog will be placed in its parent, rather than in a popup. So I would expect the dialog to be placed within the stack panel. Can you take a look at the LiveVisualTree for that test app and see what properties are causing the popup to be placed where it is?

BrianDT commented 3 years ago

@StephenLPeters Because I am interested in controlling the placement of the dialog I am only interested in the variant using ContentDialogPlacement.InPlace, I have no issues with the pop-up based version. I think the attached image shows the positioning concern quite clearly, the top left corner of the content dialog is positioned at the horizontal centre of its parent panel, I would like to know if this is the expected functionality as I would have expected the dialog to be centred in the parent panel. The Actual Height and Actual Width of the dialog show as 0 in the visual tree while it is displayed.

StephenLPeters commented 3 years ago

In your example the parent panel is a stack panel. When you use InPlace mode you are deferring to the parent for placement of the dialog. Stack panel places its first child at its top (or left for horizontally oriented stack panels.) So yes, I would say this is expected. You should be able to set the vertical alignement property on the content dialog to center to get it positioned in the center.

BrianDT commented 3 years ago

The positioning issue has been raised separately as #4662

github-actions[bot] commented 1 year ago

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.

BrianDT commented 1 year ago

A status update would be appreciated, just scheduled, backlog or discarded would be enough.

JJBrychell commented 1 year ago

@BrianDT "Obviously these extra control layers should not be required"

While we agree that there shouldn't be a crash here (and are taking steps to fix that), the results are not going to be what you expect. Control functionality is built around expected parts of the control. For example, you can't get primary/secondary/close button functionality if those parts don't exist in the template because the control has no idea what elements it should be interacting with. So, it is more than acceptable for a control to require specific parts in its template in order to get functionality associated with those parts.

Currently content dialog is pretty strict on what parts must be present. Even after we fix the crash issue, if ALL the appropriate parts are not present in the template, then content dialog considers it an unsupported template and disables virtually all content dialog functionality (e.g., focus routing, key routing, and content dialog specific layout). This appears to be the cause of a number of other issues that have been reported. For example, the alignment issue also mentioned in this issue is because the expected content dialog specific layout is not occurring due to an unsupported template. It is arguable that this restriction is too strict and that content dialog should only ignore functionality associated with missing parts of the template. We are looking into what it would take to do this, but have made no decisions.

To help with this, would it be possible to get some information on the specific requirements you have that led you to customizing the content dialog template in the first place rather than use the default? This will allow us to consider how best to loosen this restriction and the priority of addressing it. Thanks.