Let teachingtip treat placement logically not physically #3165

marcelwgn · 2020-08-20T10:50:24Z

Description

Every piece of code inside TeachingTip that handled the placement just took the values as is. I've added a function that returns the placementmode we want based on the flowdirection.

Motivation and Context

Closes #458

How Has This Been Tested?

Tested manually since the existing placement mode tests failed even if I just build master branch without any changes.

Screenshots (if appropriate):

StephenLPeters · 2020-08-21T22:59:07Z

dev/TeachingTip/TeachingTip.cpp

@@ -1343,6 +1343,47 @@ void TeachingTip::ClosePopup()
    }
 }

+winrt::TeachingTipPlacementMode TeachingTip::GetFlowDirectionAdjustedPlacement(const winrt::TeachingTipPlacementMode& placementMode)


GetFlowDirectionAdjustedPlacement [](start = 45, length = 33)

@jevansaks do you remember why we didn't do this for the initial check in? I know that we talked about it but I can't recall what we got hung up on.

Sorry, unfortunately I don't remember either.

StephenLPeters · 2020-08-21T22:59:49Z

How confident are you in your manual testing. I think I remember this being more complicated than we thought.

StephenLPeters · 2020-08-21T23:00:51Z

We should add a test which sets the flow direction to rtl

marcelwgn · 2020-08-22T01:22:50Z

I've added a test now which should test this quite thoroughly. My confidence in the manual tests I did jumped off a clip when I saw how many mistakes the TeachingTip "made" in the test. Should be good now though.

StephenLPeters · 2020-08-31T20:52:59Z

dev/TeachingTip/InteractionTests/TeachingTipTests.cs

@@ -350,6 +350,8 @@ public void SpecifiedPlacement()
                    var targetRect = GetTargetBounds();

                    // All positions are valid
+                    // The following might not always work, so repeat.


// The following might not always work, so repeat. [](start = 20, length = 50)

Really? what happens? was that the case before this change?

I am not entirely sure what the root cause is, however when running those tests locally, I noticed that the rect (and the red border we use to show it) don't always show up. The problem is that this leaves the TeachingTip to believe it has more space and fails the test.

Why wasn't this causing the test to fail before this change?

The test was very unreliable on my machine before that change, only 20% of the time it actually ran through.

StephenLPeters · 2020-08-31T20:54:29Z

dev/TeachingTip/InteractionTests/TeachingTipTests.cs

+                    ScrollTargetIntoView();
+                    ScrollBy(10);
+
+                    new CheckBox(FindElement.ByName("PageRTLCheckbox")).Check();


FindElement [](start = 33, length = 11)

Teaching Tip's test page uses the TeachingTipTestPageElements class to get things like this, I'd prefer that this get moved there.

StephenLPeters · 2020-08-31T20:55:21Z

dev/TeachingTip/InteractionTests/TeachingTipTests.cs

+                    SetPreferredPlacement(PlacementOptions.Center);
+                    VerifyPlacement("Center");
+
+                    new CheckBox(FindElement.ByName("PageRTLCheckbox")).Uncheck();


FindElement [](start = 33, length = 11)

same here

StephenLPeters · 2020-08-31T20:57:01Z

dev/TeachingTip/TeachingTip.cpp

+                return winrt::TeachingTipPlacementMode::Center;
+        }
+    }
+    return winrt::TeachingTipPlacementMode::Auto;


TeachingTipPlacementMode [](start = 18, length = 24)

Is this needed?

Either this or a default cause for our switch case, otherwise not all code paths would return a value according to the compiler.

StephenLPeters · 2020-08-31T21:01:10Z

Since flow direction is not app wide but rather a propogating property. What happens if the teaching tip is LeftToRight but its target is RightToLeft? Like what would happen if you defined your teaching tip in you App.Resources and its target was in an RTL Panel. Does it maybe make more sense for the positioning logic use the flowLayout of the target where the content of the teaching tip would use the tip's flow layout?

StephenLPeters · 2020-08-31T21:01:40Z

Since flow direction is not app wide but rather a propogating property. What happens if the teaching tip is LeftToRight but its target is RightToLeft? Like what would happen if you defined your teaching tip in you App.Resources and its target was in an RTL Panel. Does it maybe make more sense for the positioning logic use the flowLayout of the target where the content of the teaching tip would use the tip's flow layout?

@ranjeshj curious what you think.

marcelwgn · 2020-08-31T21:28:37Z

