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.

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

NavigationViewItem input handling fixes #2625

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

RBrid
Copy link
Contributor

@RBrid RBrid commented Jun 8, 2020

These changes address a series of bugs in the NavigationViewItem implementation:
#1 NavigationViewItem does not update its visual state when its IsSelected property changed.

  • In WinUI3, often when you tap a NavigationViewItem, it stays dark gray.
  • When you select a NavigationViewItem programmatically, it does not change its visual state.
    Fix: Listening to IsSelected change notifications to refresh visual state.

#2 PonterMoved does not update NavigationViewItem visual state in WinUI3.

  • Move mouse cursor over NavigationViewItem A
  • Tap NavigationViewItem B
  • Move mouse cursor a bit --> NavigationViewItem A does not update its visual state.
    Fix: Listening to PointerMoved to update visual state when pointer is not thought to be over.

#3 Mouse pointer is not captured in PointerPressed.

  • Mouse button down on NavigationViewItem.
  • Move mouse outside of NavigationViewItem.
  • Release mouse button --> NavigationViewItem does not update its NavigationViewItem visual state. It stays dark gray until you interact with it again.
    Fix: Pointer is captured in PointerPressed and released in PointerReleased, or when pointer is lost.

#4 IsEnabled change does not update NavigationViewItem visual state.
Fix: Listening to NavigationViewItem.IsEnabledChanged to clear the state flags and refresh visual state.

#5 Beginning of OnApplyTemplate does not reset template parts, or clear event handlers.
Fix: All control parts and handler are cleared at beginning of OnApplyTemplate to avoid leftover incorrect fields.

#6 Pointer input handlers that reset internal flags are not invoked when args.Handled is true causing bugs.
Fix: Input handlers that reset flags are not skipped thanks to the handledEventsToo=True feature.

#7 Multi finger inputs are not handled properly. Removing second finger clears pressed state for instance.
Fix: Only one pointer Id is tracked for a given NavigationViewItem now. Any newcomer is ignored.

o Also did some dead code cleanup, and code cleanup in general.
o Did some ad-hoc testing, expanded controls test app and NavigationView test.

(These changes also address 26317023 once pushed into WinUI)

@RBrid RBrid added the area-NavigationView NavView control label Jun 8, 2020
@RBrid RBrid requested a review from a team June 8, 2020 17:30
@RBrid RBrid self-assigned this Jun 8, 2020
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Jun 8, 2020
@StephenLPeters StephenLPeters added team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jun 8, 2020
@StephenLPeters StephenLPeters requested a review from ojhad June 8, 2020 20:09
@RBrid
Copy link
Contributor Author

RBrid commented Jun 9, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

dev/NavigationView/NavigationViewItem.cpp Outdated Show resolved Hide resolved
dev/NavigationView/NavigationViewItem.cpp Outdated Show resolved Hide resolved
@ojhad
Copy link
Contributor

ojhad commented Jun 9, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@RBrid
Copy link
Contributor Author

RBrid commented Jun 10, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NavigationView NavView control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants