Avoid CommandBarFlyoutCommandBar clipping after dynamic command bar element change (microsoft#5347)

* 32506205 fix - part 1

* Adding comments

* Adding new regression test

* Applying PR feedback

* Made couple of comments more precise

* Adding #ifdef _DEBUG for traces

* Fixing versions <= RS3

* Fixing RS2/3 KeyboardAcceleratorTextOverrideProperty usage

Author: RBrid

dev/CommandBarFlyout/CommandBarFlyout.cpp

@@ -14,10 +14,34 @@
 bool CommandBarFlyoutTrace::s_IsDebugOutputEnabled{ false };
 bool CommandBarFlyoutTrace::s_IsVerboseDebugOutputEnabled{ false };
 
+// List of AppBarButton dependency properties being listened to for raising the CommandBarFlyoutCommandBar::OnCommandBarElementDependencyPropertyChanged notifications.
+// AppBarButton::IsCompact and AppBarButton::LabelPosition having no effect on an AppBarButton's rendering, when used as a secondary command, they are not present in the list.
+winrt::DependencyProperty CommandBarFlyout::s_appBarButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount]
+{
+    winrt::AppBarButton::IconProperty(),
+    winrt::AppBarButton::LabelProperty(),
+    nullptr
+};
+
+// List of AppBarToggleButton dependency properties being listened to for raising the CommandBarFlyoutCommandBar::OnCommandBarElementDependencyPropertyChanged notifications.
+// AppBarToggleButton::IsCompact and AppBarToggleButton::LabelPosition having no effect on an AppBarToggleButton's rendering, when used as a secondary command, they are not present in the list.
+winrt::DependencyProperty CommandBarFlyout::s_appBarToggleButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount]
+{
+    winrt::AppBarToggleButton::IconProperty(),
+    winrt::AppBarToggleButton::LabelProperty(),
+    nullptr
+};
+
 CommandBarFlyout::CommandBarFlyout()
 {
     __RP_Marker_ClassById(RuntimeProfiler::ProfId_CommandBarFlyout);
 
+    if (SharedHelpers::IsRS4OrHigher() && s_appBarButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount - 1] == nullptr)
+    {
+        s_appBarButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount - 1] = winrt::AppBarButton::KeyboardAcceleratorTextOverrideProperty();
+        s_appBarToggleButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount - 1] = winrt::AppBarToggleButton::KeyboardAcceleratorTextOverrideProperty();
+    }
+
     if (winrt::IFlyoutBase6 thisAsFlyoutBase6 = *this)
     {
         thisAsFlyoutBase6.ShouldConstrainToRootBounds(false);
@@ -65,6 +89,17 @@ CommandBarFlyout::CommandBarFlyout()
                     auto button = element.try_as<winrt::AppBarButton>();
                     auto toggleButton = element.try_as<winrt::AppBarToggleButton>();
 
+                    UnhookCommandBarElementDependencyPropertyChanges(index);
+
+                    if (button)
+                    {
+                        HookAppBarButtonDependencyPropertyChanges(button, index);
+                    }
+                    else if (toggleButton)
+                    {
+                        HookAppBarToggleButtonDependencyPropertyChanges(toggleButton, index);
+                    }
+
                     if (button && !button.Flyout())
                     {
                         m_secondaryButtonClickRevokerByIndexMap[index] = button.Click(winrt::auto_revoke, closeFlyoutFunc);
@@ -91,6 +126,15 @@ CommandBarFlyout::CommandBarFlyout()
                     auto button = element.try_as<winrt::AppBarButton>();
                     auto toggleButton = element.try_as<winrt::AppBarToggleButton>();
 
+                    if (button)
+                    {
+                        HookAppBarButtonDependencyPropertyChanges(button, index);
+                    }
+                    else if (toggleButton)
+                    {
+                        HookAppBarToggleButtonDependencyPropertyChanges(toggleButton, index);
+                    }
+
                     if (button && !button.Flyout())
                     {
                         m_secondaryButtonClickRevokerByIndexMap[index] = button.Click(winrt::auto_revoke, closeFlyoutFunc);
@@ -103,12 +147,15 @@ CommandBarFlyout::CommandBarFlyout()
                     break;
                 }
                 case winrt::CollectionChange::ItemRemoved:
+                    UnhookCommandBarElementDependencyPropertyChanges(index);
+
                     SharedHelpers::EraseIfExists(m_secondaryButtonClickRevokerByIndexMap, index);
                     SharedHelpers::EraseIfExists(m_secondaryToggleButtonCheckedRevokerByIndexMap, index);
                     SharedHelpers::EraseIfExists(m_secondaryToggleButtonUncheckedRevokerByIndexMap, index);
                     break;
                 case winrt::CollectionChange::Reset:
                     SetSecondaryCommandsToCloseWhenExecuted();
+                    HookAllCommandBarElementDependencyPropertyChanges();
                     break;
                 default:
                     MUX_ASSERT(false);
@@ -250,6 +297,8 @@ CommandBarFlyout::~CommandBarFlyout()
 {
     m_primaryCommands.VectorChanged(m_primaryCommandsVectorChangedToken);
     m_secondaryCommands.VectorChanged(m_secondaryCommandsVectorChangedToken);
+
+    UnhookAllCommandBarElementDependencyPropertyChanges();
 }
 
 winrt::IObservableVector<winrt::ICommandBarElement> CommandBarFlyout::PrimaryCommands()
@@ -270,6 +319,7 @@ winrt::Control CommandBarFlyout::CreatePresenter()
     SharedHelpers::CopyVector(m_secondaryCommands, commandBar->SecondaryCommands());
 
     SetSecondaryCommandsToCloseWhenExecuted();
+    HookAllCommandBarElementDependencyPropertyChanges();
 
     winrt::FlyoutPresenter presenter;
     presenter.Background(nullptr);
@@ -407,3 +457,89 @@ tracker_ref<winrt::FlyoutPresenter> CommandBarFlyout::GetPresenter()
 {
     return m_presenter;
 }
+
+void CommandBarFlyout::HookAppBarButtonDependencyPropertyChanges(winrt::AppBarButton const& appBarButton, int index)
+{
+    const auto commandBarElementDependencyPropertiesCount = SharedHelpers::IsRS4OrHigher() ? s_commandBarElementDependencyPropertiesCount : s_commandBarElementDependencyPropertiesCountRS3;
+
+    for (int commandBarElementDependencyPropertyIndex = 0; commandBarElementDependencyPropertyIndex < commandBarElementDependencyPropertiesCount; commandBarElementDependencyPropertyIndex++)
+    {
+        m_propertyChangedRevokersByIndexMap[index][commandBarElementDependencyPropertyIndex] =
+            RegisterPropertyChanged(
+                appBarButton,
+                s_appBarButtonDependencyProperties[commandBarElementDependencyPropertyIndex], { this, &CommandBarFlyout::OnCommandBarElementDependencyPropertyChanged });
+    }
+}
+
+void CommandBarFlyout::HookAppBarToggleButtonDependencyPropertyChanges(winrt::AppBarToggleButton const& appBarToggleButton, int index)
+{
+    const auto commandBarElementDependencyPropertiesCount = SharedHelpers::IsRS4OrHigher() ? s_commandBarElementDependencyPropertiesCount : s_commandBarElementDependencyPropertiesCountRS3;
+
+    for (int commandBarElementDependencyPropertyIndex = 0; commandBarElementDependencyPropertyIndex < commandBarElementDependencyPropertiesCount; commandBarElementDependencyPropertyIndex++)
+    {
+        m_propertyChangedRevokersByIndexMap[index][commandBarElementDependencyPropertyIndex] =
+            RegisterPropertyChanged(
+                appBarToggleButton,
+                s_appBarToggleButtonDependencyProperties[commandBarElementDependencyPropertyIndex], { this, &CommandBarFlyout::OnCommandBarElementDependencyPropertyChanged });
+    }
+}
+
+void CommandBarFlyout::HookAllCommandBarElementDependencyPropertyChanges()
+{
+    UnhookAllCommandBarElementDependencyPropertyChanges();
+
+    for (uint32_t i = 0; i < SecondaryCommands().Size(); i++)
+    {
+        auto element = SecondaryCommands().GetAt(i);
+        auto button = element.try_as<winrt::AppBarButton>();
+        auto toggleButton = element.try_as<winrt::AppBarToggleButton>();
+
+        if (button)
+        {
+            HookAppBarButtonDependencyPropertyChanges(button, i);
+        }
+        else if (toggleButton)
+        {
+            HookAppBarToggleButtonDependencyPropertyChanges(toggleButton, i);
+        }
+    }
+}
+
+void CommandBarFlyout::UnhookCommandBarElementDependencyPropertyChanges(int index, bool eraseRevokers)
+{
+    const auto revokers = m_propertyChangedRevokersByIndexMap.find(index);
+    if (revokers != m_propertyChangedRevokersByIndexMap.end())
+    {
+        const auto commandBarElementDependencyPropertiesCount = SharedHelpers::IsRS4OrHigher() ? s_commandBarElementDependencyPropertiesCount : s_commandBarElementDependencyPropertiesCountRS3;
+
+        for (int commandBarElementDependencyPropertyIndex = 0; commandBarElementDependencyPropertyIndex < commandBarElementDependencyPropertiesCount; commandBarElementDependencyPropertyIndex++)
+        {
+            m_propertyChangedRevokersByIndexMap[index][commandBarElementDependencyPropertyIndex].revoke();
+        }
+
+        if (eraseRevokers)
+        {
+            m_propertyChangedRevokersByIndexMap.erase(revokers);
+        }
+    }
+}
+
+void CommandBarFlyout::UnhookAllCommandBarElementDependencyPropertyChanges()
+{
+    for (auto const& revokers : m_propertyChangedRevokersByIndexMap)
+    {
+        UnhookCommandBarElementDependencyPropertyChanges(revokers.first, false /*eraseRevokers*/);
+    }
+    m_propertyChangedRevokersByIndexMap.clear();
+}
+
+// Let the potential CommandBarFlyoutCommandBar know of the dependency property change so it can adjust its size.
+void CommandBarFlyout::OnCommandBarElementDependencyPropertyChanged(winrt::DependencyObject const& dependencyObject, winrt::DependencyProperty const& dependencyProperty)
+{
+    COMMANDBARFLYOUT_TRACE_VERBOSE(*this, TRACE_MSG_METH, METH_NAME, this);
+
+    if (auto commandBar = winrt::get_self<CommandBarFlyoutCommandBar>(m_commandBar.get()))
+    {
+        commandBar->OnCommandBarElementDependencyPropertyChanged();
+    }
+}

dev/CommandBarFlyout/CommandBarFlyout.h

@@ -30,7 +30,19 @@ class CommandBarFlyout :
     tracker_ref<winrt::CommandBarFlyoutCommandBar> m_commandBar{ this };
 
 private:
+    static constexpr int s_commandBarElementDependencyPropertiesCount{ 3 };
+    static constexpr int s_commandBarElementDependencyPropertiesCountRS3{ 2 };
+
+    static winrt::DependencyProperty s_appBarButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount];
+    static winrt::DependencyProperty s_appBarToggleButtonDependencyProperties[s_commandBarElementDependencyPropertiesCount];
+
     void SetSecondaryCommandsToCloseWhenExecuted();
+    void HookAppBarButtonDependencyPropertyChanges(winrt::AppBarButton const& appBarButton, int index);
+    void HookAppBarToggleButtonDependencyPropertyChanges(winrt::AppBarToggleButton const& appBarToggleButton, int index);
+    void HookAllCommandBarElementDependencyPropertyChanges();
+    void UnhookCommandBarElementDependencyPropertyChanges(int index, bool eraseRevokers = true);
+    void UnhookAllCommandBarElementDependencyPropertyChanges();
+    void OnCommandBarElementDependencyPropertyChanged(winrt::DependencyObject const& dependencyObject, winrt::DependencyProperty const& dependencyProperty);
 
     bool m_alwaysExpanded;
 
@@ -50,6 +62,7 @@ class CommandBarFlyout :
     std::map<int, winrt::ButtonBase::Click_revoker> m_secondaryButtonClickRevokerByIndexMap;
     std::map<int, winrt::ToggleButton::Checked_revoker> m_secondaryToggleButtonCheckedRevokerByIndexMap;
     std::map<int, winrt::ToggleButton::Unchecked_revoker> m_secondaryToggleButtonUncheckedRevokerByIndexMap;
+    std::map<int, PropertyChanged_revoker[s_commandBarElementDependencyPropertiesCount]> m_propertyChangedRevokersByIndexMap;
 
     tracker_ref<winrt::FlyoutPresenter> m_presenter{ this };