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

Exclude MoreButton if Not using PrimaryCommands #6044

Merged
merged 10 commits into from
Nov 9, 2021
Merged

Exclude MoreButton if Not using PrimaryCommands #6044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f30807
exclude MoreButton if not using primaryCommands
karkarl Oct 6, 2021
7c4bceb
workaround for commandBarFlyout no primary keyboard focus
karkarl Oct 19, 2021
253fa2b
new test wip
karkarl Oct 19, 2021
22e0742
add new test
karkarl Oct 20, 2021
40e97fa
add new test case
karkarl Oct 26, 2021
aa7c4ce
fix failing rs2 test
karkarl Oct 27, 2021
69ff35c
fix failing tests
karkarl Oct 28, 2021
330c5f7
update tests
karkarl Nov 3, 2021
8780178
set up test for dynamic primary commands scenario
karkarl Nov 4, 2021
a672e23
add dynamic add primary command test
karkarl Nov 6, 2021
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
13 changes: 9 additions & 4 deletions dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ void CommandBarFlyoutCommandBar::UpdateTemplateSettings()
collapsedWidth = static_cast<float>(expandedWidth);
}

flyoutTemplateSettings->WidthExpansionDelta(collapsedWidth - expandedWidth);
flyoutTemplateSettings->WidthExpansionDelta(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change, but if @llongley or someone else does that is good enough.

Copy link
Member

Choose a reason for hiding this comment

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

I thought there was a need for a nonzero value for this, but Karen said that she tested all of the scenarios and they all continue to look right, so if that's the case it seems OK to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When PrimaryCommands are being loaded dynamically, it causes a weird crop issue that is caused by a negative WidthExpansionDelta. Setting it to 0 fixes this specific use case. Upon investigation, it seems a non-zero WidthExpansionDelta is only for when SecondaryCommands is collapsed, and the width of primaryCommands is less than the width of SecondaryCommands, so the flyout expands to the full width when opening the secondaryCommands.

However, setting it to 0 doesn't seem to break that scenario. This may be because CBF no longer has the expanding animation. I manually all scenarios with and without the WidthExpansionDelta change, and it doesn't change or break anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

The CBF does have the expanding animation still though, there is a button for it on the CommandBarFlyout test page labeled "Show expanding CommandBarFlyout"

This is the behavior before this change:

PreChange.mp4

and this is the behavior after the change

PostChange.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I now see a subtle expanding animation that is missing from the update. I think the appropriate action is to revert the hardcoding of ExpansionWidthDelta, and fix the dynamically adding primary buttons in a separate fix. It is beyond the scope of the original accessibility bug to begin with.

flyoutTemplateSettings->WidthExpansionAnimationStartPosition(-flyoutTemplateSettings->WidthExpansionDelta() / 2.0);
flyoutTemplateSettings->WidthExpansionAnimationEndPosition(-flyoutTemplateSettings->WidthExpansionDelta());
flyoutTemplateSettings->ContentClipRect({ 0, 0, static_cast<float>(expandedWidth), primaryItemsRootDesiredSize.Height });
Expand Down Expand Up @@ -896,7 +896,9 @@ void CommandBarFlyoutCommandBar::PopulateAccessibleControls()
m_verticallyAccessibleControls.Clear();
}

for (winrt::ICommandBarElement const& command : PrimaryCommands())
const auto primaryCommands = PrimaryCommands();

