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

Fix bug where changing the DisplayMode would raise SelectionChanged with SelectedItem as null #1395

Closed
wants to merge 6 commits into from
Closed

Fix bug where changing the DisplayMode would raise SelectionChanged with SelectedItem as null #1395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8eacce0
Fix bug where changing display mode would raise SelectionChanged even…
marcelwgn Oct 1, 2019
e5cae29
Add unit test for #148
marcelwgn Oct 1, 2019
f887413
Fix NavigationView to not raise selection changed when changing displ…
marcelwgn Oct 2, 2019
469d214
Remove null check in SetSelectedItemAndExpectItemInvokeWhenSelectionC…
marcelwgn Oct 2, 2019
d9d3bcc
Apply suggestions from code review
marcelwgn Oct 2, 2019
a208cc0
Fix bug where disabling animation and flipping orientation would not …
marcelwgn Oct 16, 2019
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
29 changes: 24 additions & 5 deletions dev/NavigationView/NavigationView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

#include "pch.h"
Expand Down Expand Up @@ -397,7 +397,12 @@ void NavigationView::CreateAndHookEventsToSettings(std::wstring_view settingsNam
bool shouldSelectSetting = selectedItem && IsSettingsItem(selectedItem);

if (shouldSelectSetting)
{
{
auto scopeGuard = gsl::finally([this]()
{
m_shouldIgnoreNextSelectionChange = false;
});
m_shouldIgnoreNextSelectionChange = true;
marcelwgn marked this conversation as resolved.
Show resolved Hide resolved
SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNotInvokedFromAPI(nullptr);
}

Expand Down Expand Up @@ -425,7 +430,12 @@ void NavigationView::CreateAndHookEventsToSettings(std::wstring_view settingsNam
SetValue(s_SettingsItemProperty, settingsItem);

if (shouldSelectSetting)
{
{
auto scopeGuard = gsl::finally([this]()
{
m_shouldIgnoreNextSelectionChange = false;
});
m_shouldIgnoreNextSelectionChange = true;
SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNotInvokedFromAPI(m_settingsItem.get());
}
}
Expand Down Expand Up @@ -1923,7 +1933,17 @@ void NavigationView::UpdateSingleSelectionFollowsFocusTemplateSetting()
void NavigationView::OnSelectedItemPropertyChanged(winrt::DependencyPropertyChangedEventArgs const& args)
{
auto newItem = args.NewValue();
ChangeSelection(args.OldValue(), newItem);
auto oldItem = args.OldValue();
ChangeSelection(oldItem, newItem);

// Animate to be sure the selected item is visually higlighted!
// See #1395 for additional context
if (oldItem != newItem)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do it here. Some feature like NavigationRecommendedTransitionDirection, SelectionSuppressed and topnav animation may be broken which is implemented in ChangeSelection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I see. So should we set m_shouldRaiseInvokeItemInSelectionChange to false when before we call ChangeSelection in OnSelectedItemPropertyChanged? Or would it be better to move lines 1943/1944 inside CreateAndHookEventsToSettings ?

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 have good suggestion on it. It's possible that add a flag like 'Hi, I'm switching from topnav to leftnav, please don't raise selection change event for settings'.
Most likely it's not a easy change because we don't have too much automation on this part and you have to manually verify all kind of scenarios.
I would like @YuliKl to re-evaluate the impact of the bug.

Copy link

Choose a reason for hiding this comment

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

I admit that I'm not quite following the code intricacies within NavigationView and dont' have a good grasp on what's complicating this fix. I do know there's a real problem outlined in #148 that was encountered by a customer (although at this point, I can't locate any record of who originally found this bug).

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be stalling out. @ojhad @licanhua @YuliKl @chingucoding any idea what the next steps are?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the least we should do, is to ensure that the selected item that gets passed to the event-handler/event-args is not null.

However not raising the event in that case seems to be quite difficult, as we apparently break animations with that. The question is how common it is for developers to change the orientation of the NavigationView during runtime. If this is an edge case that is not very common, we should maybe not try to prevent the raising of that event. After all, catching that "bug" is something that is definitely achievable.

{
ChangeSelectStatusForItem(oldItem, false /*selected*/);
ChangeSelectStatusForItem(newItem, true /*selected*/);
AnimateSelectionChanged(oldItem, newItem);
}

if (m_appliedTemplate && IsTopNavigationView())
{
Expand Down Expand Up @@ -1980,7 +2000,6 @@ void NavigationView::SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNot

m_indexOfLastSelectedItemInTopNav = m_topDataProvider.IndexOf(item); // for the next time we animate
}

SelectedItem(item);
if (!isChangingSelection)
{
Expand Down
35 changes: 35 additions & 0 deletions dev/NavigationView/NavigationView_InteractionTests/NavigationViewTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2923,6 +2923,41 @@ public void KeyboardFocusToolTipTest() // Verify tooltips appear when Keyboard f
}
}

[TestMethod]
[TestProperty("TestSuite", "C")]
public void DisplayModeChangeSelectionEventTest()
{
var testScenarios = RegressionTestScenario.BuildLeftNavRegressionTestScenarios();
foreach (var testScenario in testScenarios)
{
using (var setup = new TestSetupHelper(new[] { "NavigationView Tests", "NavigationView Test" }))
{
if (!PlatformConfiguration.IsOsVersionGreaterThanOrEqual(OSVersion.Redstone3))
{
Log.Warning("Test is disabled on RS2 and earlier because SplitView lacks the requisite events.");
return;
}
Button clearSelectedItem = new Button(FindElement.ById("ClearSelectionChangeIndicatorButton"));
TextBlock selectionRaisedIndicator = new TextBlock(FindElement.ById("SelectionChangedRaised"));

ComboBox selectedItem = new ComboBox(FindElement.ById("SelectedItemCombobox"));
selectedItem.SelectItemByName("Settings");
Verify.AreEqual("True", selectionRaisedIndicator.GetText());

ComboBox displayMode = new ComboBox(FindElement.ById("PaneDisplayModeCombobox"));
clearSelectedItem.InvokeAndWait();
displayMode.SelectItemByName("Top");
Verify.AreEqual("False", selectionRaisedIndicator.GetText());
Wait.ForIdle();


displayMode.SelectItemByName("Left");
Wait.ForIdle();
Verify.AreEqual("False", selectionRaisedIndicator.GetText());
}
}
}

[TestMethod]
[TestProperty("TestSuite", "C")]
public void PaneOpenCloseEventsTest()
Expand Down
3 changes: 3 additions & 0 deletions dev/NavigationView/TestUI/NavigationViewPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@
<ComboBoxItem Content="LeftCompact" Tag="LeftCompact"/>
<ComboBoxItem Content="LeftMinimal" Tag="LeftMinimal"/>
</ComboBox>
<Button x:Name="ClearSelectionChangeIndicatorButton" AutomationProperties.Name="ClearSelectionChangeIndicatorButton" Click="ClearSelectionChangeBlock">Clear selection changed indicator</Button>
<TextBlock VerticalAlignment="Center">Selection was raised: </TextBlock>
<TextBlock VerticalAlignment="Center" x:Name="SelectionChangedRaised" AutomationProperties.Name="SelectionChangedRaised"/>
</StackPanel>

<StackPanel Orientation="Horizontal">
Expand Down
6 changes: 6 additions & 0 deletions dev/NavigationView/TestUI/NavigationViewPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ private void HeightCombobox_SelectionChanged(object sender, SelectionChangedEven

private void NavView_SelectionChanged(NavigationView sender, NavigationViewSelectionChangedEventArgs args)
{
SelectionChangedRaised.Text = "True";
if (args.SelectedItem != null)
{
var itemdata = args.SelectedItem as NavigationViewItem;
Expand All @@ -311,6 +312,11 @@ private void NavView_SelectionChanged(NavigationView sender, NavigationViewSelec
}
}

private void ClearSelectionChangeBlock(object sender,RoutedEventArgs e)
{
SelectionChangedRaised.Text = "False";
}

private void MoviesEnabledCheckbox_Checked(object sender, RoutedEventArgs e)
{
MoviesItem.IsEnabled = true;
Expand Down