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

marcelwgn · 2019-10-01T10:36:52Z

Description

~~Changed the parameter passed to SetSelectedItemAndExpectItemInvokeWhenSelectionChangedIfNotInvokedFromAPI from nullptr to the settings item instead.~~

Changed code where we update the settings item to set the m_shouldIgnoreNextSelectionChange flag to true, so SelectionChanged ignores that selection change.

Motivation and Context

Fixes #148

How Has This Been Tested?

Added new interaction test, which tests whether the item passed from SelectionChanged event is null or not.

Screenshots (if appropriate):

dev/NavigationView/NavigationView.cpp

jevansaks · 2019-10-01T20:19:03Z

/azp run

azure-pipelines · 2019-10-01T20:19:13Z

Azure Pipelines successfully started running 1 pipeline(s).

dev/NavigationView/NavigationView.cpp

licanhua · 2019-10-09T18:46:44Z

/azp run

azure-pipelines · 2019-10-09T18:46:55Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters · 2019-10-10T17:11:15Z

The failing tests looks like a legitimate failure. @chingucoding and @licanhua FYI.

licanhua · 2019-10-10T17:16:44Z

From last build, Look like InteractionTests.NavigationViewTests.EnsureLeftSettingsRetainsFocusAfterOrientationChanges is impacted by this PR.

marcelwgn · 2019-10-10T21:11:50Z

From last build, Look like InteractionTests.NavigationViewTests.EnsureLeftSettingsRetainsFocusAfterOrientationChanges is impacted by this PR.

I tried reproducing the problem locally. but the test is always succeeding. I have also tried setting the MaxTestedVersion to different Windows versions but the test is still not failing. Am I missing something?

StephenLPeters · 2019-10-14T18:19:05Z

From last build, Look like InteractionTests.NavigationViewTests.EnsureLeftSettingsRetainsFocusAfterOrientationChanges is impacted by this PR.

I tried reproducing the problem locally. but the test is always succeeding. I have also tried setting the MaxTestedVersion to different Windows versions but the test is still not failing. Am I missing something?

What is even stranger, the test expects the SettingsSelectionState text box's content to be 'True' but is finding 'False' however when you look at the screen shot at the time of the test it is displaying true.... I've run the test locally and stepped through the test manually and I can't find anything wrong, @licanhua @ojhad do either of you have an idea whats going on?

licanhua · 2019-10-14T20:04:01Z

/azp run

azure-pipelines · 2019-10-14T20:04:12Z

Azure Pipelines successfully started running 1 pipeline(s).

licanhua · 2019-10-15T20:58:52Z

@chingucoding , I don't know if it's merging issue. Could you please sync with master first, then create a new PR by merging the code with master to see what will happen?

From the screenshot, it should pass sometime but it's not. also I didn't see Line: 3861 match with your code.

Verify the left settings item is selected.
Error: Verify: AreEqual(False, True) [File: d:\a\1\s\dev\NavigationView\NavigationView_InteractionTests\NavigationViewTests.cs, Function: EnsureLeftSettingsRetainsFocusAfterOrientationChanges, Line: 3861]

…t with null as selected item

…aymode

…hangedIfNotInvokedFromAPI

marcelwgn · 2019-10-15T21:15:24Z

I think I correctly synced my branch with microsoft:master now. Should we try again now? 😅

licanhua · 2019-10-15T21:33:11Z

just wait for the test report.
I already queued it on another branch https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=42611

licanhua · 2019-10-16T19:07:02Z

@chingucoding . I can repro the problem by manual step.

In Settings|Display, disable animation.
Launch the test app, then resize it to make overflow for topnav and see scrollbar for left nav

Kill the app, relaunch it. then click NavigationView, and select NavigationViewTest, then click "Flip Orientation', then click 'Setting' on topnav, then click 'Flip' again, then click 'ReadSettingsSelected', you can see it's false.

…work correctly

marcelwgn · 2019-10-16T20:58:44Z

@chingucoding . I can repro the problem by manual step.

In Settings|Display, disable animation.

Launch the test app, then resize it to make overflow for topnav and see scrollbar for left nav