for (winrt::ICommandBarElement const& command : primaryCommands)
{
if (auto const& commandAsControl = command.try_as<winrt::Control>())
{
Expand All @@ -907,8 +909,11 @@ void CommandBarFlyoutCommandBar::PopulateAccessibleControls()

if (auto const& moreButton = m_moreButton.get())
{
m_horizontallyAccessibleControls.Append(moreButton);
m_verticallyAccessibleControls.Append(moreButton);
if (primaryCommands.Size() > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the primary commands size is reduced from 1 to 0? what about if its increased from 0 to 1? do we have the appropriate code in place to make sure that the more button is added or removed based on this? Can we add a test for these scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

{
m_horizontallyAccessibleControls.Append(moreButton);
m_verticallyAccessibleControls.Append(moreButton);
}
}

for (winrt::ICommandBarElement const& command : SecondaryCommands())
Expand Down
92 changes: 92 additions & 0 deletions dev/CommandBarFlyout/InteractionTests/CommandBarFlyoutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,5 +992,97 @@ public void VerifyDynamicSecondaryCommandLabel()
Verify.AreEqual(finalBoundingRectangle.Height, initialBoundingRectangle.Height);
}
}

[TestMethod]
public void VerifyIsFlyoutKeyboardAccessibleWithNoPrimaryCommands()
{
if (PlatformConfiguration.IsOSVersionLessThan(OSVersion.Redstone2))
{
Log.Warning("Test is disabled pre-RS2 because CommandBarFlyout is not supported pre-RS2");
return;
}

using (var setup = new CommandBarFlyoutTestSetupHelper())
{
Button showCommandBarFlyoutButtonWithNoPrimaryCommands = FindElement.ByName<Button>("Show CommandBarFlyout with no primary commands");

Log.Comment("Tap on a button to show the CommandBarFlyout.");
showCommandBarFlyoutButtonWithNoPrimaryCommands.InvokeAndWait();

Button undoButton6 = FindElement.ById<Button>("UndoButton6");
var undoButtonElement = AutomationElement.FocusedElement;
Verify.AreEqual(undoButtonElement.Current.AutomationId, undoButton6.AutomationId);

KeyboardHelper.PressKey(Key.Up);
Wait.ForIdle();

if (PlatformConfiguration.IsOSVersionLessThan(OSVersion.Redstone3))
{
// rs2 does not loop through menuflyout items, so focus stays on the first button.
Log.Comment("Press Up key to make sure it does not go to hidden controls.");
Verify.AreEqual(undoButtonElement.Current.AutomationId, undoButton6.AutomationId);
}
else
{
Log.Comment("Press Up key to make sure commands loops focus to last secondary command: Favorite.");
Button favoriteToggleButton6 = FindElement.ById<Button>("FavoriteToggleButton6");
var favoriteToggleElement = AutomationElement.FocusedElement;
Verify.AreEqual(favoriteToggleElement.Current.AutomationId, favoriteToggleButton6.AutomationId);
}
}
}

[TestMethod]
public void VerifyAddPrimaryCommandsDynamically()
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't test the removal of a primary command, only the addition of one.

More importantly, I wouldn't expect the behavior to fail when you change the number of commands in the CBF while it is closed. The bug I called out in the product code would only happen if you change the number of commands while the flyout is open.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a scenario where the number of commands will change while the flyout is open, since CBF is a flyout and transient by design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. Test case is updated to be adding PrimaryCommands dynamically.

Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness removing the last primary command could also be tested. It uses what looks like the same code path though so maybe it is fine.

if (PlatformConfiguration.IsOSVersionLessThan(OSVersion.Redstone2))
{
Log.Warning("Test is disabled pre-RS2 because CommandBarFlyout is not supported pre-RS2");
return;
}

using (var setup = new CommandBarFlyoutTestSetupHelper())
{
Button showCommandBarFlyoutButton = FindElement.ByName<Button>("Show CommandBarFlyout with no primary commands");
ToggleButton addPrimaryCommandDynamicallyCheckBox = FindElement.ById<ToggleButton>("AddPrimaryCommandDynamicallyCheckBox");
ToggleButton clearPrimaryCommandsCheckBox = FindElement.ById<ToggleButton>("ClearPrimaryCommandsCheckBox");
ToggleButton primaryCommandDynamicallyAddedCheckBox = FindElement.ById<ToggleButton>("PrimaryCommandDynamicallyAddedCheckBox");
Edit dynamicLabelTimerIntervalTextBox = new Edit(FindElement.ById("DynamicLabelTimerIntervalTextBox"));
Edit dynamicLabelChangeCountTextBox = new Edit(FindElement.ById("DynamicLabelChangeCountTextBox"));

Log.Comment("Setting DynamicLabelTimerIntervalTextBox to 1s");
dynamicLabelTimerIntervalTextBox.SetValue("1000");

Log.Comment("Setting DynamicLabelChangeCountTextBox to 1 single change");
dynamicLabelChangeCountTextBox.SetValue("1");
Wait.ForIdle();

Log.Comment("Set Flyout6 to add Primary Commands dynamically");
addPrimaryCommandDynamicallyCheckBox.Check();
Wait.ForIdle();

Log.Comment("Invoke FlyoutTarget 6 to Show CommandBarFlyout with no primary commands");
showCommandBarFlyoutButton.Click();

Log.Comment("Waiting for SecondaryCommandDynamicLabelChangedCheckBox becoming checked indicating the asynchronous Label property change occurred");
primaryCommandDynamicallyAddedCheckBox.GetToggledWaiter().Wait();
Wait.ForIdle();

KeyboardHelper.PressKey(Key.Tab);
Wait.ForIdle();

KeyboardHelper.PressKey(Key.Right);
Wait.ForIdle();

Log.Comment("Verifying Primary Commands is added and MoreButton is actionable");

Button moreButton = FindElement.ById<Button>("MoreButton");
var moreButtonElement = AutomationElement.FocusedElement;
Verify.AreEqual(moreButtonElement.Current.AutomationId, moreButton.AutomationId);

Log.Comment("Dismissing flyout");
KeyboardHelper.PressKey(Key.Escape);
}
}
}
}
10 changes: 9 additions & 1 deletion dev/CommandBarFlyout/TestUI/CommandBarFlyoutPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,20 @@
<Button x:Name="FlyoutTarget3" Content="Show tall CommandBarFlyout" Margin="10" Click="OnFlyoutTarget3Click" />
<Button x:Name="FlyoutTarget4" Content="Show max-width CommandBarFlyout" Margin="10" Click="OnFlyoutTarget4Click" />
<Button x:Name="FlyoutTarget5" Content="Show CommandBarFlyout with sub-menu" Margin="10" Click="OnFlyoutTarget5Click" />
<Button x:Name="FlyoutTarget6" Content="Show CommandBarFlyout with no primary commands" Margin="10" Click="OnFlyoutTarget6Click" />
<StackPanel Orientation="Horizontal">
<Button x:Name="FlyoutTarget6" Content="Show CommandBarFlyout with no primary commands" Margin="10" Click="OnFlyoutTarget6Click" />
<Button x:Name="EditCommandCount6" Content="Add / Remove Primary Command" Click="OnEditCommandCount6Click" />
</StackPanel>
<Button x:Name="FlyoutTarget7" Content="Show CommandBarFlyout with non-focusable primary commands" Margin="10" Click="OnFlyoutTarget7Click" />
<Button x:Name="FlyoutTarget8" Content="Show CommandBarFlyout with no secondary commands" Margin="10" Click="OnFlyoutTarget8Click" />
<Button x:Name="FlyoutTarget9" Content="Show CommandBarFlyout with AlwaysExpanded" Margin="10" Click="OnFlyoutTarget9Click" />
<contract13Present:Button x:Name="FlyoutTarget10" Content="Show Windows.UI.Xaml.Controls.CommandBarFlyout" Margin="10" Click="OnFlyoutTarget10Click" />
<CheckBox x:Name="IsRTLCheckBox" Content="Is Page in RightToLeft FlowDirection?" AutomationProperties.AutomationId="IsRTLCheckBox" Margin="10,10,10,2" Checked="IsRTLCheckBox_Checked" Unchecked="IsRTLCheckBox_Unchecked" />
<StackPanel Orientation="Horizontal" Margin="10,0,10,2">
<CheckBox x:Name="AddPrimaryCommandDynamicallyCheckBox" Content="Add Primary Commands Dynamically?" AutomationProperties.AutomationId="AddPrimaryCommandDynamicallyCheckBox" />
<CheckBox x:Name="PrimaryCommandDynamicallyAddedCheckBox" Content="Primary Commands Added?" AutomationProperties.AutomationId="PrimaryCommandDynamicallyAddedCheckBox" Margin="5,0,0,0" />
</StackPanel>
<CheckBox x:Name="ClearPrimaryCommandsCheckBox" Content="Clear Primary Commands Asynchronously?" AutomationProperties.AutomationId="ClearPrimaryCommandsCheckBox" Margin="10,0,10,2" />
<StackPanel Orientation="Horizontal" Margin="10,0,10,2">
<CheckBox x:Name="UseSecondaryCommandDynamicLabelCheckBox" Content="Use Secondary Command with Dynamic Label?" AutomationProperties.AutomationId="UseSecondaryCommandDynamicLabelCheckBox" />
<CheckBox x:Name="SecondaryCommandDynamicLabelChangedCheckBox" Content="Label Changed?" AutomationProperties.AutomationId="SecondaryCommandDynamicLabelChangedCheckBox" Margin="5,0,0,0" />
Expand Down
86 changes: 85 additions & 1 deletion dev/CommandBarFlyout/TestUI/CommandBarFlyoutPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using Common;
using System;
using System.Linq;
using Windows.Foundation.Metadata;
using Windows.System;
using Windows.UI.Xaml;
Expand All @@ -20,8 +21,13 @@ public sealed partial class CommandBarFlyoutPage : TestPage
private DispatcherTimer clearSecondaryCommandsTimer = new DispatcherTimer();
private CommandBarFlyout clearSecondaryCommandsFlyout;

