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 problem where TeachingTip would not update position when target has moved #1609

Merged
merged 9 commits into from
Nov 27, 2019

Conversation

marcelwgn
Copy link
Contributor

@marcelwgn marcelwgn commented Nov 13, 2019

Description

Fix bug where moving of target of a TeachingTip would leave TeachingTip in place and possibly dangling separated from its target.

Motivation and Context

Fixes #1547

How Has This Been Tested?

Added new test.

Screenshots (if appropriate):

tt_after_targetupdatefix

@kaiguo
Copy link
Contributor

kaiguo commented Nov 14, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Co-Authored-By: Kai Guo <guokai.ok@gmail.com>
@marcelwgn
Copy link
Contributor Author

It seems like some tests are failing now:
https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=49051

However I was unable to reproduce those tests failing on my machine 🤔

@StephenLPeters
Copy link
Contributor

@SavoySchuler FYI. I actually had this feature in developement for a long time, there is even a test hook which enables it, search the code for m_tipFollowsTarget to see. We decided during development that we did not want this behavior because when the target scrolls outside of the effective view port our tip would potentially point to the wrong piece of UI, our guidance specifies not to target an element in a scrollable surface with a teaching tip for exactly this reason. I think to fix #1547 we need to attach to the window's maximized event.

@StephenLPeters
Copy link
Contributor

If we do decide to change the tip follows target behavior the control is set up such that changing the m_tipFollowsTarget boolean flag's default value from false to true will enable the feature.

@StephenLPeters
Copy link
Contributor

    public void TipCanFollowTarget()

I think this test already does what you are testing for


Refers to: dev/TeachingTip/InteractionTests/TeachingTipTests.cs:187 in 45b682e. [](commit_id = 45b682e, deletion_comment = False)

@StephenLPeters
Copy link
Contributor

It seems like some tests are failing now:
https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=49051

However I was unable to reproduce those tests failing on my machine 🤔

The bulk of your test failures are the test app crashing on rs2-4 because the effectiveViewportChanged event you are attaching to is only available on rs5+. The rs5 and 19h1 failure in TipsWhichDoNotFitDoNotOpen I can't explain

@jevansaks jevansaks added the release note PR that we want to call out in the next release summary label Nov 23, 2019
@marcelwgn
Copy link
Contributor Author

    public void TipCanFollowTarget()

I think this test already does what you are testing for

Refers to: dev/TeachingTip/InteractionTests/TeachingTipTests.cs:187 in 45b682e. [](commit_id = 45b682e, deletion_comment = False)

Thank you for pointing that out. Missed that there were already tests covering what I tried to test with that. 😅 I have removed the test now.

Height="40" Width="100">Click to open TeachingTip
<Button.Resources>
<muxc:TeachingTip x:Name="TeachingTipInResourcesOnEdge"
AutomationProperties.Name="TeachingTipInResources"
Copy link
Contributor

Choose a reason for hiding this comment

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

TeachingTipInResources [](start = 69, length = 22)

This has the same automation name as the other teaching tip on this page, I'd be surprised if this didn't cause test issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly this did not crash the tests. I guess since we don't have test that actually get the TeachingTips using their UIA name, no test is impacted by conflicting UIA names.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Co-Authored-By: Stephen L Peters <stpete@microsoft.com>
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters merged commit 9e5d035 into microsoft:master Nov 27, 2019
@msft-github-bot
Copy link
Collaborator

🎉Microsoft.UI.Xaml v2.3.191127001-prerelease has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release note PR that we want to call out in the next release summary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Targeted TeachingTips don't change position when the window is resized by changing maximized state.
5 participants