Kill the app, relaunch it. then click NavigationView, and select NavigationViewTest, then click "Flip Orientation', then click 'Setting' on topnav, then click 'Flip' again, then click 'ReadSettingsSelected', you can see it's false.

I was able to reproduce the issue now with the steps you provided. I think this is something about not correctly animating selection changed. While testing this I also noticed with animations disabled, changing the selected item in DisplayMode = Top does not correctly change the "underline" of the items.

I fixed that with the latest commit.

And thank you so much @licanhua for finding a way to reproduce this locally! 😅

licanhua · 2019-10-16T21:45:41Z

dev/NavigationView/NavigationView.cpp

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


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

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 ?

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.

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

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

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.

StephenLPeters · 2019-10-21T16:28:39Z

/azp run

azure-pipelines · 2019-10-21T16:28:50Z

Azure Pipelines successfully started running 1 pipeline(s).

ojhad · 2019-10-30T03:29:16Z

Hey @chingucoding
Thanks for your work on this fix! I am currently working on a refactor of NavigationView where the internal ListViews will be replaced with ItemsRepeater. There is a chance that this bug might become a non-issue as a result of the refactor. I think it makes most sense to hold off merging this fix until the refactor is in. We can revisit this if it is still an issue. I will close this PR for now while we wait and ping you once the refactor is done.

marcelwgn · 2019-10-30T09:38:49Z

Hey @chingucoding
Thanks for your work on this fix! I am currently working on a refactor of NavigationView where the internal ListViews will be replaced with ItemsRepeater. There is a chance that this bug might become a non-issue as a result of the refactor. I think it makes most sense to hold off merging this fix until the refactor is in. We can revisit this if it is still an issue. I will close this PR for now while we wait and ping you once the refactor is done.

Thanks for the update on that @ojhad. Looking forward to that! I assume it is better to not work on the NavigationView until the refactor is finished?

ojhad · 2019-10-31T17:29:52Z

Hey @chingucoding
Thanks for your work on this fix! I am currently working on a refactor of NavigationView where the internal ListViews will be replaced with ItemsRepeater. There is a chance that this bug might become a non-issue as a result of the refactor. I think it makes most sense to hold off merging this fix until the refactor is in. We can revisit this if it is still an issue. I will close this PR for now while we wait and ping you once the refactor is done.

Thanks for the update on that @ojhad. Looking forward to that! I assume it is better to not work on the NavigationView until the refactor is finished?

I would say contributing bug fixes to the control should still be fine! I decided to defer this fix primarily because of the concerns raised in the discussion above and the difficulty of resolving them.

ojhad · 2020-02-06T02:32:48Z

@chingucoding This can be revisited now that the refactor is in. It is still an issue, however now there is less complication regarding the selection changes. Let me know if there is anything I can help with!

marcelwgn · 2020-02-07T22:53:36Z

@chingucoding This can be revisited now that the refactor is in. It is still an issue, however now there is less complication regarding the selection changes. Let me know if there is anything I can help with!

@ojhad Thanks for the notice!
I will look into this once as soon as I have time.

