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

Fixing CommandBarFlyout lifetime crashing bug #6929

Merged
merged 1 commit into from
Apr 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions dev/CommandBarFlyout/CommandBarFlyout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,10 @@ CommandBarFlyout::CommandBarFlyout()
{
args.Cancel(true);

const winrt::CommandBarFlyout commandBarFlyout = *this;

commandBar->PlayCloseAnimation(
winrt::make_weak(commandBarFlyout),
[this]()
RBrid marked this conversation as resolved.
Show resolved Hide resolved
{
m_isClosingAfterCloseAnimation = true;
Expand Down
10 changes: 8 additions & 2 deletions dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -403,17 +403,23 @@ bool CommandBarFlyoutCommandBar::HasCloseAnimation()
}

void CommandBarFlyoutCommandBar::PlayCloseAnimation(
const winrt::weak_ref<winrt::CommandBarFlyout>& weakCommandBarFlyout,
std::function<void()> onCompleteFunc)
{
COMMANDBARFLYOUT_TRACE_INFO(*this, TRACE_MSG_METH, METH_NAME, this);

if (auto closingStoryboard = m_closingStoryboard.get())
{
if (closingStoryboard.GetCurrentState() != winrt::ClockState::Active)
{
m_closingStoryboardCompletedCallbackRevoker = closingStoryboard.Completed(winrt::auto_revoke,
{
[this, onCompleteFunc](auto const&, auto const&)
[this, weakCommandBarFlyout, onCompleteFunc](auto const&, auto const&)
{
onCompleteFunc();
if (weakCommandBarFlyout.get())
{
onCompleteFunc();
}
}
});

Expand Down
2 changes: 1 addition & 1 deletion dev/CommandBarFlyout/CommandBarFlyoutCommandBar.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class CommandBarFlyoutCommandBar :
void PlayOpenAnimation();
bool HasCloseAnimation();
bool HasSecondaryOpenCloseAnimations();
void PlayCloseAnimation(std::function<void()> onCompleteFunc);
void PlayCloseAnimation(const winrt::weak_ref<winrt::CommandBarFlyout>& weakCommandBarFlyout, std::function<void()> onCompleteFunc);

void BindOwningFlyoutPresenterToCornerRadius();

Expand Down
56 changes: 54 additions & 2 deletions dev/CommandBarFlyout/InteractionTests/CommandBarFlyoutTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,8 @@ public void CanTapOnSecondaryItemWithFlyoutWithoutClosing()
Log.Comment("Opening the CommandBar and invoking the first button in the secondary commands list.");
openCommandBarAction();


setup.ExecuteAndWaitForEvents(() => FindElement.ById<Button>("ProofingButton").Invoke(), new List<string>() { "ProofingButton clicked" });


Verify.IsTrue(isFlyoutOpenCheckBox.ToggleState == ToggleState.On);
}
}
Expand Down Expand Up @@ -1264,5 +1262,59 @@ public void VerifyAddPrimaryCommandsDynamically()
KeyboardHelper.PressKey(Key.Escape);
}
}

[TestMethod]
public void CanHideCommandBarFlyoutInFlyoutClosedHandler()
{
CanHideCommandBarFlyoutInFlyoutClosedHandler(useAnimations: false);
CanHideCommandBarFlyoutInFlyoutClosedHandler(useAnimations: true);
}

private void CanHideCommandBarFlyoutInFlyoutClosedHandler(bool useAnimations)
{
using (var setup = new CommandBarFlyoutTestSetupHelper())
{
ToggleButton useAnimatedCommandBarFlyoutCommandBarStyleCheckBox = FindElement.ById<ToggleButton>("UseAnimatedCommandBarFlyoutCommandBarStyleCheckBox");
Verify.IsNotNull(useAnimatedCommandBarFlyoutCommandBarStyleCheckBox);

if (useAnimations && useAnimatedCommandBarFlyoutCommandBarStyleCheckBox.ToggleState == ToggleState.Off)
{
Log.Comment("Using DefaultCommandBarFlyoutCommandBarStyle with animations.");
useAnimatedCommandBarFlyoutCommandBarStyleCheckBox.Toggle();
Wait.ForIdle();
}

ToggleButton hideFlyoutOnFlyoutClosedCheckBox = FindElement.ById<ToggleButton>("HideFlyoutOnFlyoutClosedCheckBox");
Verify.IsNotNull(hideFlyoutOnFlyoutClosedCheckBox);

if (hideFlyoutOnFlyoutClosedCheckBox.ToggleState == ToggleState.Off)
{
Log.Comment("Hiding CommandBarFlyout in FlyoutClosed handler.");
hideFlyoutOnFlyoutClosedCheckBox.Toggle();
Wait.ForIdle();
}

Button showCommandBarFlyoutButton = FindElement.ByName<Button>("Show CommandBarFlyout with AlwaysExpanded");
ToggleButton isFlyoutOpenCheckBox = FindElement.ById<ToggleButton>("IsFlyoutOpenCheckBox");

Log.Comment("Invoking button 'Show CommandBarFlyout with AlwaysExpanded' to show the Flyout9 command bar.");
showCommandBarFlyoutButton.Invoke();
Wait.ForIdle();

Log.Comment("Checking command bar opened successfully.");
Verify.AreEqual(ToggleState.On, isFlyoutOpenCheckBox.ToggleState);

using (var waiter = isFlyoutOpenCheckBox.GetToggledWaiter())
{
Log.Comment("Invoking the first secondary command to close the CommandBarFlyout.");
setup.ExecuteAndWaitForEvents(() => FindElement.ById<Button>("UndoButton9").Invoke(), new List<string>() { "UndoButton9 clicked" });
waiter.Wait();
}
Wait.ForIdle();

Log.Comment("Checking command bar closed successfully.");
Verify.AreEqual(ToggleState.Off, isFlyoutOpenCheckBox.ToggleState);
}
}
}
}
9 changes: 9 additions & 0 deletions dev/CommandBarFlyout/TestUI/CommandBarFlyoutPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,18 @@
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:local="using:MUXControlsTestApp"
xmlns:muxc="using:Microsoft.UI.Xaml.Controls"
xmlns:muxcp="using:Microsoft.UI.Xaml.Controls.Primitives"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:contract13Present="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractPresent(Windows.Foundation.UniversalApiContract,13)"
mc:Ignorable="d">