Since flow direction is not app wide but rather a propogating property. What happens if the teaching tip is LeftToRight but its target is RightToLeft? Like what would happen if you defined your teaching tip in you App.Resources and its target was in an RTL Panel. Does it maybe make more sense for the positioning logic use the flowLayout of the target where the content of the teaching tip would use the tip's flow layout?

That's a very good point. Maybe, reacting based on the target (if it exists) is actually the better idea. However I am not sure how common it is for a Page or part of an app to have a different flowdirection then the rest of the app. I think the main reason why we need to support RTL is for users who use RTL languages resulting in all apps being in RTL. In that case, it shouldn't really matter if we use the targets direction or the TeachingTips one.

ranjeshj · 2020-09-01T21:22:50Z

Since flow direction is not app wide but rather a propogating property. What happens if the teaching tip is LeftToRight but its target is RightToLeft? Like what would happen if you defined your teaching tip in you App.Resources and its target was in an RTL Panel. Does it maybe make more sense for the positioning logic use the flowLayout of the target where the content of the teaching tip would use the tip's flow layout?

That's a very good point. Maybe, reacting based on the target (if it exists) is actually the better idea. However I am not sure how common it is for a Page or part of an app to have a different flowdirection then the rest of the app. I think the main reason why we need to support RTL is for users who use RTL languages resulting in all apps being in RTL. In that case, it shouldn't really matter if we use the targets direction or the TeachingTips one.

Not entirely sure. Adding @MikeHillberg and @YuliKl. How do we deal with this in a popup ?

StephenLPeters · 2020-09-01T21:39:19Z

I agree that most of the time the flow direction of the target and the tip will be the same. The question is what is the right behavior when they aren't.

StephenLPeters · 2020-09-03T22:12:26Z

/azp run

azure-pipelines · 2020-09-03T22:12:42Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-10-28T19:48:57Z

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

Handy links:

ghost · 2020-11-05T15:10:45Z

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

Handy links:

Let teachingtip treat placement logically not physically

e784a06

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Aug 20, 2020

StephenLPeters reviewed Aug 21, 2020

View reviewed changes

ranjeshj added team-Controls Issue for the Controls team area-TeachingTip and removed needs-triage Issue needs to be triaged by the area owners labels Aug 21, 2020

Fix teachingtip behavior and add test

4d692de

StephenLPeters reviewed Aug 31, 2020

View reviewed changes

CR feedback

06931bb

Merge branch 'master' into user/chingucoding/teachingtip-RTL

d236c73

StephenLPeters approved these changes Sep 24, 2020

View reviewed changes

StephenLPeters merged commit a560a86 into microsoft:master Sep 24, 2020

ghost mentioned this pull request Oct 28, 2020

TeachingTip in RTL treats Placement=Left as physical instead of logical #458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let teachingtip treat placement logically not physically #3165

Let teachingtip treat placement logically not physically #3165

marcelwgn commented Aug 20, 2020

StephenLPeters Aug 21, 2020

jevansaks Aug 21, 2020

StephenLPeters commented Aug 21, 2020

StephenLPeters commented Aug 21, 2020

marcelwgn commented Aug 22, 2020

StephenLPeters Aug 31, 2020

marcelwgn Aug 31, 2020

StephenLPeters Sep 2, 2020

marcelwgn Sep 3, 2020

StephenLPeters Aug 31, 2020

marcelwgn Sep 3, 2020

StephenLPeters Aug 31, 2020

marcelwgn Sep 3, 2020

StephenLPeters Aug 31, 2020

marcelwgn Aug 31, 2020

StephenLPeters commented Aug 31, 2020

StephenLPeters commented Aug 31, 2020

marcelwgn commented Aug 31, 2020

ranjeshj commented Sep 1, 2020

StephenLPeters commented Sep 1, 2020

StephenLPeters commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

ghost commented Oct 28, 2020

ghost commented Nov 5, 2020

Let teachingtip treat placement logically not physically #3165

Let teachingtip treat placement logically not physically #3165

Conversation

marcelwgn commented Aug 20, 2020

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Aug 21, 2020

StephenLPeters commented Aug 21, 2020

marcelwgn commented Aug 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenLPeters commented Aug 31, 2020

StephenLPeters commented Aug 31, 2020

marcelwgn commented Aug 31, 2020

ranjeshj commented Sep 1, 2020

StephenLPeters commented Sep 1, 2020

StephenLPeters commented Sep 3, 2020

azure-pipelines bot commented Sep 3, 2020

ghost commented Oct 28, 2020

ghost commented Nov 5, 2020