Skip to content

Commit

Permalink
Fixing AppBarButton visual state overlap (#6933)
Browse files Browse the repository at this point in the history 
Following 664933b we ran into the issue that we now have two different visual state groups targeting the same property (SubItemChevron.Margin), which can cause problems in that one can pave over the other.  I've fixed things by duplicating the SubItemChevron so the states for buttons in the primary and secondary items can be managed separately from each other, and toggled visibility of the two based on where the button is located.

Also added an automated API test that reproduces the issue without the fix.
  • Loading branch information
@llongley
llongley authored Apr 5, 2022
1 parent 2605a04 commit 3867eb0
Show file tree
Hide file tree
Showing 7 changed files with 508 additions and 187 deletions.
95 changes: 93 additions & 2 deletions dev/CommonStyles/APITests/CommonStylesTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,9 +448,13 @@ public void VerifyAppBarButtonLightweightStyling()
xmlns:x='http://schemas.microsoft.com/winfx/2006/xaml'>
<StackPanel.Resources>
<Visibility x:Key='AppBarButtonHasFlyoutChevronVisibility'>Collapsed</Visibility>
<SolidColorBrush x:Key='AppBarButtonSubItemChevronForeground' Color='Red' />
<x:String x:Key='AppBarButtonFlyoutGlyph'>&#xE972;</x:String>
<x:Double x:Key='AppBarButtonSubItemChevronFontSize'>12</x:Double>
<SolidColorBrush x:Key='AppBarButtonSubItemChevronForeground' Color='Red' />
<Thickness x:Key='AppBarButtonSubItemChevronMargin'>-24,20,12,0</Thickness>
<x:String x:Key='AppBarButtonOverflowFlyoutGlyph'>&#xE973;</x:String>
<x:Double x:Key='AppBarButtonSecondarySubItemChevronFontSize'>14</x:Double>
<Thickness x:Key='AppBarButtonSecondarySubItemChevronMargin'>-25,20,12,0</Thickness>
</StackPanel.Resources>
<AppBarButton x:Name='TestAppBarButton'/>
</StackPanel>");
Expand All @@ -466,12 +470,99 @@ public void VerifyAppBarButtonLightweightStyling()

RunOnUIThread.Execute(() =>
{
Panel subItemChevronPanel = (Panel)GetVisualChildByName(appBarButton, "SubItemChevronPanel");
FontIcon subItemChevron = (FontIcon)GetVisualChildByName(appBarButton, "SubItemChevron");
FontIcon overflowSubItemChevron = (FontIcon)GetVisualChildByName(appBarButton, "OverflowSubItemChevron");

Verify.AreEqual((Visibility)root.Resources["AppBarButtonHasFlyoutChevronVisibility"], subItemChevronPanel.Visibility);

Verify.AreEqual((Visibility)root.Resources["AppBarButtonHasFlyoutChevronVisibility"], subItemChevron.Visibility);
Verify.AreEqual((string)root.Resources["AppBarButtonFlyoutGlyph"], subItemChevron.Glyph);
Verify.AreEqual((double)root.Resources["AppBarButtonSubItemChevronFontSize"], subItemChevron.FontSize);
Verify.AreEqual(((SolidColorBrush)root.Resources["AppBarButtonSubItemChevronForeground"]).Color, ((SolidColorBrush)subItemChevron.Foreground).Color);
Verify.AreEqual((Thickness)root.Resources["AppBarButtonSubItemChevronMargin"], subItemChevron.Margin);

Verify.AreEqual((string)root.Resources["AppBarButtonOverflowFlyoutGlyph"], overflowSubItemChevron.Glyph);
Verify.AreEqual((double)root.Resources["AppBarButtonSecondarySubItemChevronFontSize"], overflowSubItemChevron.FontSize);
Verify.AreEqual(((SolidColorBrush)root.Resources["AppBarButtonSubItemChevronForeground"]).Color, ((SolidColorBrush)overflowSubItemChevron.Foreground).Color);
Verify.AreEqual((Thickness)root.Resources["AppBarButtonSecondarySubItemChevronMargin"], overflowSubItemChevron.Margin);
});
}

