Expander Fixes #4394

robloo · 2021-03-02T04:43:59Z

Description

Closes #4390
Closes #4391
Closes #4248 (The Header is now stretched by default and will respect min/max/width)

Motivation and Context

Fixes 'Expanding' event to be named 'Expanded' which follows convention
Ensures events are fired after the state is applied
The control is now responsive. Unlike all other content controls, the Expander previously set a MaxWidth in the default style directly. This prevents the control from stretching horizontally.
The control now resizes to fit content as all other content controls such as Button, Border, Grid do.

How Has This Been Tested?

Manually tested in MUXControlsTestApp

Screenshots (if appropriate):

The header and content container elements should always be stretched to give child controls maximum available area.

robloo · 2021-03-02T05:42:57Z

I think there are more issues with the default Expander style. This should be reviewed more carefully, I only fixed the ones I noticed. I'm certain all the height/width settings are not optimal. The control needs to be much more responsive in its architecture and I only did a quick pass on fixing this. (Stretching vertically is still likely not supported for example)

dev/Expander/Expander.idl

dev/Expander/Expander.xaml

This aligns with the default Button style

These should follow the layout root (parent grid) sizing.

robloo · 2021-03-02T20:12:57Z

Setting a height for this control is a little strange when the expander is collapsed (see below). ~~Should the Header take all of this space in this case?~~ Edit: WPF behaves the same so nevermind.

(UWP)

(WPF)

dev/Expander/Expander.xaml

michael-hawker · 2021-03-03T18:04:25Z

dev/Expander/Expander.xaml

+                            HorizontalAlignment="Stretch"
+                            HorizontalContentAlignment="Stretch"
+                            VerticalContentAlignment="Center"


I don't see why these shouldn't still be template bound and then given these values as defaults in the Style setters like the other properties? Otherwise it's even harder to customize.

The issue is that we have to worry about positioning two 'areas' of the control separately: The Header and the Content. There are cases where you want the Header left aligned for example, but the actual content of the expander stretched.

What is done now is the Horizontal/VerticalContentAlignment is template bound to the content presenter. If we template bound in both areas we are forcing the header/content to always be in sync. Instead, stretching the 'container' elements allows independent positing of child content elements in either the header or the content area while also still supporting horizontal stretch.

Adding lightweight styling resources separate for the header/content area is also possible.

Agree, we should template bind the most common one that developers would want to customize and expose the other as theme resources so they can be customized as well.

Experience tells me all properties of the Expander needs to be template bound to the content area rather than the header area. It will be most intuitive that way but it's opposite of how the code currently functions so additional changes are needed. (Background/Padding, etc. map to the header) We can expose theme resources for these header properties.

Edit: I should add that WPF appears to map some properties to both the header and the content areas simultaneously. I'm not sure I agree with that approach and think we should keep them separate. This is just FYI.

I agree they should be seperate and generally we expect these properties to apply to the content area. So the control's background property should set the background of the content we provide to the control in its (metadata marked) content property.

In reply to: 590643542 [](ancestors = 590643542)

Yeah, that makes sense. But the other ones should at least be controlled by static/theme resources to provide better customization. Expander is odd because we usually see folks wanting to customize the header itself compared to the content, as the content is pretty much all end-developer driven vs. any chrome provided by the control itself at least in the default templates we've used in the past.

But the other ones should at least be controlled by static/theme resources to provide better customization

Yes, I'll add the lightweight styling resources so these can be customized without re-templating. There is well established convention for this now (but we are creating a lot of named resources indicating the styling system is deficient...)

Expander is odd because we usually see folks wanting to customize the header itself compared to the content,

I can see that point. Also keep in mind that the header is also one of the places the developer should customize the least. The design of the header is really set by the fluent design system and shouldn't change much so that end-users recognize it. It's like a ComboBox in my mind.

I added two resources to control the header's content alignment: ExpanderHeaderHorizontalContentAlignment and ExpanderHeaderVerticalContentAlignment. I am leaving the HorizontalAlignment of the header itself to stretched. This ensures it fits the grid which is the root for sizing purposes. Child content can still be aligned any way you would like (not only stretched).

dev/Expander/Expander.xaml

StephenLPeters · 2021-03-10T23:20:06Z

/azp run

azure-pipelines · 2021-03-10T23:20:23Z