private DispatcherTimer clearPrimaryCommandsTimer = new DispatcherTimer();
private CommandBarFlyout clearPrimaryCommandsFlyout;

private DispatcherTimer dynamicLabelTimer = new DispatcherTimer();
private DispatcherTimer dynamicCommandTimer = new DispatcherTimer();
private AppBarButton dynamicLabelSecondaryCommand;
private CommandBarFlyout dynamicCommandBarFlyout;
private string originalLabelSecondaryCommand;
private int dynamicLabelChangeCount;

Expand All @@ -31,9 +37,14 @@ public CommandBarFlyoutPage()

dynamicLabelTimer.Tick += DynamicLabelTimer_Tick;

dynamicCommandTimer.Tick += DynamicCommandTimer_Tick;

clearSecondaryCommandsTimer.Interval = new TimeSpan(0, 0, 3 /*sec*/);
clearSecondaryCommandsTimer.Tick += ClearSecondaryCommandsTimer_Tick;

clearSecondaryCommandsTimer.Interval = new TimeSpan(0, 0, 3 /*sec*/);
clearPrimaryCommandsTimer.Tick += ClearPrimaryCommandsTimer_Tick;

if (ApiInformation.IsTypePresent("Windows.UI.Xaml.Input.KeyboardAccelerator"))
{
UndoButton1.KeyboardAccelerators.Add(new KeyboardAccelerator() { Key = VirtualKey.Z, Modifiers = VirtualKeyModifiers.Control });
Expand Down Expand Up @@ -180,8 +191,10 @@ private void ShowFlyoutAt(FlyoutBase flyout, FrameworkElement targetElement, Fly
{
bool useSecondaryCommandDynamicLabel = (bool)UseSecondaryCommandDynamicLabelCheckBox.IsChecked;
bool clearSecondaryCommands = (bool)ClearSecondaryCommandsCheckBox.IsChecked;
bool addPrimaryCommandDynamicallyCheckBox = (bool)AddPrimaryCommandDynamicallyCheckBox.IsChecked;
bool clearPrimaryCommands = (bool)ClearPrimaryCommandsCheckBox.IsChecked;

if (useSecondaryCommandDynamicLabel || clearSecondaryCommands)
if (useSecondaryCommandDynamicLabel || addPrimaryCommandDynamicallyCheckBox || clearSecondaryCommands || clearPrimaryCommands)
{
CommandBarFlyout commandBarFlyout = flyout as CommandBarFlyout;

Expand All @@ -199,6 +212,17 @@ private void ShowFlyoutAt(FlyoutBase flyout, FrameworkElement targetElement, Fly
SetClearSecondaryCommandsFlyout(commandBarFlyout);
}
}

if (addPrimaryCommandDynamicallyCheckBox)
{
dynamicCommandBarFlyout = commandBarFlyout;
SetDynamicPrimaryCommand();
}

if (clearPrimaryCommands && commandBarFlyout.PrimaryCommands != null && commandBarFlyout.PrimaryCommands.Count > 0)
{
SetClearPrimaryCommandsFlyout(commandBarFlyout);
}
}
}

Expand Down Expand Up @@ -227,6 +251,20 @@ private void SetClearSecondaryCommandsFlyout(CommandBarFlyout commandBarFlyout)
}
}