[TestMethod]
public void VerifyAppBarButtonChevronMarginsDoNotCollide()
{
StackPanel root = null;
ManualResetEvent rootLoadedEvent = new ManualResetEvent(false);
CommandBar commandBar = null;
ManualResetEvent commandBarSizeChangedEvent = new ManualResetEvent(false);
AppBarButton appBarButton = null;

RunOnUIThread.Execute(() =>
{
root = (StackPanel)XamlReader.Load(
@"<StackPanel xmlns='http://schemas.microsoft.com/winfx/2006/xaml/presentation'
xmlns:x='http://schemas.microsoft.com/winfx/2006/xaml'>
<StackPanel.Resources>
<Visibility x:Key='AppBarButtonHasFlyoutChevronVisibility'>Visible</Visibility>
<x:String x:Key='AppBarButtonFlyoutGlyph'>&#xE972;</x:String>
<Thickness x:Key='AppBarButtonSubItemChevronMargin'>-22,20,12,0</Thickness>
<Thickness x:Key='AppBarButtonSubItemChevronLabelOnRightMargin'>-6,20,12,0</Thickness>
<MenuFlyout x:Key='MenuFlyout' Placement='Bottom'>
<MenuFlyoutItem Text='Item 1'/>
<MenuFlyoutItem Text='Item 2'/>
<MenuFlyoutItem Text='Item 3'/>
</MenuFlyout>
</StackPanel.Resources>
<CommandBar x:Name='CommandBar' DefaultLabelPosition='Right'>
<AppBarButton Label='Add' Icon='Add' Flyout='{StaticResource MenuFlyout}'/>
<AppBarSeparator />
<AppBarButton x:Name='LastAppBarButton' Label='Add' Icon='Add' Flyout='{StaticResource MenuFlyout}'/>
<AppBarSeparator />
<CommandBar.SecondaryCommands>
<AppBarButton Icon='Add' Label='Add' />
</CommandBar.SecondaryCommands>
</CommandBar>
</StackPanel>");

root.Loaded += (sender, args) => { rootLoadedEvent.Set(); };
commandBar = (CommandBar)root.FindName("CommandBar");
commandBar.SizeChanged += (sender, args) => { commandBarSizeChangedEvent.Set(); };
appBarButton = (AppBarButton)root.FindName("LastAppBarButton");

Content = root;
Content.UpdateLayout();
});

rootLoadedEvent.WaitOne();
IdleSynchronizer.Wait();

FontIcon subItemChevron = null;

RunOnUIThread.Execute(() =>
{
subItemChevron = (FontIcon)GetVisualChildByName(appBarButton, "SubItemChevron");

Verify.AreEqual((Thickness)root.Resources["AppBarButtonSubItemChevronLabelOnRightMargin"], subItemChevron.Margin);

commandBarSizeChangedEvent.Reset();
commandBar.Width = 0;
});

commandBarSizeChangedEvent.WaitOne();
IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
commandBarSizeChangedEvent.Reset();
commandBar.Width = double.NaN;
});

commandBarSizeChangedEvent.WaitOne();
IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
Verify.AreEqual((Thickness)root.Resources["AppBarButtonSubItemChevronLabelOnRightMargin"], subItemChevron.Margin);
});
}