Azure Pipelines successfully started running 1 pipeline(s).

This reverts commit 1223fb1.

This reverts commit 363c732.

- Header and content resources are clearly named and separated - Relevant control properties now apply to the content area instead of header

- Border thickness of the content area is no longer controlled by the BorderThickness DP of the control itself - This is required as BorderThickness of the control has local precedence over the visual state setter. - The correct solution for this is another filter converter for border thickness

robloo · 2021-03-13T01:02:52Z

dev/Expander/Expander.xaml

-                                BorderBrush="{ThemeResource ExpanderDropDownBorderBrush}"
-                                BorderThickness="{StaticResource ExpanderDropdownDownBorderThickness}"
+                                BorderBrush="{TemplateBinding BorderBrush}"
+                                BorderThickness="{StaticResource ExpanderContentDownBorderThickness}"


This is not ideal.

BorderThickness="{StaticResource ExpanderContentDownBorderThickness}" should be

BorderThickness="{TemplateBinding BorderThickness}"

However, that would not work due to the BorderThickness of the control having local precedence (higher precedence than the visual state setter). The visual state is being used to switch between border thicknesses that have the top or bottom edge set to zero depending on the expand direction.

The correct solution to this is to add another filter converter for the border thickness similar to the CornerRadius. I expect push back so I didn't implement that.

What is done now is considered a hack but is usable for an initial release. Developers can still customize the thickness of the content area using ExpanderContentDownBorderThickness and ExpanderContentUpBorderThickness even while the BorderThickness of the control is ignored.

dev/Expander/Expander.xaml

robloo · 2021-03-13T01:17:21Z

This is ready for review and merge.

ranjeshj · 2021-03-15T13:51:41Z

/azp run

azure-pipelines · 2021-03-15T13:51:57Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2021-03-15T22:53:18Z

dev/Expander/Expander.xaml

-        <Setter Property="BorderBrush" Value="{ThemeResource ExpanderBorderBrush}" />
-        <Setter Property="Padding" Value="{StaticResource ExpanderHeaderPadding}" />
-        <Setter Property="HorizontalAlignment" Value="Stretch" />
-        <Setter Property="HorizontalContentAlignment" Value="Left" />


Why is this HorizontalContentAlignment no longer needed? Looking at this change it looks like the template never actually used this before. Is this just deleting unused lines?

#4394 (comment)

The default actually should be center per Expander Fixes #4394 (comment)

I followed convention of the Button template

When the default value of 'Center' is required, the property is simply omitted. This is better as all controls will fallback to the same default so if once in 1000 years the default was actually changed you wouldn't need to go and update all the templates (single source of truth). The rule of thumb: only change what you need to.

@tashatitova Did I misunderstand about the HorizontalContentAlignment? Were you only referring to VerticalContentAlignment in that comment referenced above?

@tashatitova Can you confirm about HorizontalContentAlignment? The more I think about it, I think this was my misunderstanding and you were only talking about VerticalContentAlignment. I would like to make this change and get this PR merged ASAP.

I just re-read it and your last comment emphasizes vertical. I'm going to make the change and add back HorizontalContentAlignment. Horizontal seems like it should be Left so @StephenLPeters was right to point this out.

StephenLPeters · 2021-03-15T22:57:47Z

dev/Expander/Expander_themeresources.xaml

+
+            <!-- Content -->
+            <StaticResource x:Key="ExpanderContentBackground" ResourceKey="CardBackgroundFillColorSecondaryBrush" />
+            <StaticResource x:Key="ExpanderContentBorderBrush" ResourceKey="ExpanderHeaderBorderBrush" />


Have you tested that this works? I'm worried that if you override the ExpanderHeaderBorderBrush at the local scope the ExpanderContentBorderBrush wont see your override. This is because the resource lookup doesn't restart when a resource redirection is encountered. So when the template is expanded and this resource is found, we'll start looking for a value for the ExpanderHeaderBorderBrush from our current position in the tree walk, which will miss the local overrides lower in the tree.

Have you tested that this works

No, I have not tested this specifically and didn't have a reason to at the time. My changes here were only renaming and I copied the original functionality. Code before renaming is below:

microsoft-ui-xaml/dev/Expander/Expander_themeresources.xaml

Line 41 in d20fcb8

