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.

Sign up for GitHub

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

Fix bug where removal of TeachingTip would leave the Popup open #1327

Merged
merged 17 commits into from
Oct 22, 2019
Merged

Fix bug where removal of TeachingTip would leave the Popup open #1327

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d9e58de
Fix bug where removal of TeachingTip would leave the Popup open
marcelwgn Sep 13, 2019
0e37f67
Update dev/TeachingTip/TeachingTip.cpp
marcelwgn Sep 13, 2019
1ce35c2
Update dev/TeachingTip/TeachingTip.h
marcelwgn Sep 13, 2019
b3c0a3b
Update dev/TeachingTip/TeachingTip.cpp
marcelwgn Sep 13, 2019
a63dc92
Fix missmatching function name
marcelwgn Sep 14, 2019
e6f3eb6
Move function declaration to correct position in header file
marcelwgn Sep 14, 2019
f431cf9
Update behaviour to set open state to false when unloaded
marcelwgn Sep 14, 2019
54c3775
Add new test for unload event of TeachingTip
marcelwgn Sep 14, 2019
cff76b5
Apply suggestion from code review
marcelwgn Sep 19, 2019
2f3095e
Remove obsolete code in TeachingTipPage
marcelwgn Oct 2, 2019
257ef26
Re-add test window bound closing code.
marcelwgn Oct 2, 2019
976bec6
Add logic to deal with target unloading
marcelwgn Oct 13, 2019
543fa69
Add tests for unloading of target of TT (for #1426)
marcelwgn Oct 13, 2019
95658c3
Fix wrongly named function
marcelwgn Oct 14, 2019
195fe4b
Switch to Revoker pattern
marcelwgn Oct 14, 2019
1282b29
Apply suggestions from code review
marcelwgn Oct 15, 2019
7d760f1
Merge branch 'master' of https://github.com/microsoft/microsoft-ui-xa…
StephenLPeters Oct 18, 2019
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
18 changes: 18 additions & 0 deletions dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,24 @@ public TextBlock GetEffectiveForegroundOfTeachingTipContentTextBlock()
}
private TextBlock effectiveForegroundOfTeachingTipContentTextBlock;

public Button GetRemoveTeachingTipButton()
{
return GetElement(ref effectiveRemoveTeachingTipButton, "RemoveTeachingTipButton");
}

private Button effectiveRemoveTeachingTipButton;
marcelwgn marked this conversation as resolved.
Show resolved Hide resolved
public TextBlock GetTeachingTipContent()
{
return GetElement(ref effectiveTeachingTipContent, "TeachingTipContentTextBlock");
}
private TextBlock effectiveTeachingTipContent;

public CheckBox GetToolTipContentUnloadedCheckbox()
marcelwgn marked this conversation as resolved.
Show resolved Hide resolved
{
return GetElement(ref effectiveToolTipContentUnloadedCheckbox, "VisualTreeTeachingTipContentTextBlockUnloaded");
}
private CheckBox effectiveToolTipContentUnloadedCheckbox;

private T GetElement<T>(ref T element, string elementName) where T : UIObject
{
if (element == null)
Expand Down
24 changes: 23 additions & 1 deletion dev/TeachingTip/InteractionTests/TeachingTipTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Windows.UI.Xaml.Tests.MUXControls.InteractionTests.Infra;
using Windows.UI.Xaml.Tests.MUXControls.InteractionTests.Infra;
using Windows.UI.Xaml.Tests.MUXControls.InteractionTests.Common;
using System;
using System.Numerics;
Expand Down Expand Up @@ -115,6 +115,28 @@ public void CloseReasonIsAccurate()
}
}
}
[TestMethod]
public void TeachingTipRemovalClosesPopup()
{
using (var setup = new TestSetupHelper("TeachingTip Tests"))
{
elements = new TeachingTipTestPageElements();
ScrollTargetIntoView();
OpenTeachingTip();

CheckBox unloadedCheckbox = elements.GetToolTipContentUnloadedCheckbox();
Verify.IsTrue(unloadedCheckbox.ToggleState == ToggleState.Off);

// Finding the button to remove the teaching tip
Button removeButton = elements.GetRemoveTeachingTipButton();

// Removing teaching tip

removeButton.InvokeAndWait();
Verify.IsTrue(unloadedCheckbox.ToggleState == ToggleState.On);
Verify.IsTrue(elements.GetIsOpenCheckBox().ToggleState == ToggleState.Off);
}
}

[TestMethod]
public void TipCanFollowTarget()
Expand Down
7 changes: 7 additions & 0 deletions dev/TeachingTip/TeachingTip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ TeachingTip::TeachingTip()
__RP_Marker_ClassById(RuntimeProfiler::ProfId_TeachingTip);
SetDefaultStyleKey(this);
EnsureProperties();
Unloaded({ this, &TeachingTip::ClosePopupOnUnloadEvent });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unloaded({ this, &TeachingTip::ClosePopupOnUnloadEvent }); [](start = 4, length = 58)

I'm slightly concerned about how this will appear to behave to the developer. Teaching tips can be declared anywhere and I'm not positive under which circumstances the tip will be loaded (and thus unloaded). I did some experiments placing the teaching tip in different resource dictionaries and it behaves as I would expect, but I wonder if we haven't thought about all the scenarios.