Expand Down
66 changes: 35 additions & 31 deletions dev/CommonStyles/AppBarButton_themeresources.xaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
<x:Double x:Key="AppBarButtonSubItemChevronFontSize">8</x:Double>
<x:Double x:Key="AppBarButtonSecondarySubItemChevronFontSize">12</x:Double>
<Thickness x:Key="AppBarButtonSubItemChevronMargin">-23,20,12,0</Thickness>
<Thickness x:Key="AppBarButtonSubItemChevronLabelOnRightMargin">-7,20,12,0</Thickness>
<Thickness x:Key="AppBarButtonSecondarySubItemChevronMargin">0,0,16,0</Thickness>
</ResourceDictionary>
<ResourceDictionary x:Key="HighContrast">
Expand Down Expand Up @@ -73,6 +74,7 @@
<x:Double x:Key="AppBarButtonSubItemChevronFontSize">8</x:Double>
<x:Double x:Key="AppBarButtonSecondarySubItemChevronFontSize">12</x:Double>
<Thickness x:Key="AppBarButtonSubItemChevronMargin">-23,20,12,0</Thickness>
<Thickness x:Key="AppBarButtonSubItemChevronLabelOnRightMargin">-7,20,12,0</Thickness>
<Thickness x:Key="AppBarButtonSecondarySubItemChevronMargin">0,0,16,0</Thickness>
</ResourceDictionary>
<ResourceDictionary x:Key="Light">
Expand Down Expand Up @@ -107,6 +109,7 @@
<x:Double x:Key="AppBarButtonSubItemChevronFontSize">8</x:Double>
<x:Double x:Key="AppBarButtonSecondarySubItemChevronFontSize">12</x:Double>
<Thickness x:Key="AppBarButtonSubItemChevronMargin">-23,20,12,0</Thickness>
<Thickness x:Key="AppBarButtonSubItemChevronLabelOnRightMargin">-7,20,12,0</Thickness>
<Thickness x:Key="AppBarButtonSecondarySubItemChevronMargin">0,0,16,0</Thickness>
</ResourceDictionary>
</ResourceDictionary.ThemeDictionaries>
Expand Down Expand Up @@ -193,7 +196,7 @@
<DiscreteObjectKeyFrame KeyTime="0" Value="{StaticResource AppBarButtonTextLabelOnRightMargin}" />
</ObjectAnimationUsingKeyFrames>
<ObjectAnimationUsingKeyFrames Storyboard.TargetName="SubItemChevron" Storyboard.TargetProperty="Margin">
<DiscreteObjectKeyFrame KeyTime="0" Value="-7,20,12,0" />
<DiscreteObjectKeyFrame KeyTime="0" Value="{ThemeResource AppBarButtonSubItemChevronLabelOnRightMargin}" />
</ObjectAnimationUsingKeyFrames>
</Storyboard>
</VisualState>
Expand Down Expand Up @@ -297,10 +300,8 @@
</VisualState>
<VisualState x:Name="OverflowNormal">
<VisualState.Setters>
<Setter Target="SubItemChevron.Glyph" Value="{ThemeResource AppBarButtonOverflowFlyoutGlyph}"/>
<Setter Target="SubItemChevron.Margin" Value="{ThemeResource AppBarButtonSecondarySubItemChevronMargin}"/>
<Setter Target="SubItemChevron.VerticalAlignment" Value="Center"/>
<Setter Target="SubItemChevron.FontSize" Value="{ThemeResource AppBarButtonSecondarySubItemChevronFontSize}"/>
<Setter Target="SubItemChevron.Visibility" Value="Collapsed" />
<Setter Target="OverflowSubItemChevron.Visibility" Value="Visible" />
</VisualState.Setters>
</VisualState>
<VisualState x:Name="OverflowPointerOver">
Expand All @@ -312,11 +313,8 @@
<Setter Target="OverflowTextLabel.Foreground" Value="{ThemeResource AppBarButtonForegroundPointerOver}" />
<contract6Present:Setter Target="KeyboardAcceleratorTextLabel.Foreground" Value="{ThemeResource AppBarButtonKeyboardAcceleratorTextForegroundPointerOver}" />
<Setter Target="SubItemChevron.Foreground" Value="{ThemeResource AppBarButtonSubItemChevronForegroundPointerOver}" />
<Setter Target="SubItemChevron.Glyph" Value="{ThemeResource AppBarButtonOverflowFlyoutGlyph}"/>
<Setter Target="SubItemChevron.Margin" Value="{ThemeResource AppBarButtonSecondarySubItemChevronMargin}"/>
<Setter Target="SubItemChevron.VerticalAlignment" Value="Center"/>
<Setter Target="SubItemChevron.FontSize" Value="{ThemeResource AppBarButtonSecondarySubItemChevronFontSize}"/>
<Setter Target="SubItemChevron.Foreground" Value="{ThemeResource AppBarButtonSubItemChevronForegroundPointerOver}" />
<Setter Target="SubItemChevron.Visibility" Value="Collapsed" />
<Setter Target="OverflowSubItemChevron.Visibility" Value="Visible" />
</VisualState.Setters>
</VisualState>
<VisualState x:Name="OverflowPressed">
Expand All @@ -328,11 +326,8 @@
<Setter Target="OverflowTextLabel.Foreground" Value="{ThemeResource AppBarButtonForegroundPressed}" />
<contract6Present:Setter Target="KeyboardAcceleratorTextLabel.Foreground" Value="{ThemeResource AppBarButtonKeyboardAcceleratorTextForegroundPressed}" />
<Setter Target="SubItemChevron.Foreground" Value="{ThemeResource AppBarButtonSubItemChevronForegroundPressed}" />
<Setter Target="SubItemChevron.Glyph" Value="{ThemeResource AppBarButtonOverflowFlyoutGlyph}"/>
<Setter Target="SubItemChevron.Margin" Value="{ThemeResource AppBarButtonSecondarySubItemChevronMargin}"/>
<Setter Target="SubItemChevron.VerticalAlignment" Value="Center"/>
<Setter Target="SubItemChevron.FontSize" Value="{ThemeResource AppBarButtonSecondarySubItemChevronFontSize}"/>
<Setter Target="SubItemChevron.Foreground" Value="{ThemeResource AppBarButtonSubItemChevronForegroundPressed}" />
<Setter Target="SubItemChevron.Visibility" Value="Collapsed" />
<Setter Target="OverflowSubItemChevron.Visibility" Value="Visible" />
</VisualState.Setters>
</VisualState>
<VisualState x:Name="OverflowSubMenuOpened">
Expand All @@ -344,10 +339,8 @@
<Setter Target="OverflowTextLabel.Foreground" Value="{ThemeResource AppBarButtonForegroundSubMenuOpened}" />
<contract6Present:Setter Target="KeyboardAcceleratorTextLabel.Foreground" Value="{ThemeResource AppBarButtonKeyboardAcceleratorTextForegroundSubMenuOpened}" />
<Setter Target="SubItemChevron.Foreground" Value="{ThemeResource AppBarButtonSubItemChevronForegroundSubMenuOpened}" />
<Setter Target="SubItemChevron.Glyph" Value="{ThemeResource AppBarButtonOverflowFlyoutGlyph}"/>
<Setter Target="SubItemChevron.Margin" Value="{ThemeResource AppBarButtonSecondarySubItemChevronMargin}"/>
<Setter Target="SubItemChevron.VerticalAlignment" Value="Center"/>
<Setter Target="SubItemChevron.FontSize" Value="{ThemeResource AppBarButtonSecondarySubItemChevronFontSize}"/>
<Setter Target="SubItemChevron.Visibility" Value="Collapsed" />
<Setter Target="OverflowSubItemChevron.Visibility" Value="Visible" />
</VisualState.Setters>
</VisualState>