… change (#1956) * Transfer changes from #1395 * Add appropriate check for shoudIgnoreNextSelectionChange * CR feedback * Add check for settings item * Switch to custom settings only flag for selection change suppresion * Fix CI failure

marcelwgn requested a review from a team as a code owner October 1, 2019 10:36

licanhua reviewed Oct 1, 2019

View reviewed changes

dev/NavigationView/NavigationView.cpp Outdated Show resolved Hide resolved

jevansaks assigned licanhua Oct 1, 2019

licanhua reviewed Oct 2, 2019

View reviewed changes

dev/NavigationView/NavigationView.cpp Outdated Show resolved Hide resolved

licanhua reviewed Oct 2, 2019

View reviewed changes

dev/NavigationView/NavigationView.cpp Show resolved Hide resolved

licanhua approved these changes Oct 2, 2019

View reviewed changes

marcelwgn added 5 commits October 15, 2019 23:05

Fix bug where changing display mode would raise SelectionChanged even…

8eacce0

…t with null as selected item

Add unit test for microsoft#148

e5cae29

Fix NavigationView to not raise selection changed when changing displ…

f887413

…aymode

Remove null check in SetSelectedItemAndExpectItemInvokeWhenSelectionC…

469d214

…hangedIfNotInvokedFromAPI

Apply suggestions from code review

d9d3bcc

marcelwgn force-pushed the navview-displaymodechange-event-bug branch from 1a28c90 to d9d3bcc Compare October 15, 2019 21:07

Fix bug where disabling animation and flipping orientation would not …

a208cc0

…work correctly

licanhua reviewed Oct 16, 2019

View reviewed changes

ojhad closed this Oct 30, 2019

ojhad mentioned this pull request Nov 12, 2019

Update NavigationView to use ItemsRepeater (Dev Tracking) #1477

Closed

23 tasks

marcelwgn added a commit to marcelwgn/microsoft-ui-xaml that referenced this pull request Feb 8, 2020

Transfer changes from microsoft#1395

4062667

StephenLPeters mentioned this pull request Feb 11, 2020

Fix NavView issue with SelectionChanged being raised upon displaymode change #1956

Merged

marcelwgn deleted the navview-displaymodechange-event-bug branch April 16, 2021 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcelwgn commented Oct 1, 2019 •

edited

Loading

jevansaks commented Oct 1, 2019

azure-pipelines bot commented Oct 1, 2019

licanhua commented Oct 9, 2019

azure-pipelines bot commented Oct 9, 2019

StephenLPeters commented Oct 10, 2019 •

edited

Loading

licanhua commented Oct 10, 2019

marcelwgn commented Oct 10, 2019

StephenLPeters commented Oct 14, 2019

licanhua commented Oct 14, 2019

azure-pipelines bot commented Oct 14, 2019

licanhua commented Oct 15, 2019

marcelwgn commented Oct 15, 2019

licanhua commented Oct 15, 2019

licanhua commented Oct 16, 2019

marcelwgn commented Oct 16, 2019

licanhua Oct 16, 2019

marcelwgn Oct 16, 2019

licanhua Oct 16, 2019

YuliKl Oct 17, 2019

StephenLPeters Oct 18, 2019

marcelwgn Oct 19, 2019

StephenLPeters commented Oct 21, 2019

azure-pipelines bot commented Oct 21, 2019

ojhad commented Oct 30, 2019

marcelwgn commented Oct 30, 2019

ojhad commented Oct 31, 2019

ojhad commented Feb 6, 2020

marcelwgn commented Feb 7, 2020

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

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

Conversation

marcelwgn commented Oct 1, 2019 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

jevansaks commented Oct 1, 2019

azure-pipelines bot commented Oct 1, 2019

licanhua commented Oct 9, 2019

azure-pipelines bot commented Oct 9, 2019

StephenLPeters commented Oct 10, 2019 • edited Loading

licanhua commented Oct 10, 2019

marcelwgn commented Oct 10, 2019

StephenLPeters commented Oct 14, 2019

licanhua commented Oct 14, 2019

azure-pipelines bot commented Oct 14, 2019

licanhua commented Oct 15, 2019

marcelwgn commented Oct 15, 2019

licanhua commented Oct 15, 2019

licanhua commented Oct 16, 2019

marcelwgn commented Oct 16, 2019

licanhua Oct 16, 2019

Choose a reason for hiding this comment

marcelwgn Oct 16, 2019

Choose a reason for hiding this comment

licanhua Oct 16, 2019

Choose a reason for hiding this comment

YuliKl Oct 17, 2019

Choose a reason for hiding this comment

StephenLPeters Oct 18, 2019

Choose a reason for hiding this comment

marcelwgn Oct 19, 2019

Choose a reason for hiding this comment

StephenLPeters commented Oct 21, 2019

azure-pipelines bot commented Oct 21, 2019

ojhad commented Oct 30, 2019

marcelwgn commented Oct 30, 2019

ojhad commented Oct 31, 2019

ojhad commented Feb 6, 2020

marcelwgn commented Feb 7, 2020

marcelwgn commented Oct 1, 2019 •

edited

Loading

StephenLPeters commented Oct 10, 2019 •

edited

Loading