m_automationNameChangedRevoker = RegisterPropertyChanged(*this, winrt::AutomationProperties::NameProperty(), { this, &TeachingTip::OnAutomationNameChanged });
m_automationIdChangedRevoker = RegisterPropertyChanged(*this, winrt::AutomationProperties::AutomationIdProperty(), { this, &TeachingTip::OnAutomationIdChanged });
SetValue(s_TemplateSettingsProperty, winrt::make<::TeachingTipTemplateSettings>());
Expand All @@ -25,6 +26,12 @@ winrt::AutomationPeer TeachingTip::OnCreateAutomationPeer()
return winrt::make<TeachingTipAutomationPeer>(*this);
}

void TeachingTip::ClosePopupOnUnloadEvent(winrt::IInspectable const& /* sender */, winrt::RoutedEventArgs const& /* e */)
marcelwgn marked this conversation as resolved.
Show resolved Hide resolved
{
IsOpen(false);
ClosePopup();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that if the user navigates away from the teaching tip (the unloaded event is fired) we will never play the closing animation. Is this actually the design want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SavoySchuler

}

void TeachingTip::OnApplyTemplate()
{
m_acceleratorKeyActivatedRevoker.revoke();
Expand Down
1 change: 1 addition & 0 deletions dev/TeachingTip/TeachingTip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ class TeachingTip :
void OnTargetLayoutUpdated(const winrt::IInspectable&, const winrt::IInspectable&);
void OnTargetLoaded(const winrt::IInspectable&, const winrt::IInspectable&);
void RepositionPopup();
void ClosePopupOnUnloadEvent(winrt::IInspectable const&, winrt::RoutedEventArgs const& e);

void CreateExpandAnimation();
void CreateContractAnimation();
Expand Down
12 changes: 8 additions & 4 deletions dev/TeachingTip/TestUI/TeachingTipPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<local:TestPage
<local:TestPage
x:Class="MUXControlsTestApp.TeachingTipPage"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
Expand All @@ -14,11 +14,12 @@
<Grid.ColumnDefinitions>
<ColumnDefinition Width="Auto"/>
<ColumnDefinition Width="*"/>
<ColumnDefinition Width="*"/>
</Grid.ColumnDefinitions>

<Grid Grid.Column="0" Margin="5" MinWidth="500">
<ScrollViewer x:Name="ContentScrollViewer">
<StackPanel>
<StackPanel x:Name="ContentStackPanel">
<Grid Background="Green" Width="100" Height="1000"/>
<muxc:TeachingTip x:Name="TeachingTipInVisualTree"
AutomationProperties.Name="TeachingTipInVisualTree"
Expand All @@ -37,7 +38,7 @@
</muxc:TeachingTip.HeroContent>
<muxc:TeachingTip.Content>
<StackPanel Orientation="Horizontal">
<TextBlock VerticalAlignment="Center">Cancel closes:</TextBlock>
<TextBlock VerticalAlignment="Center" x:Name="TeachingTipContentTextBlock" Unloaded="RemoveTeachingTipTextBlockContent_Unloaded">Cancel closes:</TextBlock>
<CheckBox x:Name="CancelClosesCheckBoxInVisualTree" IsChecked="False" VerticalAlignment="Center"/>
</StackPanel>
</muxc:TeachingTip.Content>
Expand All @@ -60,7 +61,7 @@
<Grid MinHeight="200" MinWidth="200" Background="red"/>
</muxc:TeachingTip.HeroContent>
<muxc:TeachingTip.Content>
<StackPanel Orientation="Horizontal">
<StackPanel Orientation="Horizontal" >
<TextBlock VerticalAlignment="Center">Cancel closes:</TextBlock>
<CheckBox x:Name="CancelClosesCheckBoxInResources" IsChecked="False" VerticalAlignment="Center"/>
</StackPanel>
Expand Down Expand Up @@ -373,6 +374,7 @@
<RowDefinition Height="Auto"/>
<RowDefinition Height="Auto"/>
<RowDefinition Height="Auto"/>
<RowDefinition Height="Auto"/>
marcelwgn marked this conversation as resolved.
Show resolved Hide resolved
</Grid.RowDefinitions>
<Grid.ColumnDefinitions>
<ColumnDefinition Width="*"/>
Expand Down Expand Up @@ -537,6 +539,8 @@

<TextBlock Grid.Row="47" Foreground="Red" Text="Is Page RTL" VerticalAlignment="Center"/>
<CheckBox Grid.Row="47" Grid.Column="1" IsChecked="{x:Bind IsPageRTL, Mode=TwoWay}"/>
<Button Grid.Row="48" Grid.Column="1" x:Name="RemoveTeachingTipButton" Click="RemoveTeachingTipButton_Click">Remove tooltip from page</Button>
<CheckBox Grid.Row="49" Grid.Column="1" x:Name="VisualTreeTeachingTipContentTextBlockUnloaded">Visual tree teachingtip unloaded</CheckBox>
</Grid>
</ScrollViewer>

Expand Down
9 changes: 9 additions & 0 deletions dev/TeachingTip/TestUI/TeachingTipPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,5 +941,14 @@ public string BrushToString(Brush brush)

return "Unknown";
}

private void RemoveTeachingTipButton_Click(object sender, RoutedEventArgs e)
{
ContentStackPanel.Children.Remove(TeachingTipInVisualTree);
}
private void RemoveTeachingTipTextBlockContent_Unloaded(object sender, RoutedEventArgs e)
{
VisualTreeTeachingTipContentTextBlockUnloaded.IsChecked = true;
}
}
}