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

Fix issue with settings button in top mode not receiving pointer events #2442

Merged

Conversation

marcelwgn
Copy link
Collaborator

Description

There is a regression that was introduced with #2004 where the settings button didn't receive any visual states for pointer events. Now we subscribe to the item's pointer events if it does not have a NavigationViewItemPresenter as children, which would handle this normally.

Motivation and Context

Fixes #2431

How Has This Been Tested?

Tested manually.

Screenshots (if appropriate):

gif

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label May 13, 2020
@Felix-Dev
Copy link
Contributor

Since this was a regression should we add a test for this?

@marcelwgn
Copy link
Collaborator Author

marcelwgn commented May 13, 2020

The problem is that we want to check pointer behaviors such as hover and how they affect the way they change visual states. And that is something I am not sure if that is possible within a reasonable amount of work. Something that might be problematic might also be the reliability of such tests, which sometimes was not great (e.g. MenuBar test where we repeat the test 10 for every test run).

@StephenLPeters @ranjeshj Your thoughts on this?

m_presenterPointerExitedRevoker = this->PointerExited(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerExited });
m_presenterPointerCanceledRevoker = this->PointerCanceled(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerCanceled });
m_presenterPointerCaptureLostRevoker = this->PointerCaptureLost(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerCaptureLost });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I might suggest:

winrt::UIElement const presenter = []()
{
    if (auto presenter = GetTemplateChildT<winrt::NavigationViewItemPresenter>(c_navigationViewItemPresenterName, controlProtected))
    {
        m_navigationViewItemPresenter.set(presenter);
        return presenter;
    }
    return this;
}

        m_presenterPointerPressedRevoker = presenter.PointerPressed(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerPressed });
        m_presenterPointerReleasedRevoker = presenter.PointerReleased(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerReleased });
        m_presenterPointerEnteredRevoker = presenter.PointerEntered(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerEntered });
        m_presenterPointerExitedRevoker = presenter.PointerExited(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerExited });
        m_presenterPointerCanceledRevoker = presenter.PointerCanceled(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerCanceled });
        m_presenterPointerCaptureLostRevoker = presenter.PointerCaptureLost(winrt::auto_revoke, { this, &NavigationViewItem::OnPresenterPointerCaptureLost });

you might need to cast somethign in the lambda to get it to compile

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea, updated the code.

@StephenLPeters
Copy link
Contributor

I don't think it would be very hard to write a test for this, we have input helpers for moving the mouse. But my impression is that this test would not catch a new regression very often so it might not be worth the cost.


In reply to: 628084096 [](ancestors = 628084096)

@marcelwgn
Copy link
Collaborator Author

I think the problem would be the following: If we hover over the settings button we need to wait until the visual states have updated, and then get the visual states of the button. However from inside an interaction test, we are not able to dig into the actual FrameworkElement if I recall correctly. Whenever we wanted to test visual states, we either do it in an API test (which won't work here) or we do it in the test UI and write the result in the test page, which then gets read by the interaction test.

Since we need to invoke an action (which could move the pointer), getting the result could potentially be unreliable. But if you want, I can still add a test.

@ranjeshj ranjeshj added area-NavigationView NavView control needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels May 13, 2020
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:

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters merged commit 196bf8b into microsoft:master May 20, 2020
ranjeshj pushed a commit that referenced this pull request Jul 7, 2020
…ts (#2442)

* Fix issue with settings button in top mode not receiving pointer events

* Remove empty line

* CR feedback
@ranjeshj ranjeshj removed the needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) label Aug 12, 2020
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.

NavigationView: Settings item has no pointerover change in top mode
5 participants