private void SetClearPrimaryCommandsFlyout(CommandBarFlyout commandBarFlyout)
{
if (commandBarFlyout == null)
{
clearPrimaryCommandsFlyout = null;
clearPrimaryCommandsTimer.Stop();
}
else
{
clearPrimaryCommandsFlyout = commandBarFlyout;
clearPrimaryCommandsTimer.Start();
}
}

private void SetDynamicSecondaryCommand(AppBarButton appBarButton)
{
if (appBarButton == null)
Expand Down Expand Up @@ -255,6 +293,13 @@ private void SetDynamicSecondaryCommand(AppBarButton appBarButton)
}
}

private void SetDynamicPrimaryCommand()
{
dynamicCommandTimer.Interval = new TimeSpan(0, 0, 0, 0, int.Parse(DynamicLabelTimerIntervalTextBox.Text) /*msec*/);
dynamicLabelChangeCount = int.Parse(DynamicLabelChangeCountTextBox.Text);
dynamicCommandTimer.Start();
}

private void ClearSecondaryCommandsTimer_Tick(object sender, object e)
{
if (clearSecondaryCommandsFlyout != null)
Expand All @@ -263,6 +308,14 @@ private void ClearSecondaryCommandsTimer_Tick(object sender, object e)
}
}

private void ClearPrimaryCommandsTimer_Tick(object sender, object e)
{
if (clearPrimaryCommandsFlyout != null)
{
clearPrimaryCommandsFlyout.PrimaryCommands.Clear();
}
}

