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

TeachingTip: Pressing F6 now sets focus on custom content of light-dismissable tip #3545

Closed
wants to merge 10 commits into from
Closed

TeachingTip: Pressing F6 now sets focus on custom content of light-dismissable tip #3545

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d7db9d1
VS inner loop commit.
Felix-Dev Nov 2, 2020
17981a2
Fix custom content not getting focus if no close button exists.
Felix-Dev Nov 2, 2020
84b753d
Add interaction test.
Felix-Dev Nov 2, 2020
a6b55d9
Fix merge conflicts.
Felix-Dev Nov 2, 2020
c2fdb9b
Change implementation to use FocusManager::GetFocusedElement() instead.
Felix-Dev Nov 23, 2020
cbf2934
Merge branch 'master' into user/Felix-Dev/teachingtip-customcontent-f…
Felix-Dev Nov 23, 2020
76a7868
VS innerloop sln auto-update.
Felix-Dev Dec 5, 2020
9e0b41f
fix merge conflicts
Felix-Dev Dec 5, 2020
74d0a68
Update interaction test for changed TeachingTip test UI.
Felix-Dev Dec 5, 2020
6b98833
Move logic into lambda.
Felix-Dev Dec 7, 2020
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
26 changes: 25 additions & 1 deletion dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,18 @@ public TextBlock GetPopupVerticalOffsetTextBlock()
}
private TextBlock popupVerticalOffsetTextBlock;

public ComboBox GetContentComboBox()
{
return GetElement(ref contentComboBox, "ContentComboBox");
}
private ComboBox contentComboBox;

public Button GetSetContentButton()
{
return GetElement(ref setContentButton, "SetContentButton");
}
private Button setContentButton;

public ComboBox GetHeroContentComboBox()
{
return GetElement(ref heroContentComboBox, "HeroContentComboBox");
Expand Down Expand Up @@ -483,14 +495,26 @@ public enum PlacementOptions
Auto
}

public enum HeroContentOptions
public enum ContentOptions
{
RedSquare,
BlueSquare,
Image,
AutoSaveImage,
ShortText,
LongText,
Button,
NoContent
}

public enum HeroContentOptions
{
RedSquare,
BlueSquare,
Image,
NoContent
}

public enum TitleContentOptions
{
No,
Expand Down
96 changes: 90 additions & 6 deletions dev/TeachingTip/InteractionTests/TeachingTipTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ public void TestCleanup()
TestCleanupHelper.Cleanup();
}


[TestMethod]
public void CloseReasonIsAccurate()
{
Expand Down Expand Up @@ -693,7 +692,6 @@ public void SpecifiedPlacementRTL()
}
}


[TestMethod]
public void NoIconDoesNotCrash()
{
Expand All @@ -718,7 +716,6 @@ public void NoIconDoesNotCrash()
}
}


[TestMethod]
public void CanSwitchShouldConstrainToRootBounds()
{
Expand Down Expand Up @@ -752,7 +749,6 @@ public void CanSwitchShouldConstrainToRootBounds()
}
}


[TestMethod]
public void TipsWhichDoNotFitDoNotOpen()
{
Expand All @@ -768,7 +764,6 @@ public void TipsWhichDoNotFitDoNotOpen()
ScrollBy(10);
UseTestWindowBounds(10, 10, 10, 10);


elements.GetShowButton().InvokeAndWait();

var message1 = GetTeachingTipDebugMessage(1);
Expand All @@ -787,7 +782,6 @@ public void TipsWhichDoNotFitDoNotOpen()
}
}


[TestMethod]
public void VerifyTheming()
{
Expand Down Expand Up @@ -1010,6 +1004,47 @@ void EnableLightDismiss()
}
}

[TestMethod]
public void F6PutsFocusOnContentIfNoCloseButton()
{
using (var setup = new TestSetupHelper(new[] { "TeachingTip Tests", "TeachingTip Test" }))
{
elements = new TeachingTipTestPageElements();
ScrollTargetIntoView();

ClearTeachingTipDebugMessages();

// A light-dismissable teaching tip has no close button.
EnableLightDismiss(true);

// Configure the teaching tip so that it only has one focusable child element - a content button.
SetContent(ContentOptions.Button);
SetHeroContent(HeroContentOptions.NoContent);

OpenTeachingTip();

Log.Comment("Set focus on custom button by pressing F6.");
KeyboardHelper.PressKey(Key.F6);

var message = GetTeachingTipDebugMessage(0);
Verify.IsTrue(message.ToString().Contains("Content Button GotKeyboardFocus Event"));

Log.Comment("Set focus back to teaching tip by pressing F6 again.");
KeyboardHelper.PressKey(Key.F6);

message = GetTeachingTipDebugMessage(1);
Verify.IsTrue(message.ToString().Contains("Content Button LostFocus Event"));

Log.Comment("Verify teaching tip is still open.");
WaitAndVerifyIfCheckStateUnchanged(elements.GetIsOpenCheckBox(), ToggleState.On);

Log.Comment("Close teaching tip by pressing Esc.");
KeyboardHelper.PressKey(Key.Escape);

WaitForTipClosed();
}
}

