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

Remove code that removed focus visual in SelectionFollowsFocus mode #3190

Conversation

marcelwgn
Copy link
Collaborator

Description

When SelectionFollowsFocus was set to enabled we disabled the focus visual, which isn't desired anymore.

Motivation and Context

Fixes #1539

How Has This Been Tested?

Tested manually:
gif

Screenshots (if appropriate):

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 24, 2020
@YuliKl YuliKl added area-NavigationView NavView control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Aug 24, 2020
@llongley llongley requested a review from ojhad August 25, 2020 21:19
@@ -165,7 +165,6 @@ class NavigationView :
void UpdateVisualState(bool useTransitions = false);
void UpdateVisualStateForOverflowButton();
void UpdateLeftNavigationOnlyVisualState(bool useTransitions);
void UpdateNavigationViewUseSystemVisual();
static void PropagateShowFocusVisualToAllNavigationViewItemsInRepeater(winrt::ItemsRepeater const& ir, bool showFocusVisual);
Copy link
Contributor

Choose a reason for hiding this comment

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

PropagateShowFocusVisualToAllNavigationViewItemsInRepeater [](start = 16, length = 58)

I think this isn't called anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this isn't used anymore, missed that function. Is removed now.

@@ -508,10 +508,6 @@
</Setter>
</Style>

<!--Note: Even though UseSystemFocusVisuals is set to True here in the Style, and because
bindings in Styles are only evaluated once, we were unable to bring a TemplateSettings.UseSystemFocusVisuals
Copy link
Contributor

Choose a reason for hiding this comment

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

bindings in Styles are only evaluated once [](start = 4, length = 42)

@chingucoding and @Felix-Dev does this tid bit explain some of the Binding issues you've been seeing in other PRs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That certainly explains the issues I have seen. So the workaround is to essentially handle the binding inside the control logic then?

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

@chingucoding The pipeline fix just went in, could you please merge master into this PR?

@StephenLPeters
Copy link
Contributor

@chingucoding I think this one was missed?

@marcelwgn
Copy link
Collaborator Author

5f20943 was the merge with master which funnily came in before your message to merge with master.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

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

:shipit:

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.

Setting SelectionFollowsFocus causes focus rect to not render
4 participants