private void DynamicLabelTimer_Tick(object sender, object e)
{
if (dynamicLabelSecondaryCommand != null)
Expand All @@ -287,6 +340,21 @@ private void DynamicLabelTimer_Tick(object sender, object e)
}
}


private void DynamicCommandTimer_Tick(object sender, object e)
{
dynamicCommandBarFlyout.PrimaryCommands.Add(new AppBarButton() {
Content = new TextBlock() { Text = "Test" }
});

PrimaryCommandDynamicallyAddedCheckBox.IsChecked = true;

if (--dynamicLabelChangeCount == 0)
{
dynamicCommandTimer.Stop();
}
}

private void IsRTLCheckBox_Checked(object sender, RoutedEventArgs e)
{
FlowDirection = FlowDirection.RightToLeft;
Expand All @@ -296,5 +364,21 @@ private void IsRTLCheckBox_Unchecked(object sender, RoutedEventArgs e)
{
FlowDirection = FlowDirection.LeftToRight;
}

private void OnEditCommandCount6Click(object sender, RoutedEventArgs e)
{
var flyout6 = Flyout6 as CommandBarFlyout;

if (flyout6.PrimaryCommands.Count() == 0)
{
flyout6.PrimaryCommands.Add(new AppBarButton() {
Content = new TextBlock() { Text = "Test" }
});
}
else
{
flyout6.PrimaryCommands.RemoveAt(0);
}
}
}
}