Expand Down Expand Up @@ -379,7 +372,7 @@
<VisualState x:Name="NoFlyout" />
<VisualState x:Name="HasFlyout">
<VisualState.Setters>
<Setter Target="SubItemChevron.Visibility" Value="{ThemeResource AppBarButtonHasFlyoutChevronVisibility}" />
<Setter Target="SubItemChevronPanel.Visibility" Value="{ThemeResource AppBarButtonHasFlyoutChevronVisibility}" />
</VisualState.Setters>
</VisualState>
</VisualStateGroup>
Expand Down Expand Up @@ -456,18 +449,29 @@
VerticalAlignment="Center"
Visibility="Collapsed"
AutomationProperties.AccessibilityView="Raw" />
<FontIcon x:Name="SubItemChevron"
<Grid x:Name="SubItemChevronPanel"
Grid.Column="2"
Glyph="{ThemeResource AppBarButtonFlyoutGlyph}"
FontFamily="{ThemeResource SymbolThemeFontFamily}"
FontSize="{ThemeResource AppBarButtonSubItemChevronFontSize}"
AutomationProperties.AccessibilityView="Raw"
Foreground="{ThemeResource AppBarButtonSubItemChevronForeground}"
VerticalAlignment="Top"
Margin="-23,20,12,0"
MirroredWhenRightToLeft="True"
Visibility="Collapsed"/>