private void CloseOpenAndCloseWithJustKeyboardViaF6()
{
KeyboardHelper.PressKey(Key.F6);
Expand Down Expand Up @@ -1215,6 +1250,39 @@ private void SetPreferredPlacement(PlacementOptions placement)
elements.GetSetPreferredPlacementButton().InvokeAndWait();
}

private void SetContent(ContentOptions content)
{
switch (content)
{
case ContentOptions.RedSquare:
elements.GetContentComboBox().SelectItemByName("Red Square");
break;
case ContentOptions.BlueSquare:
elements.GetContentComboBox().SelectItemByName("Blue Square");
break;
case ContentOptions.Image:
elements.GetContentComboBox().SelectItemByName("Image");
break;
case ContentOptions.AutoSaveImage:
elements.GetContentComboBox().SelectItemByName("AutoSave Image");
break;
case ContentOptions.ShortText:
elements.GetContentComboBox().SelectItemByName("Short Text");
break;
case ContentOptions.LongText:
elements.GetContentComboBox().SelectItemByName("Long Text");
break;
case ContentOptions.Button:
elements.GetContentComboBox().SelectItemByName("Button");
break;
default:
elements.GetContentComboBox().SelectItemByName("No Content");
break;
}

elements.GetSetContentButton().InvokeAndWait();
}

private void SetHeroContent(HeroContentOptions heroContent)
{
switch (heroContent)
Expand Down Expand Up @@ -1561,5 +1629,21 @@ private bool WaitForOffsetToSettle(Edit text, double millisecondsTimeout, bool f
return false;
}
}