<local:TestPage.Resources>
<Style x:Key="animatedCommandBarFlyoutCommandBarStyle" TargetType="muxcp:CommandBarFlyoutCommandBar" BasedOn="{StaticResource DefaultCommandBarFlyoutCommandBarStyle}">
<Setter Property="Background" Value="Gold" />
</Style>
</local:TestPage.Resources>

<Grid>
<Grid.Background>
<LinearGradientBrush StartPoint="0,0" EndPoint="1,1">
Expand Down Expand Up @@ -255,6 +262,8 @@
<TextBox x:Name="DynamicWidthChangeCountTextBox" Text="4" Margin="5,0,0,0" AutomationProperties.AutomationId="DynamicWidthChangeCountTextBox"/>
</StackPanel>
<CheckBox x:Name="ClearSecondaryCommandsCheckBox" Content="Clear Secondary Commands Asynchronously?" AutomationProperties.AutomationId="ClearSecondaryCommandsCheckBox" Margin="10,0,10,2" />
<CheckBox x:Name="UseAnimatedCommandBarFlyoutCommandBarStyleCheckBox" Content="Use DefaultCommandBarFlyoutCommandBarStyle with animations?" AutomationProperties.AutomationId="UseAnimatedCommandBarFlyoutCommandBarStyleCheckBox" Margin="10,0,10,2" />
<CheckBox x:Name="HideFlyoutOnFlyoutClosedCheckBox" Content="Hide flyout in FlyoutClosed handler?" AutomationProperties.AutomationId="HideFlyoutOnFlyoutClosedCheckBox" Margin="10,0,10,2" />
</StackPanel>
</ScrollViewer>
<StackPanel Grid.Row="1">
Expand Down
49 changes: 41 additions & 8 deletions dev/CommandBarFlyout/TestUI/CommandBarFlyoutPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@
using MUXControlsTestApp.Utilities;

