From de7ddff2f1e5e228a698293b888575d046f8198d Mon Sep 17 00:00:00 2001 From: Dilip Ojha Date: Tue, 21 Apr 2020 15:08:44 -0700 Subject: [PATCH] HNav Keyboarding & Gamepad updates + visual state fix (#2271) * fixed arrow keyboard navigation * fixed gamepad expanding * deleted tab last focused item logic * fixed chevron on load states * keyboard focus impl * fixed merge issue * addressed comments * fixed and added test * disabled new arrow key test for rs2 --- dev/NavigationView/NavigationView.cpp | 149 ++++++++++-------- dev/NavigationView/NavigationView.h | 8 +- dev/NavigationView/NavigationView.xaml | 7 +- dev/NavigationView/NavigationViewItem.cpp | 58 ++++--- .../NavigationViewItemRevokers.h | 1 - .../NavigationViewTests.cs | 120 +++++++++++++- .../NavigationView_rs1_themeresources.xaml | 45 +++++- 7 files changed, 277 insertions(+), 111 deletions(-) diff --git a/dev/NavigationView/NavigationView.cpp b/dev/NavigationView/NavigationView.cpp index b1c6d38c05..3ce1268f62 100644 --- a/dev/NavigationView/NavigationView.cpp +++ b/dev/NavigationView/NavigationView.cpp @@ -108,7 +108,6 @@ void NavigationView::UnhookEventsAndClearFields(bool isFromDestructor) m_settingsItemTappedRevoker.revoke(); m_settingsItemKeyDownRevoker.revoke(); - m_settingsItemKeyUpRevoker.revoke(); m_settingsItem.set(nullptr); m_paneSearchButtonClickRevoker.revoke(); @@ -130,13 +129,11 @@ void NavigationView::UnhookEventsAndClearFields(bool isFromDestructor) m_leftNavItemsRepeaterElementPreparedRevoker.revoke(); m_leftNavItemsRepeaterElementClearingRevoker.revoke(); m_leftNavRepeaterLoadedRevoker.revoke(); - m_leftNavRepeaterGettingFocusRevoker.revoke(); m_leftNavRepeater.set(nullptr); m_topNavItemsRepeaterElementPreparedRevoker.revoke(); m_topNavItemsRepeaterElementClearingRevoker.revoke(); m_topNavRepeaterLoadedRevoker.revoke(); - m_topNavRepeaterGettingFocusRevoker.revoke(); m_topNavRepeater.set(nullptr); m_topNavOverflowItemsRepeaterElementPreparedRevoker.revoke(); @@ -384,7 +381,6 @@ void NavigationView::OnApplyTemplate() m_leftNavItemsRepeaterElementPreparedRevoker = leftNavRepeater.ElementPrepared(winrt::auto_revoke, { this, &NavigationView::OnRepeaterElementPrepared }); m_leftNavItemsRepeaterElementClearingRevoker = leftNavRepeater.ElementClearing(winrt::auto_revoke, { this, &NavigationView::OnRepeaterElementClearing }); - m_leftNavRepeaterGettingFocusRevoker = leftNavRepeater.GettingFocus(winrt::auto_revoke, { this, &NavigationView::OnRepeaterGettingFocus }); m_leftNavRepeaterLoadedRevoker = leftNavRepeater.Loaded(winrt::auto_revoke, { this, &NavigationView::OnRepeaterLoaded }); @@ -405,7 +401,6 @@ void NavigationView::OnApplyTemplate() m_topNavItemsRepeaterElementPreparedRevoker = topNavRepeater.ElementPrepared(winrt::auto_revoke, { this, &NavigationView::OnRepeaterElementPrepared }); m_topNavItemsRepeaterElementClearingRevoker = topNavRepeater.ElementClearing(winrt::auto_revoke, { this, &NavigationView::OnRepeaterElementClearing }); - m_topNavRepeaterGettingFocusRevoker = topNavRepeater.GettingFocus(winrt::auto_revoke, { this, &NavigationView::OnRepeaterGettingFocus }); m_topNavRepeaterLoadedRevoker = topNavRepeater.Loaded(winrt::auto_revoke, { this, &NavigationView::OnRepeaterLoaded }); @@ -943,7 +938,6 @@ void NavigationView::OnRepeaterElementPrepared(const winrt::ItemsRepeater& ir, c auto nviRevokers = winrt::make_self(); nviRevokers->tappedRevoker = nvi.Tapped(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemTapped }); nviRevokers->keyDownRevoker = nvi.KeyDown(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemKeyDown }); - nviRevokers->keyUpRevoker = nvi.KeyUp(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemKeyUp }); nviRevokers->gotFocusRevoker = nvi.GotFocus(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemOnGotFocus }); nviRevokers->isSelectedRevoker = RegisterPropertyChanged(nvi, winrt::NavigationViewItemBase::IsSelectedProperty(), { this, &NavigationView::OnNavigationViewItemIsSelectedPropertyChanged }); nviRevokers->isExpandedRevoker = RegisterPropertyChanged(nvi, winrt::NavigationViewItem::IsExpandedProperty(), { this, &NavigationView::OnNavigationViewItemExpandedPropertyChanged }); @@ -1011,12 +1005,10 @@ void NavigationView::CreateAndHookEventsToSettings(std::wstring_view settingsNam m_settingsItemTappedRevoker.revoke(); m_settingsItemKeyDownRevoker.revoke(); - m_settingsItemKeyUpRevoker.revoke(); m_settingsItem.set(settingsItem); m_settingsItemTappedRevoker = settingsItem.Tapped(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemTapped }); m_settingsItemKeyDownRevoker = settingsItem.KeyDown(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemKeyDown }); - m_settingsItemKeyUpRevoker = settingsItem.KeyUp(winrt::auto_revoke, { this, &NavigationView::OnNavigationViewItemKeyUp }); auto nvibImpl = winrt::get_self(settingsItem); nvibImpl->SetNavigationViewParent(*this); @@ -2147,18 +2139,6 @@ void NavigationView::OnNavigationViewItemTapped(const winrt::IInspectable& sende } } -void NavigationView::OnNavigationViewItemKeyUp(const winrt::IInspectable& sender, const winrt::KeyRoutedEventArgs& args) -{ - // Because ListViewItem eats the events, we only get these keys on KeyUp. - if (args.OriginalKey() == winrt::VirtualKey::GamepadA) - { - if (auto nvi = sender.try_as()) - { - HandleKeyEventForNavigationViewItem(nvi, args); - } - } -} - void NavigationView::OnNavigationViewItemKeyDown(const winrt::IInspectable& sender, const winrt::KeyRoutedEventArgs& args) { if (auto nvi = sender.try_as()) @@ -2172,7 +2152,6 @@ void NavigationView::HandleKeyEventForNavigationViewItem(const winrt::Navigation auto key = args.Key(); if (IsSettingsItem(nvi)) { - // Because ListViewItem eats the events, we only get these keys on KeyDown. if (key == winrt::VirtualKey::Space || key == winrt::VirtualKey::Enter) { @@ -2197,6 +2176,90 @@ void NavigationView::HandleKeyEventForNavigationViewItem(const winrt::Navigation args.Handled(true); KeyboardFocusLastItemFromItem(nvi); break; + case winrt::VirtualKey::Down: + FocusNextDownItem(nvi, args); + break; + case winrt::VirtualKey::Up: + FocusNextUpItem(nvi, args); + break; + } + } +} + +void NavigationView::FocusNextUpItem(const winrt::NavigationViewItem& nvi, const winrt::KeyRoutedEventArgs& args) +{ + if (args.OriginalSource() != nvi) + { + return; + } + + bool shouldHandleFocus = true; + auto const nviImpl = winrt::get_self(nvi); + auto const nextFocusableElement = winrt::FocusManager::FindNextFocusableElement(winrt::FocusNavigationDirection::Up); + + if (auto const nextFocusableNVI = nextFocusableElement.try_as()) + { + + auto const nextFocusableNVIImpl = winrt::get_self(nextFocusableNVI); + + if (nextFocusableNVIImpl->Depth() == nviImpl->Depth()) + { + // If we not at the top of the list for our current depth and the item above us has children, check whether we should move focus onto a child + if (DoesNavigationViewItemHaveChildren(nextFocusableNVI)) + { + // Focus on last lowest level visible container + if (auto const childRepeater = nextFocusableNVIImpl->GetRepeater()) + { + if (auto const lastFocusableElement = winrt::FocusManager::FindLastFocusableElement(childRepeater)) + { + if (auto lastFocusableNVI = lastFocusableElement.try_as()) + { + args.Handled(lastFocusableNVI.Focus(winrt::FocusState::Keyboard)); + } + } + else + { + args.Handled(nextFocusableNVIImpl->Focus(winrt::FocusState::Keyboard)); + } + + } + } + else + { + // Traversing up a list where XYKeyboardFocus will result in correct behavior + shouldHandleFocus = false; + } + } + } + + // We are at the top of the list, focus on parent + if (shouldHandleFocus && !args.Handled() && nviImpl->Depth() > 0) + { + if (auto const parentContainer = GetParentNavigationViewItemForContainer(nvi)) + { + args.Handled(parentContainer.Focus(winrt::FocusState::Keyboard)); + } + } +} + +// If item has focusable children, move focus to first focusable child, otherise just defer to default XYKeyboardFocus behavior +void NavigationView::FocusNextDownItem(const winrt::NavigationViewItem& nvi, const winrt::KeyRoutedEventArgs& args) +{ + if (args.OriginalSource() != nvi) + { + return; + } + + if (DoesNavigationViewItemHaveChildren(nvi)) + { + auto const nviImpl = winrt::get_self(nvi); + if (auto const childRepeater = nviImpl->GetRepeater()) + { + auto const firstFocusableElement = winrt::FocusManager::FindFirstFocusableElement(childRepeater); + if (auto controlFirst = firstFocusableElement.try_as()) + { + args.Handled(controlFirst.Focus(winrt::FocusState::Keyboard)); + } } } } @@ -2259,54 +2322,10 @@ void NavigationView::KeyboardFocusLastItemFromItem(const winrt::NavigationViewIt } } -void NavigationView::OnRepeaterGettingFocus(const winrt::IInspectable& sender, const winrt::GettingFocusEventArgs& args) -{ - if (args.InputDevice() == winrt::FocusInputDeviceKind::Keyboard) - { - if (auto const oldFocusedElement = args.OldFocusedElement()) - { - auto const oldElementParent = winrt::VisualTreeHelper::GetParent(oldFocusedElement); - auto const rootRepeater = [this]() - { - if (IsTopNavigationView()) - { - return m_topNavRepeater.get(); - } - return m_leftNavRepeater.get(); - }(); - // If focus is coming from outside the root repeater, put focus on last focused item - if (rootRepeater != oldElementParent) - { - if (auto const argsAsIGettingFocusEventArgs2 = args.try_as()) - { - if (auto const lastFocusedNvi = rootRepeater.TryGetElement(m_indexOfLastFocusedItem)) - { - if (argsAsIGettingFocusEventArgs2.TrySetNewFocusedElement(lastFocusedNvi)) - { - args.Handled(true); - } - } - } - } - } - } -} - void NavigationView::OnNavigationViewItemOnGotFocus(const winrt::IInspectable& sender, winrt::RoutedEventArgs const& e) { if (auto nvi = sender.try_as()) { - // Store index of last focused item in order to get proper focus behavior. - // No need to keep track of last focused item if the item is in the overflow menu. - m_indexOfLastFocusedItem = [this, nvi]() - { - if (IsTopNavigationView()) - { - return m_topNavRepeater.get().GetElementIndex(nvi);; - } - return m_leftNavRepeater.get().GetElementIndex(nvi); - }(); - // Achieve selection follows focus behavior if (IsNavigationViewListSingleSelectionFollowsFocus()) { diff --git a/dev/NavigationView/NavigationView.h b/dev/NavigationView/NavigationView.h index 29333617fa..35a91976d9 100644 --- a/dev/NavigationView/NavigationView.h +++ b/dev/NavigationView/NavigationView.h @@ -257,7 +257,6 @@ class NavigationView : void OnNavigationViewItemTapped(const winrt::IInspectable& sender, const winrt::TappedRoutedEventArgs& args); void OnNavigationViewItemKeyDown(const winrt::IInspectable& sender, const winrt::KeyRoutedEventArgs& args); - void OnNavigationViewItemKeyUp(const winrt::IInspectable& sender, const winrt::KeyRoutedEventArgs& args); void OnNavigationViewItemOnGotFocus(const winrt::IInspectable& sender, const winrt::RoutedEventArgs& e); void OnNavigationViewItemExpandedPropertyChanged(const winrt::DependencyObject& sender, const winrt::DependencyProperty& args); @@ -320,6 +319,8 @@ class NavigationView : bool ShouldShowFocusVisual(); void KeyboardFocusFirstItemFromItem(const winrt::NavigationViewItemBase& nvib); void KeyboardFocusLastItemFromItem(const winrt::NavigationViewItemBase& nvib); + void FocusNextDownItem(const winrt::NavigationViewItem& nvi, const winrt::KeyRoutedEventArgs& args); + void FocusNextUpItem(const winrt::NavigationViewItem& nvi, const winrt::KeyRoutedEventArgs& args); void ApplyCustomMenuItemContainerStyling(const winrt::NavigationViewItemBase& nvib, const winrt::ItemsRepeater& ir, int index); com_ptr m_navigationViewItemsFactory{ nullptr }; @@ -377,7 +378,6 @@ class NavigationView : winrt::Button::Click_revoker m_paneToggleButtonClickRevoker{}; winrt::UIElement::Tapped_revoker m_settingsItemTappedRevoker{}; winrt::UIElement::KeyDown_revoker m_settingsItemKeyDownRevoker{}; - winrt::UIElement::KeyUp_revoker m_settingsItemKeyUpRevoker{}; winrt::Button::Click_revoker m_paneSearchButtonClickRevoker{}; winrt::CoreApplicationViewTitleBar::LayoutMetricsChanged_revoker m_titleBarMetricsChangedRevoker{}; winrt::CoreApplicationViewTitleBar::IsVisibleChanged_revoker m_titleBarIsVisibleChangedRevoker{}; @@ -396,12 +396,10 @@ class NavigationView : winrt::ItemsRepeater::ElementPrepared_revoker m_leftNavItemsRepeaterElementPreparedRevoker{}; winrt::ItemsRepeater::ElementClearing_revoker m_leftNavItemsRepeaterElementClearingRevoker{}; winrt::ItemsRepeater::Loaded_revoker m_leftNavRepeaterLoadedRevoker{}; - winrt::ItemsRepeater::GettingFocus_revoker m_leftNavRepeaterGettingFocusRevoker{}; winrt::ItemsRepeater::ElementPrepared_revoker m_topNavItemsRepeaterElementPreparedRevoker{}; winrt::ItemsRepeater::ElementClearing_revoker m_topNavItemsRepeaterElementClearingRevoker{}; winrt::ItemsRepeater::Loaded_revoker m_topNavRepeaterLoadedRevoker{}; - winrt::ItemsRepeater::GettingFocus_revoker m_topNavRepeaterGettingFocusRevoker{}; winrt::ItemsRepeater::ElementPrepared_revoker m_topNavOverflowItemsRepeaterElementPreparedRevoker{}; winrt::ItemsRepeater::ElementClearing_revoker m_topNavOverflowItemsRepeaterElementClearingRevoker{}; @@ -443,8 +441,6 @@ class NavigationView : // 2 and 3 are internal implementation and will call by ClosePane/OpenPane. the flag is to indicate 1 if it's false bool m_isOpenPaneForInteraction{ false }; - int32_t m_indexOfLastFocusedItem{ -1 }; - bool m_moveTopNavOverflowItemOnFlyoutClose{ false }; }; diff --git a/dev/NavigationView/NavigationView.xaml b/dev/NavigationView/NavigationView.xaml index aad8e00dc0..1c449c3ce5 100644 --- a/dev/NavigationView/NavigationView.xaml +++ b/dev/NavigationView/NavigationView.xaml @@ -441,8 +441,9 @@ HorizontalAlignment="Stretch" VerticalAlignment="Top"> - @@ -531,6 +532,7 @@ + @@ -595,7 +597,8 @@ ContentTemplateSelector="{TemplateBinding ContentTemplateSelector}" Content="{TemplateBinding Content}" primitiveContract7Present:CornerRadius="{TemplateBinding CornerRadius}" - IsTabStop="False"> + IsTabStop="false" + Control.IsTemplateFocusTarget="True"> m_rootGrid.get()); - }); - } - else - { - if (auto const flyout = winrt::FlyoutBase::GetAttachedFlyout(m_rootGrid.get())) + // There seems to be a race condition happening which sometimes + // prevents the opening of the flyout. Queue callback as a workaround. + SharedHelpers::QueueCallbackForCompositionRendering( + [strongThis = get_strong()]() + { + winrt::FlyoutBase::ShowAttachedFlyout(strongThis->m_rootGrid.get()); + }); + } + else { - flyout.Hide(); + if (auto const flyout = winrt::FlyoutBase::GetAttachedFlyout(m_rootGrid.get())) + { + flyout.Hide(); + } } } } diff --git a/dev/NavigationView/NavigationViewItemRevokers.h b/dev/NavigationView/NavigationViewItemRevokers.h index 624cab7a61..e7592189f9 100644 --- a/dev/NavigationView/NavigationViewItemRevokers.h +++ b/dev/NavigationView/NavigationViewItemRevokers.h @@ -8,7 +8,6 @@ class NavigationViewItemRevokers : public winrt::implements - + + + + + + + + @@ -759,9 +766,16 @@ - + + + + + + + + @@ -1061,9 +1075,16 @@ - + + + + + + + + @@ -1213,9 +1234,16 @@ - + + + + + + + + @@ -1380,9 +1408,16 @@ - + + + + + + + +