private bool WaitAndVerifyIfCheckStateUnchanged(CheckBox checkBox, ToggleState state, double millisecondsTimeout = 2000)
{
using (UIEventWaiter waiter = checkBox.GetToggledWaiter())
{
if (checkBox.ToggleState != state)
{
return false;
}

Log.Comment($"Waiting for {millisecondsTimeout}ms after which a toggle state change would have typically occured", millisecondsTimeout);
waiter.TryWait(TimeSpan.FromMilliseconds(millisecondsTimeout));

return checkBox.ToggleState == state;
}
}
}
}
21 changes: 20 additions & 1 deletion dev/TeachingTip/TeachingTip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ void TeachingTip::OnApplyTemplate()
m_contentRootGrid.set(GetTemplateChildT<winrt::Grid>(s_contentRootGridName, controlProtected));
m_nonHeroContentRootGrid.set(GetTemplateChildT<winrt::Grid>(s_nonHeroContentRootGridName, controlProtected));
m_heroContentBorder.set(GetTemplateChildT<winrt::Border>(s_heroContentBorderName, controlProtected));
m_mainContentPresenter.set(GetTemplateChildT<winrt::ContentPresenter>(s_mainContentPresenterName, controlProtected));
m_actionButton.set(GetTemplateChildT<winrt::Button>(s_actionButtonName, controlProtected));
m_alternateCloseButton.set(GetTemplateChildT<winrt::Button>(s_alternateCloseButtonName, controlProtected));
m_closeButton.set(GetTemplateChildT<winrt::Button>(s_closeButtonName, controlProtected));
Expand Down Expand Up @@ -1122,12 +1123,30 @@ void TeachingTip::OnF6AcceleratorKeyClicked(const winrt::CoreDispatcher&, const

if (f6Button)
{
auto const scopedRevoker = f6Button.GettingFocus(winrt::auto_revoke, [this](auto const&, auto const& args) {
auto const scopedRevoker = f6Button.GettingFocus(winrt::auto_revoke, [this](auto const&, const winrt::GettingFocusEventArgs& args) {
m_previouslyFocusedElement = winrt::make_weak(args.OldFocusedElement());
});
const bool setFocus = f6Button.Focus(winrt::FocusState::Keyboard);
args.Handled(setFocus);
}
// If there is no close button, try setting focus on the tip's custom content.
else if (const auto mainContentPresenter = m_mainContentPresenter.get())
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making a seperate else block for this case, can we just expand the lambda that sets the f6Button a bit? something like:

            const winrt::Button f6Button = [this]() -> winrt::Button
            {
                auto firstButton = m_closeButton.get();
                auto secondButton = m_alternateCloseButton.get();
                //Prefer the close button to the alternate, except when there is no content.
                if (!CloseButtonContent())
                {
                    std::swap(firstButton, secondButton);
                }
                if (firstButton && firstButton.Visibility() == winrt::Visibility::Visible)
                {
                    return firstButton;
                }
                else if (secondButton && secondButton.Visibility() == winrt::Visibility::Visible)
                {
                    return secondButton;
                }
else if (const auto mainContentPresenter = m_mainContentPresenter.get())
{
    return const auto focusableElement = winrt::FocusManager::FindFirstFocusableElement(mainContentPresenter);
}
                return nullptr;
            }();

Then we shouldn't need this block at all

Copy link
Contributor Author

@Felix-Dev Felix-Dev Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@StephenLPeters Moved code into lambda as suggested. Note that I currently am solely using the FocusManager::GetFocusedElement() API to get the previously focused element as as the GettingFocus event is not available to all possibly focusably elements (such as a Hyperlink).

That said, if you prefer we use the UIElement.GettingFocus API wherever possible (and the documentation suggests this), I can add a check for the f6FocusableElement to see if it is a UIElement and if it is, we use the GettingFocus event instead.

{
if (const auto focusableElement = winrt::FocusManager::FindFirstFocusableElement(mainContentPresenter))
{
const auto previouslyFocusedElement = winrt::FocusManager::GetFocusedElement();

const bool setFocus = SetFocus(focusableElement, winrt::FocusState::Keyboard);
if (setFocus)
{
if (const auto previouslyFocusedElementAsDO = previouslyFocusedElement.try_as<winrt::DependencyObject>())
{
m_previouslyFocusedElement = winrt::make_weak(previouslyFocusedElementAsDO);
}
}
args.Handled(setFocus);
}
}
}
}
}
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 @@ -176,6 +176,7 @@ class TeachingTip :
tracker_ref<winrt::Grid> m_contentRootGrid{ this };
tracker_ref<winrt::Grid> m_nonHeroContentRootGrid{ this };
tracker_ref<winrt::Border> m_heroContentBorder{ this };
tracker_ref<winrt::ContentPresenter> m_mainContentPresenter{ this };
tracker_ref<winrt::Button> m_actionButton{ this };
tracker_ref<winrt::Button> m_alternateCloseButton{ this };
tracker_ref<winrt::Button> m_closeButton{ this };
Expand Down
25 changes: 13 additions & 12 deletions dev/TeachingTip/TestUI/TeachingTipPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@
<ScrollViewer x:Name="ContentScrollViewer">
<StackPanel x:Name="ContentStackPanel">
<Grid Background="Green" Width="100" Height="1000"/>
<muxc:TeachingTip x:Name="TeachingTipInVisualTree"
AutomationProperties.Name="TeachingTipInVisualTree"
Title="We've Added Auto Saving!"
Subtitle="Documents will now automatically save to OneDrive."
Closed="OnTeachingTipClosed"
Closing="OnTeachingTipClosing"
ActionButtonClick="OnTeachingTipActionButtonClicked"
CloseButtonClick="OnTeachingTipCloseButtonClicked"
Target="{x:Bind targetButton}">
<muxc:TeachingTip x:Name="TeachingTipInVisualTree"
AutomationProperties.Name="TeachingTipInVisualTree"
Title="We've Added Auto Saving!"
Subtitle="Documents will now automatically save to OneDrive."
Closed="OnTeachingTipClosed"
Closing="OnTeachingTipClosing"
ActionButtonClick="OnTeachingTipActionButtonClicked"
CloseButtonClick="OnTeachingTipCloseButtonClicked"
Target="{x:Bind targetButton}">
<muxc:TeachingTip.IconSource>
<muxc:SymbolIconSource Symbol="People"/>
</muxc:TeachingTip.IconSource>
Expand Down Expand Up @@ -183,16 +183,17 @@
<Button AutomationProperties.Name="SetSubtitleButton" Click="OnSetSubtitleButtonClicked" Grid.Row="9" Grid.Column="1">Set</Button>

<TextBlock Grid.Row="10" Text="Content:" Foreground="Red"/>
<ComboBox x:Name="ContentComboBox" Grid.Row="11">
<ComboBox x:Name="ContentComboBox" AutomationProperties.Name="ContentComboBox" Grid.Row="11">
<ComboBoxItem x:Name="ContentRedSquare">Red Square</ComboBoxItem>
<ComboBoxItem x:Name="ContentBlueSquare">Blue Square</ComboBoxItem>
<ComboBoxItem x:Name="ContentImage">Image</ComboBoxItem>
<ComboBoxItem x:Name="ContentShort">Short Text</ComboBoxItem>
<ComboBoxItem x:Name="ContentLong">Long Text</ComboBoxItem>
<ComboBoxItem x:Name="ContentAutoSave">AutoSaveImage</ComboBoxItem>
<ComboBoxItem x:Name="ContentAutoSave">AutoSave Image</ComboBoxItem>
<ComboBoxItem x:Name="ContentButton">Button</ComboBoxItem>
<ComboBoxItem x:Name="ContentNo">No Content</ComboBoxItem>
</ComboBox>
<Button Click="OnSetContentButtonClicked" Grid.Row="11" Grid.Column="1">Set</Button>
<Button AutomationProperties.Name="SetContentButton" Click="OnSetContentButtonClicked" Grid.Row="11" Grid.Column="1">Set</Button>

