Exclude MoreButton if Not using PrimaryCommands #6044

karkarl · 2021-10-06T01:06:50Z

Description

If no PrimaryCommands is present, the SeeMore button is still visible and focusable, causing keyboard accessibility issues.

The fix is to check if PrimaryCommands().Size() > 0 at PopulateAccessibleControls() on PrimaryCommands().VectorChanged(). If size is 0, the SeeMore button is excluded from being populated in the horizontally and vertically accessible controls member variable, and is no longer accessible.

Motivation and Context

Fixes #5857 and internal bug 35345814

How Has This Been Tested?

ranjeshj · 2021-10-06T19:51:31Z

@karenbtlai can you please add a test ?

karkarl · 2021-10-20T00:06:42Z

/azp run

azure-pipelines · 2021-10-20T00:06:52Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2021-10-25T20:41:35Z

dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp

@@ -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)


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?

StephenLPeters · 2021-10-25T20:42:23Z

dev/CommandBarFlyout/InteractionTests/CommandBarFlyoutTests.cs

+                var favoriteToggleElement = AutomationElement.FocusedElement;
+                Verify.AreEqual(favoriteToggleElement.Current.AutomationId, favoriteToggleButton6.AutomationId);
+
+                Log.Comment("Press Down key to make sure commands loops back focus to first secondary command: Redo.");


Can we make this test faster by simply pressing the up key from the first element and making sure it goes to the last and not the more button?

karkarl · 2021-10-26T04:09:57Z

/azp run

azure-pipelines · 2021-10-26T04:10:12Z

Azure Pipelines successfully started running 1 pipeline(s).

karkarl · 2021-10-27T22:05:12Z

/azp run

azure-pipelines · 2021-10-27T22:05:22Z

Azure Pipelines successfully started running 1 pipeline(s).

karkarl · 2021-10-28T20:59:01Z

/azp run

azure-pipelines · 2021-10-28T20:59:13Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2021-10-29T19:26:02Z

dev/CommandBarFlyout/InteractionTests/CommandBarFlyoutTests.cs

+
+        [TestMethod]
+        public void VerifyAddRemovePrimaryCommands()
+        {


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.

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.

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

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.

karkarl · 2021-11-03T21:00:34Z

/azp run

azure-pipelines · 2021-11-03T21:00:46Z

Azure Pipelines successfully started running 1 pipeline(s).

karkarl · 2021-11-06T01:24:21Z

/azp run

azure-pipelines · 2021-11-06T01:24:33Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters

Make sure that the WidthExpansionDelta change makes sense.

StephenLPeters · 2021-11-08T20:04:39Z

dev/CommandBarFlyout/InteractionTests/CommandBarFlyoutTests.cs

+
+        [TestMethod]
+        public void VerifyAddRemovePrimaryCommands()
+        {


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.

StephenLPeters · 2021-11-08T20:04:58Z

dev/CommandBarFlyout/CommandBarFlyoutCommandBar.cpp

@@ -678,7 +678,7 @@ void CommandBarFlyoutCommandBar::UpdateTemplateSettings()
            collapsedWidth = static_cast<float>(expandedWidth);
        }

-        flyoutTemplateSettings->WidthExpansionDelta(collapsedWidth - expandedWidth);
+        flyoutTemplateSettings->WidthExpansionDelta(0);


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

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.

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.

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

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.

exclude MoreButton if not using primaryCommands

0f30807

ghost added the needs-triage Issue needs to be triaged by the area owners label Oct 6, 2021

ranjeshj added team-Controls Issue for the Controls team area-Commanding AppBar, UICommand, MVVM, etc and removed needs-triage Issue needs to be triaged by the area owners labels Oct 6, 2021

ranjeshj requested a review from llongley October 6, 2021 19:51

karkarl added 3 commits October 19, 2021 13:20

workaround for commandBarFlyout no primary keyboard focus

7c4bceb

new test wip

253fa2b

add new test

22e0742

StephenLPeters reviewed Oct 25, 2021

View reviewed changes

add new test case

40e97fa

fix failing rs2 test

aa7c4ce

fix failing tests

69ff35c

llongley approved these changes Oct 29, 2021

View reviewed changes

StephenLPeters suggested changes Oct 29, 2021

View reviewed changes

update tests

330c5f7

karkarl added 2 commits November 3, 2021 17:29

set up test for dynamic primary commands scenario

8780178

add dynamic add primary command test

a672e23

StephenLPeters approved these changes Nov 8, 2021

View reviewed changes

karkarl merged commit b29a29b into main Nov 9, 2021

karkarl deleted the user/karenlai/CommandBarFlyoutFocusFix branch November 9, 2021 19:40

karkarl mentioned this pull request Nov 18, 2021

Revert WidthExpansionDelta #6343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude MoreButton if Not using PrimaryCommands #6044

Exclude MoreButton if Not using PrimaryCommands #6044

karkarl commented Oct 6, 2021 •

edited

Loading

ranjeshj commented Oct 6, 2021

karkarl commented Oct 20, 2021

azure-pipelines bot commented Oct 20, 2021

StephenLPeters Oct 25, 2021

karkarl Oct 26, 2021

StephenLPeters Oct 25, 2021

karkarl Oct 26, 2021 •

edited

Loading

karkarl commented Oct 26, 2021

azure-pipelines bot commented Oct 26, 2021

karkarl commented Oct 27, 2021

azure-pipelines bot commented Oct 27, 2021

karkarl commented Oct 28, 2021

azure-pipelines bot commented Oct 28, 2021

StephenLPeters Oct 29, 2021

karkarl Nov 3, 2021

karkarl Nov 6, 2021

StephenLPeters Nov 8, 2021

karkarl commented Nov 3, 2021

azure-pipelines bot commented Nov 3, 2021

karkarl commented Nov 6, 2021

azure-pipelines bot commented Nov 6, 2021

StephenLPeters left a comment

StephenLPeters Nov 8, 2021

StephenLPeters Nov 8, 2021

llongley Nov 9, 2021

karkarl Nov 9, 2021

StephenLPeters Nov 15, 2021

karkarl Nov 15, 2021

Exclude MoreButton if Not using PrimaryCommands #6044

Exclude MoreButton if Not using PrimaryCommands #6044

Conversation

karkarl commented Oct 6, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

ranjeshj commented Oct 6, 2021

karkarl commented Oct 20, 2021

azure-pipelines bot commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karkarl Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

karkarl commented Oct 26, 2021

azure-pipelines bot commented Oct 26, 2021

karkarl commented Oct 27, 2021

azure-pipelines bot commented Oct 27, 2021

karkarl commented Oct 28, 2021

azure-pipelines bot commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karkarl commented Nov 3, 2021

azure-pipelines bot commented Nov 3, 2021

karkarl commented Nov 6, 2021

azure-pipelines bot commented Nov 6, 2021

StephenLPeters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karkarl commented Oct 6, 2021 •

edited

Loading

karkarl Oct 26, 2021 •

edited

Loading