<StaticResource x:Key="ExpanderBorderBrush" ResourceKey="CardStrokeColorDefaultBrush" />

microsoft-ui-xaml/dev/Expander/Expander_themeresources.xaml

Lines 45 to 46 in d20fcb8

<StaticResource x:Key="ExpanderDropDownBackground" ResourceKey="CardBackgroundFillColorSecondaryBrush" />

<StaticResource x:Key="ExpanderDropDownBorderBrush" ResourceKey="ExpanderBorderBrush" />

ExpanderBorderBrush renamed to ExpanderHeaderBorderBrush
ExpanderDropDownBorderBrush renamed to ExpanderContentBorderBrush

Resource resolution is tricky to understand for those outside the codebase and is not well documented at least that I've found. So I'll take your word for how it functions in this area. If you want me to make the change that's fine. It will just be set to CardStrokeColorDefaultBrush as well.

Edit:

I'm worried that if you override the ExpanderHeaderBorderBrush at the local scope the ExpanderContentBorderBrush wont see your override

Actually, that's probably not a bad thing. If you change the Header resource one wouldn't expect the Content to change with it. Either way this line of code should be modified as indicated above.

Note ExpanderHeaderDisabledBorderBrush is also affected but would make sense to follow the ExpanderHeaderBorderBrush when changed. Still, this should change as well due to potential resource lookup issues.

microsoft-ui-xaml/dev/Expander/Expander_themeresources.xaml

Line 22 in cf8bcef

<StaticResource x:Key="ExpanderHeaderDisabledBorderBrush" ResourceKey="ExpanderHeaderBorderBrush" />

There is concern these would not be re-evaluated when expected

robloo · 2021-03-25T18:38:30Z

@StephenLPeters I see some commits that are going to cause a lot of merge issues. What/when is the plan for getting this merged?

ranjeshj · 2021-03-26T15:55:37Z

/azp run

azure-pipelines · 2021-03-26T15:55:48Z

Azure Pipelines successfully started running 1 pipeline(s).

resolved

robloo added 2 commits March 1, 2021 23:42

Rename 'Expanding' to 'Expanded'

1223fb1

Ensure Expanded/Collapsed fire after state is applied

363c732

ghost added the needs-triage Issue needs to be triaged by the area owners label Mar 2, 2021

robloo added 2 commits March 2, 2021 00:26

Remove MaxWidth set in the default style

17c6d85

Make the expander properly size to content

1f80b68

The header and content container elements should always be stretched to give child controls maximum available area.

robloo mentioned this pull request Mar 2, 2021

New Expander control causes LayoutCycle exception in winui 2.6 preview #4255

Closed

ranjeshj reviewed Mar 2, 2021

View reviewed changes

dev/Expander/Expander.idl Outdated Show resolved Hide resolved

ranjeshj requested review from tashatitova and StephenLPeters March 2, 2021 14:07

ranjeshj added area-Expander team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 2, 2021

tashatitova previously requested changes Mar 2, 2021

View reviewed changes

dev/Expander/Expander.xaml Show resolved Hide resolved

dev/Expander/Expander.xaml Outdated Show resolved Hide resolved

dev/Expander/Expander.xaml Outdated Show resolved Hide resolved

robloo commented Mar 2, 2021

View reviewed changes

dev/Expander/Expander.xaml Outdated Show resolved Hide resolved

robloo added 4 commits March 2, 2021 14:52

Remove Stretch content alignment (use Center default)

66bf68f

This aligns with the default Button style

Add Center VerticalAlignment

0ad7bc5

This aligns with the default Button style

Remove Min/MaxWidth from intermediate controls

ee209e1

These should follow the layout root (parent grid) sizing.

Update Expander test page

0c58a25

robloo commented Mar 2, 2021

View reviewed changes

dev/Expander/Expander.xaml Outdated Show resolved Hide resolved

robloo added 4 commits March 2, 2021 16:10

Add fixed-height Expander example

25e8c59

Stretch header content

585b15c

Standardize resource names

21c73eb

The header VerticalContentAlignment must be center

f2d4e00

michael-hawker reviewed Mar 3, 2021

View reviewed changes

dev/Expander/Expander.xaml Show resolved Hide resolved

ranjeshj requested a review from jevansaks March 5, 2021 14:16

robloo mentioned this pull request Mar 11, 2021