<TextBlock Grid.Row="12" Text="ActionButtonContent:" Foreground="Red"/>
<ComboBox x:Name="ActionButtonContentComboBox" SelectedIndex="0" Grid.Row="13">
Expand Down
46 changes: 30 additions & 16 deletions dev/TeachingTip/TestUI/TeachingTipPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,6 @@ public void OnSetIconButtonClicked(object sender, RoutedEventArgs args)
SymbolIconSource symbolIconSource = new SymbolIconSource();
symbolIconSource.Symbol = Symbol.People;
getTeachingTip().IconSource = symbolIconSource;

}
else
{
Expand Down Expand Up @@ -325,6 +324,23 @@ public void OnSetContentButtonClicked(object sender, RoutedEventArgs args)
image.Source = bitmapImage;
getTeachingTip().Content = image;
}
else if (this.ContentComboBox.SelectedItem == ContentButton)
{
Button button = new Button() { Content = "Content Button" };
button.GotFocus += (s, e) =>
{
if (button.FocusState == FocusState.Keyboard)
{
AddToMessageBoard("Content Button GotKeyboardFocus Event");
}
};
button.LostFocus += (s, e) => AddToMessageBoard("Content Button LostFocus Event");

StackPanel panel = new StackPanel() { Orientation = Orientation.Horizontal };
panel.Children.Add(button);

getTeachingTip().Content = panel;
}
else
{
getTeachingTip().Content = null;
Expand Down Expand Up @@ -828,20 +844,12 @@ public void OnTipShadowUnchecked(object sender, RoutedEventArgs args)

public void OnTeachingTipClosed(object sender, TeachingTipClosedEventArgs args)
{
if (lstTeachingTipEvents != null)
{
lstTeachingTipEvents.Items.Add(lstTeachingTipEvents.Items.Count.ToString() + ") " + args.ToString() + " Reason: " + args.Reason.ToString());
lstTeachingTipEvents.ScrollIntoView(lstTeachingTipEvents.Items.Last<object>());
}
AddToMessageBoard(args.ToString() + " Reason: " + args.Reason.ToString());
}

public void OnTeachingTipClosing(TeachingTip sender, TeachingTipClosingEventArgs args)
{
if (lstTeachingTipEvents != null)
{
lstTeachingTipEvents.Items.Add(lstTeachingTipEvents.Items.Count.ToString() + ") " + args.ToString() + " Reason: " + args.Reason.ToString());
lstTeachingTipEvents.ScrollIntoView(lstTeachingTipEvents.Items.Last<object>());
}
AddToMessageBoard(args.ToString() + " Reason: " + args.Reason.ToString());

CheckBox cancelClosesCheckBox = null;
if (sender == TeachingTipInResources)
Expand Down Expand Up @@ -872,14 +880,21 @@ private void Timer_Tick(object sender, object e)

public void OnTeachingTipActionButtonClicked(object sender, object args)
{
lstTeachingTipEvents.Items.Add(lstTeachingTipEvents.Items.Count.ToString() + ") " + "Action Button Clicked Event");
lstTeachingTipEvents.ScrollIntoView(lstTeachingTipEvents.Items.Last<object>());
AddToMessageBoard("Action Button Clicked Event");
}

public void OnTeachingTipCloseButtonClicked(object sender, object args)
{
lstTeachingTipEvents.Items.Add(lstTeachingTipEvents.Items.Count.ToString() + ") " + "Close Button Clicked Event");
lstTeachingTipEvents.ScrollIntoView(lstTeachingTipEvents.Items.Last<object>());
AddToMessageBoard("Close Button Clicked Event");
}

private void AddToMessageBoard(string message)
{
if (lstTeachingTipEvents != null)
{
lstTeachingTipEvents.Items.Add(lstTeachingTipEvents.Items.Count.ToString() + ") " + message);
lstTeachingTipEvents.ScrollIntoView(lstTeachingTipEvents.Items.Last<object>());
}
}

private void BtnClearTeachingTipEvents_Click(object sender, RoutedEventArgs e)
Expand Down Expand Up @@ -988,6 +1003,5 @@ private void RemoveOpenButton_Click(object sender, RoutedEventArgs e)
{
ContentStackPanel.Children.Remove(targetButton);
}

}
}