using CommandBarFlyout = Microsoft.UI.Xaml.Controls.CommandBarFlyout;
using CommandBarFlyoutCommandBar = Microsoft.UI.Xaml.Controls.Primitives.CommandBarFlyoutCommandBar;

namespace MUXControlsTestApp
{
public sealed partial class CommandBarFlyoutPage : TestPage
{
private Style animatedCommandBarFlyoutCommandBarStyle;

private DispatcherTimer clearSecondaryCommandsTimer = new DispatcherTimer();
private CommandBarFlyout clearSecondaryCommandsFlyout;

Expand All @@ -44,6 +47,8 @@ public CommandBarFlyoutPage()
{
this.InitializeComponent();

animatedCommandBarFlyoutCommandBarStyle = this.Resources["animatedCommandBarFlyoutCommandBarStyle"] as Style;

dynamicLabelTimer.Tick += DynamicLabelTimer_Tick;
dynamicVisibilityTimer.Tick += DynamicVisibilityTimer_Tick;
dynamicWidthTimer.Tick += DynamicWidthTimer_Tick;
Expand Down Expand Up @@ -126,18 +131,36 @@ public void OnFlyoutOpened(object sender, object args)

CommandBarFlyout commandBarFlyout = sender as CommandBarFlyout;

if (commandBarFlyout != null && (bool)UseOverflowContentRootDynamicWidthCheckBox.IsChecked && commandBarFlyout.SecondaryCommands != null && commandBarFlyout.SecondaryCommands.Count > 0)
if (commandBarFlyout != null)
{
FrameworkElement secondaryCommandAsFE = commandBarFlyout.SecondaryCommands[0] as FrameworkElement;
FrameworkElement overflowContentRoot = secondaryCommandAsFE.FindVisualParentByName("OverflowContentRoot");

if (overflowContentRoot == null)
if ((bool)UseOverflowContentRootDynamicWidthCheckBox.IsChecked && commandBarFlyout.SecondaryCommands != null && commandBarFlyout.SecondaryCommands.Count > 0)
{
secondaryCommandAsFE.Loaded += SecondaryCommandAsFE_Loaded;
FrameworkElement secondaryCommandAsFE = commandBarFlyout.SecondaryCommands[0] as FrameworkElement;
FrameworkElement overflowContentRoot = secondaryCommandAsFE.FindVisualParentByName("OverflowContentRoot");

if (overflowContentRoot == null)
{
secondaryCommandAsFE.Loaded += SecondaryCommandAsFE_Loaded;
}
else
{
SetDynamicOverflowContentRoot(overflowContentRoot);
}
}
else

if (animatedCommandBarFlyoutCommandBarStyle != null && (bool)UseAnimatedCommandBarFlyoutCommandBarStyleCheckBox.IsChecked && commandBarFlyout.PrimaryCommands != null && commandBarFlyout.PrimaryCommands.Count > 0)
{
SetDynamicOverflowContentRoot(overflowContentRoot);
FrameworkElement primaryCommandAsFE = commandBarFlyout.PrimaryCommands[0] as FrameworkElement;

if (primaryCommandAsFE != null)
{
var commandBarFlyoutCommandBar = primaryCommandAsFE.FindVisualParentByType<CommandBarFlyoutCommandBar>();

if (commandBarFlyoutCommandBar != null)
{
commandBarFlyoutCommandBar.Style = animatedCommandBarFlyoutCommandBarStyle;
}
}
}
}
}
Expand All @@ -160,6 +183,16 @@ public void OnFlyoutClosed(object sender, object args)
SetDynamicVisibilitySecondaryCommand(null);
SetDynamicOverflowContentRoot(null);
SetClearSecondaryCommandsFlyout(null);

if ((bool)HideFlyoutOnFlyoutClosedCheckBox.IsChecked)
{
var commandBarFlyout = sender as CommandBarFlyout;

if (commandBarFlyout != null)
{
commandBarFlyout.Hide();
}
}
}

private void RecordEvent(string eventString)
Expand Down