Bug: Expander Events Naming #4390

Closed

robloo added 7 commits March 12, 2021 16:04

Revert "Rename 'Expanding' to 'Expanded'"

77554a7

This reverts commit 1223fb1.

Revert "Ensure Expanded/Collapsed fire after state is applied"

6a72d39

This reverts commit 363c732.

Merge branch 'master' into robloo-expander-fixes

4739faf

Padding now applies to the Content area

210ca0c

Reorg and rename expander theme resources

f081da4

- Header and content resources are clearly named and separated - Relevant control properties now apply to the content area instead of header

Add resources to control header content alignment

9e9a810

robloo commented Mar 13, 2021

View reviewed changes

dev/Expander/Expander.xaml Show resolved Hide resolved

Fix Expander event ordering

6543952

Fix some Static not Theme resources

cf8bcef

StephenLPeters reviewed Mar 15, 2021

View reviewed changes

robloo added 2 commits March 16, 2021 01:03

Remove some resource redirections

ece7611

There is concern these would not be re-evaluated when expected

Remove unnecessary margins

53d69f4

ranjeshj requested a review from tashatitova March 26, 2021 15:55

StephenLPeters approved these changes Mar 26, 2021

View reviewed changes

StephenLPeters merged commit 9929977 into microsoft:master Mar 26, 2021

robloo deleted the robloo-expander-fixes branch March 27, 2021 15:35

michael-hawker mentioned this pull request Jul 16, 2021

Collapsed/Expanded Event for Expander Control #5495

Open

robloo mentioned this pull request Oct 19, 2022

Align Expander Fluent ControlTheme with WinUI AvaloniaUI/Avalonia#9234

Closed

robloo mentioned this pull request Nov 28, 2022

Add Expander Events AvaloniaUI/Avalonia#9555

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expander Fixes #4394

Expander Fixes #4394

robloo commented Mar 2, 2021 •

edited

Loading

robloo commented Mar 2, 2021 •

edited

Loading

robloo commented Mar 2, 2021 •

edited

Loading

michael-hawker Mar 3, 2021

robloo Mar 3, 2021

ranjeshj Mar 9, 2021

robloo Mar 9, 2021

robloo Mar 9, 2021

StephenLPeters Mar 10, 2021 •

edited

Loading

michael-hawker Mar 10, 2021

robloo Mar 11, 2021

robloo Mar 13, 2021

StephenLPeters commented Mar 10, 2021

azure-pipelines bot commented Mar 10, 2021

robloo Mar 13, 2021 •

edited

Loading

robloo commented Mar 13, 2021

ranjeshj commented Mar 15, 2021

azure-pipelines bot commented Mar 15, 2021

StephenLPeters Mar 15, 2021

robloo Mar 16, 2021

robloo Mar 16, 2021

robloo Mar 25, 2021

robloo Mar 25, 2021

StephenLPeters Mar 15, 2021

robloo Mar 16, 2021 •

edited

Loading

robloo Mar 16, 2021

robloo commented Mar 25, 2021

ranjeshj commented Mar 26, 2021

azure-pipelines bot commented Mar 26, 2021

	<StaticResource x:Key="ExpanderDropDownBackground" ResourceKey="CardBackgroundFillColorSecondaryBrush" />
	<StaticResource x:Key="ExpanderDropDownBorderBrush" ResourceKey="ExpanderBorderBrush" />

Expander Fixes #4394

Expander Fixes #4394

Conversation

robloo commented Mar 2, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

robloo commented Mar 2, 2021 • edited Loading

robloo commented Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Mar 10, 2021

azure-pipelines bot commented Mar 10, 2021

robloo Mar 13, 2021 • edited Loading

Choose a reason for hiding this comment

robloo commented Mar 13, 2021

ranjeshj commented Mar 15, 2021

azure-pipelines bot commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robloo Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robloo commented Mar 25, 2021

ranjeshj commented Mar 26, 2021

azure-pipelines bot commented Mar 26, 2021

robloo commented Mar 2, 2021 •

edited

Loading

robloo commented Mar 2, 2021 •

edited

Loading

robloo commented Mar 2, 2021 •

edited

Loading

StephenLPeters Mar 10, 2021 •

edited

Loading

robloo Mar 13, 2021 •

edited

Loading

robloo Mar 16, 2021 •

edited

Loading