Visibility="Collapsed">
<FontIcon x:Name="SubItemChevron"
Glyph="{ThemeResource AppBarButtonFlyoutGlyph}"
FontFamily="{ThemeResource SymbolThemeFontFamily}"
FontSize="{ThemeResource AppBarButtonSubItemChevronFontSize}"
AutomationProperties.AccessibilityView="Raw"
Foreground="{ThemeResource AppBarButtonSubItemChevronForeground}"
VerticalAlignment="Top"
Margin="{ThemeResource AppBarButtonSubItemChevronMargin}"
MirroredWhenRightToLeft="True"/>
<FontIcon x:Name="OverflowSubItemChevron"
Glyph="{ThemeResource AppBarButtonOverflowFlyoutGlyph}"
FontFamily="{ThemeResource SymbolThemeFontFamily}"
FontSize="{ThemeResource AppBarButtonSecondarySubItemChevronFontSize}"
AutomationProperties.AccessibilityView="Raw"
Foreground="{ThemeResource AppBarButtonSubItemChevronForeground}"
VerticalAlignment="Center"
Margin="{ThemeResource AppBarButtonSecondarySubItemChevronMargin}"
MirroredWhenRightToLeft="True"
Visibility="Collapsed"/>
</Grid>
</Grid>

</Grid>
Expand Down
32 changes: 28 additions & 4 deletions test/MUXControlsTestApp/verification/AppBarButton-7.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,37 @@
Padding=0,0,0,0
RenderSize=0,0
Visibility=Collapsed
[Windows.UI.Xaml.Controls.FontIcon]
[Windows.UI.Xaml.Controls.Grid]
Background=[NULL]
BorderBrush=[NULL]
BorderThickness=0,0,0,0
CornerRadius=0,0,0,0
FocusVisualPrimaryBrush=#E4000000
FocusVisualPrimaryThickness=2,2,2,2
FocusVisualSecondaryBrush=#B3FFFFFF
FocusVisualSecondaryThickness=1,1,1,1
Foreground=#9E000000
Margin=-23,20,12,0
Name=SubItemChevron
Margin=0,0,0,0
Name=SubItemChevronPanel
Padding=0,0,0,0
RenderSize=0,0
Visibility=Collapsed
[Windows.UI.Xaml.Controls.FontIcon]
FocusVisualPrimaryBrush=#E4000000
FocusVisualPrimaryThickness=2,2,2,2
FocusVisualSecondaryBrush=#B3FFFFFF
FocusVisualSecondaryThickness=1,1,1,1
Foreground=#9E000000
Margin=-23,20,12,0
Name=SubItemChevron
RenderSize=0,0
Visibility=Visible
[Windows.UI.Xaml.Controls.FontIcon]
FocusVisualPrimaryBrush=#E4000000
FocusVisualPrimaryThickness=2,2,2,2
FocusVisualSecondaryBrush=#B3FFFFFF
FocusVisualSecondaryThickness=1,1,1,1
Foreground=#9E000000
Margin=0,0,16,0
Name=OverflowSubItemChevron
RenderSize=0,0
Visibility=Collapsed
Loading

0 comments on commit 3867eb0

Please sign in to comment.