Skip to content

Commit

Permalink
HNav Keyboarding & Gamepad updates + visual state fix (#2271)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@ojhad
ojhad authored Apr 21, 2020
1 parent a35af5b commit de7ddff
Show file tree
Hide file tree
Showing 7 changed files with 277 additions and 111 deletions.
149 changes: 84 additions & 65 deletions dev/NavigationView/NavigationView.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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 });

Expand All @@ -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 });

Expand Down Expand Up @@ -943,7 +938,6 @@ void NavigationView::OnRepeaterElementPrepared(const winrt::ItemsRepeater& ir, c
auto nviRevokers = winrt::make_self<NavigationViewItemRevokers>();
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 });
Expand Down Expand Up @@ -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<NavigationViewItem>(settingsItem);
nvibImpl->SetNavigationViewParent(*this);
Expand Down Expand Up @@ -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<winrt::NavigationViewItem>())
{
HandleKeyEventForNavigationViewItem(nvi, args);
}
}
}

void NavigationView::OnNavigationViewItemKeyDown(const winrt::IInspectable& sender, const winrt::KeyRoutedEventArgs& args)
{
if (auto nvi = sender.try_as<winrt::NavigationViewItem>())
Expand All @@ -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)
{
Expand All @@ -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<NavigationViewItem>(nvi);
auto const nextFocusableElement = winrt::FocusManager::FindNextFocusableElement(winrt::FocusNavigationDirection::Up);

if (auto const nextFocusableNVI = nextFocusableElement.try_as<winrt::NavigationViewItem>())
{

auto const nextFocusableNVIImpl = winrt::get_self<NavigationViewItem>(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<winrt::Control>())
{
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<NavigationViewItem>(nvi);
if (auto const childRepeater = nviImpl->GetRepeater())
{
auto const firstFocusableElement = winrt::FocusManager::FindFirstFocusableElement(childRepeater);
if (auto controlFirst = firstFocusableElement.try_as<winrt::Control>())
{
args.Handled(controlFirst.Focus(winrt::FocusState::Keyboard));
}
}
}
}
Expand Down Expand Up @@ -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<winrt::IGettingFocusEventArgs2>())
{
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<winrt::NavigationViewItem>())
{
// 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())
{
Expand Down
8 changes: 2 additions & 6 deletions dev/NavigationView/NavigationView.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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<NavigationViewItemsFactory> m_navigationViewItemsFactory{ nullptr };
Expand Down Expand Up @@ -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{};
Expand All @@ -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{};
Expand Down Expand Up @@ -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 };
};

7 changes: 5 additions & 2 deletions dev/NavigationView/NavigationView.xaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,8 +441,9 @@
HorizontalAlignment="Stretch"
VerticalAlignment="Top">
<ScrollViewer
TabNavigation="Once"
VerticalScrollBarVisibility="Auto">
<local:ItemsRepeater
<local:ItemsRepeater
x:Name="MenuItemsHost"
AutomationProperties.Name="{TemplateBinding AutomationProperties.Name}"
AutomationProperties.AccessibilityView = "Content">
Expand Down Expand Up @@ -531,6 +532,7 @@
<Setter Property="FontSize" Value="{ThemeResource ControlContentThemeFontSize}" />
<Setter Property="UseSystemFocusVisuals" Value="True" />
<Setter Property="HorizontalContentAlignment" Value="Stretch" />
<Setter Property="TabNavigation" Value="Once"/>
<contract7Present:Setter Property="CornerRadius" Value="{ThemeResource ControlCornerRadius}" />
<Setter Property="Template">
<Setter.Value>
Expand Down Expand Up @@ -595,7 +597,8 @@
ContentTemplateSelector="{TemplateBinding ContentTemplateSelector}"
Content="{TemplateBinding Content}"
primitiveContract7Present:CornerRadius="{TemplateBinding CornerRadius}"
IsTabStop="False">
IsTabStop="false"
Control.IsTemplateFocusTarget="True">
</primitives:NavigationViewItemPresenter>
<local:ItemsRepeater
Grid.Row="1"
Expand Down
58 changes: 33 additions & 25 deletions dev/NavigationView/NavigationViewItem.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ static constexpr auto c_disabled = L"Disabled"sv;
static constexpr auto c_enabled = L"Enabled"sv;
static constexpr auto c_normal = L"Normal"sv;
static constexpr auto c_chevronHidden = L"ChevronHidden"sv;
static constexpr auto c_chevronVisible = L"ChevronVisible"sv;

static constexpr auto c_chevronVisibleOpen = L"ChevronVisibleOpen"sv;
static constexpr auto c_chevronVisibleClosed = L"ChevronVisibleClosed"sv;

void NavigationViewItem::UpdateVisualStateNoTransition()
{
Expand Down Expand Up @@ -123,6 +123,11 @@ void NavigationViewItem::OnApplyTemplate()
UpdateItemIndentation();
UpdateVisualStateNoTransition();
ReparentRepeater();
// We dont want to update the repeater visibilty during OnApplyTemplate if NavigationView is in a mode when items are shown in a flyout
if (!ShouldRepeaterShowInFlyout())
{
ShowHideChildren();
}

auto visual = winrt::ElementCompositionPreview::GetElementVisual(*this);
NavigationView::CreateAndAttachHeaderAnimation(visual);
Expand Down Expand Up @@ -468,7 +473,7 @@ void NavigationViewItem::UpdateVisualStateForChevron()
{
if (auto const presenter = m_navigationViewItemPresenter.get())
{
auto const chevronState = HasChildren() && !(m_isClosedCompact && ShouldRepeaterShowInFlyout()) ? c_chevronVisible : c_chevronHidden;
auto const chevronState = HasChildren() && !(m_isClosedCompact && ShouldRepeaterShowInFlyout()) ? ( IsExpanded() ? c_chevronVisibleOpen : c_chevronVisibleClosed) : c_chevronHidden;
winrt::VisualStateManager::GoToState(m_navigationViewItemPresenter.get(), chevronState, true);
}
}
Expand Down Expand Up @@ -516,33 +521,36 @@ NavigationViewItemPresenter * NavigationViewItem::GetPresenter()

void NavigationViewItem::ShowHideChildren()
{
bool shouldShowChildren = IsExpanded();
auto visibility = shouldShowChildren ? winrt::Visibility::Visible : winrt::Visibility::Collapsed;
m_repeater.get().Visibility(visibility);

if (ShouldRepeaterShowInFlyout())
if (auto const repeater = m_repeater.get())
{
if (shouldShowChildren)
bool shouldShowChildren = IsExpanded();
auto visibility = shouldShowChildren ? winrt::Visibility::Visible : winrt::Visibility::Collapsed;
repeater.Visibility(visibility);

if (ShouldRepeaterShowInFlyout())
{
// Verify that repeater is parented correctly
if (!m_isRepeaterParentedToFlyout)
if (shouldShowChildren)
{
ReparentRepeater();
}
// Verify that repeater is parented correctly
if (!m_isRepeaterParentedToFlyout)
{
ReparentRepeater();
}

// 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
{
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();
}
}
}
}
Expand Down
Loading

0 comments on commit de7ddff

Please sign in to comment.