From e784a06f5cae3385a9c23f5d5a45bc80b3249645 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Thu, 20 Aug 2020 12:47:07 +0200 Subject: [PATCH 1/3] Let teachingtip treat placement logically not physically --- dev/TeachingTip/TeachingTip.cpp | 51 +++++++++++++++++++++++++++++---- dev/TeachingTip/TeachingTip.h | 2 ++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/dev/TeachingTip/TeachingTip.cpp b/dev/TeachingTip/TeachingTip.cpp index a00ea3e8aa..296bc3df1a 100644 --- a/dev/TeachingTip/TeachingTip.cpp +++ b/dev/TeachingTip/TeachingTip.cpp @@ -1,4 +1,4 @@ -#include "pch.h" +#include "pch.h" #include "common.h" #include "TeachingTip.h" #include "RuntimeProfiler.h" @@ -313,7 +313,7 @@ bool TeachingTip::UpdateTail() UpdateSizeBasedTemplateSettings(); - switch (m_currentEffectiveTailPlacementMode) + switch (GetFlowDirectionAdjustedPlacement(m_currentEffectiveTailPlacementMode)) { // An effective placement of auto means the tip should not display a tail. case winrt::TeachingTipPlacementMode::Auto: @@ -459,7 +459,7 @@ bool TeachingTip::PositionTargetedPopup() { // Depending on the effective placement mode of the tip we use a combination of the tip's size, the target's position within the app, the target's // size, and the target offset property to determine the appropriate vertical and horizontal offsets of the popup that the tip is contained in. - switch (m_currentEffectiveTipPlacementMode) + switch (GetFlowDirectionAdjustedPlacement(m_currentEffectiveTipPlacementMode)) { case winrt::TeachingTipPlacementMode::Top: popup.VerticalOffset(m_currentTargetBoundsInCoreWindowSpace.Y - tipHeight - offset.Top); @@ -556,7 +556,7 @@ bool TeachingTip::PositionUntargetedPopup() // offset property to determine the appropriate vertical and horizontal offsets of the popup that the tip is contained in. if (auto&& popup = m_popup.get()) { - switch (PreferredPlacement()) + switch (GetFlowDirectionAdjustedPlacement(PreferredPlacement())) { case winrt::TeachingTipPlacementMode::Auto: case winrt::TeachingTipPlacementMode::Bottom: @@ -646,7 +646,7 @@ void TeachingTip::UpdateSizeBasedTemplateSettings() return std::make_tuple(0.0, 0.0); }(); - switch (m_currentEffectiveTailPlacementMode) + switch (GetFlowDirectionAdjustedPlacement(m_currentEffectiveTailPlacementMode)) { case winrt::TeachingTipPlacementMode::Top: templateSettings->TopRightHighlightMargin(OtherPlacementTopRightHighlightMargin(width, height)); @@ -1343,6 +1343,47 @@ void TeachingTip::ClosePopup() } } +winrt::TeachingTipPlacementMode TeachingTip::GetFlowDirectionAdjustedPlacement(const winrt::TeachingTipPlacementMode& placementMode) +{ + if (FlowDirection() == winrt::FlowDirection::LeftToRight) + { + return placementMode; + } + else + { + switch (placementMode) + { + case winrt::TeachingTipPlacementMode::Auto: + return winrt::TeachingTipPlacementMode::Auto; + case winrt::TeachingTipPlacementMode::Left: + return winrt::TeachingTipPlacementMode::Right; + case winrt::TeachingTipPlacementMode::Right: + return winrt::TeachingTipPlacementMode::Left; + case winrt::TeachingTipPlacementMode::Top: + return winrt::TeachingTipPlacementMode::Top; + case winrt::TeachingTipPlacementMode::Bottom: + return winrt::TeachingTipPlacementMode::Bottom; + case winrt::TeachingTipPlacementMode::LeftBottom: + return winrt::TeachingTipPlacementMode::RightBottom; + case winrt::TeachingTipPlacementMode::LeftTop: + return winrt::TeachingTipPlacementMode::RightTop; + case winrt::TeachingTipPlacementMode::TopLeft: + return winrt::TeachingTipPlacementMode::TopRight; + case winrt::TeachingTipPlacementMode::TopRight: + return winrt::TeachingTipPlacementMode::TopLeft; + case winrt::TeachingTipPlacementMode::RightTop: + return winrt::TeachingTipPlacementMode::LeftTop; + case winrt::TeachingTipPlacementMode::RightBottom: + return winrt::TeachingTipPlacementMode::LeftBottom; + case winrt::TeachingTipPlacementMode::BottomRight: + return winrt::TeachingTipPlacementMode::BottomLeft; + case winrt::TeachingTipPlacementMode::BottomLeft: + return winrt::TeachingTipPlacementMode::BottomRight; + } + } + return winrt::TeachingTipPlacementMode::Auto; +} + void TeachingTip::OnTargetChanged() { m_targetLayoutUpdatedRevoker.revoke(); diff --git a/dev/TeachingTip/TeachingTip.h b/dev/TeachingTip/TeachingTip.h index c61ece5cfa..3e5245ad49 100644 --- a/dev/TeachingTip/TeachingTip.h +++ b/dev/TeachingTip/TeachingTip.h @@ -120,6 +120,8 @@ class TeachingTip : void ClosePopupWithAnimationIfAvailable(); void ClosePopup(); + winrt::TeachingTipPlacementMode GetFlowDirectionAdjustedPlacement(const winrt::TeachingTipPlacementMode& placementMode); + void SetViewportChangedEvent(const gsl::strict_not_null& target); void RevokeViewportChangedEvent(); void WindowSizeChanged(const winrt::CoreWindow&, const winrt::WindowSizeChangedEventArgs&); From 4d692de94dfd957ebad7a8bb635efa48daadf170 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Sat, 22 Aug 2020 03:20:01 +0200 Subject: [PATCH 2/3] Fix teachingtip behavior and add test --- .../InteractionTests/TeachingTipTests.cs | 179 ++++++++++++++++++ dev/TeachingTip/TeachingTip.cpp | 13 +- dev/TeachingTip/TestUI/TeachingTipPage.xaml | 2 +- 3 files changed, 188 insertions(+), 6 deletions(-) diff --git a/dev/TeachingTip/InteractionTests/TeachingTipTests.cs b/dev/TeachingTip/InteractionTests/TeachingTipTests.cs index d5ae124744..41b357a95f 100644 --- a/dev/TeachingTip/InteractionTests/TeachingTipTests.cs +++ b/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. + UseTestBounds(targetRect.W - 500, targetRect.X - 500, targetRect.Y + 1000, targetRect.Z + 1000, targetRect, true); UseTestBounds(targetRect.W - 500, targetRect.X - 500, targetRect.Y + 1000, targetRect.Z + 1000, targetRect, true); SetPreferredPlacement(PlacementOptions.Top); @@ -502,6 +504,183 @@ public void SpecifiedPlacement() } } + [TestMethod] + public void SpecifiedPlacementRTL() + { + using (var setup = new TestSetupHelper("TeachingTip Tests")) + { + elements = new TeachingTipTestPageElements(); + + foreach (TipLocationOptions location in Enum.GetValues(typeof(TipLocationOptions))) + { + SetTeachingTipLocation(location); + + ScrollTargetIntoView(); + ScrollBy(10); + + new CheckBox(FindElement.ByName("PageRTLCheckbox")).Check(); + + SetHeroContent(HeroContentOptions.NoContent); + + var targetRect = GetTargetBounds(); + + // All positions are valid + // The following might not always work, so repeat. + UseTestBounds(targetRect.W - 500, targetRect.X - 500, targetRect.Y + 1000, targetRect.Z + 1000, targetRect, true); + UseTestBounds(targetRect.W - 500, targetRect.X - 500, targetRect.Y + 1000, targetRect.Z + 1000, targetRect, true); + + SetPreferredPlacement(PlacementOptions.Top); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.Bottom); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.Left); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.Right); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.TopRight); + VerifyPlacement("TopLeft"); + SetPreferredPlacement(PlacementOptions.TopLeft); + VerifyPlacement("TopRight"); + SetPreferredPlacement(PlacementOptions.BottomRight); + VerifyPlacement("BottomLeft"); + SetPreferredPlacement(PlacementOptions.BottomLeft); + VerifyPlacement("BottomRight"); + SetPreferredPlacement(PlacementOptions.LeftTop); + VerifyPlacement("RightTop"); + SetPreferredPlacement(PlacementOptions.LeftBottom); + VerifyPlacement("RightBottom"); + SetPreferredPlacement(PlacementOptions.RightTop); + VerifyPlacement("LeftTop"); + SetPreferredPlacement(PlacementOptions.RightBottom); + VerifyPlacement("LeftBottom"); + SetPreferredPlacement(PlacementOptions.Center); + VerifyPlacement("Center"); + + // Eliminate left of the target + UseTestBounds(targetRect.W - 120, targetRect.X - 500, targetRect.Y + 1000, targetRect.Z + 1000, targetRect, true); + + SetPreferredPlacement(PlacementOptions.Top); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.Bottom); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.Left); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.Right); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.TopRight); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.TopLeft); + VerifyPlacement("TopRight"); + SetPreferredPlacement(PlacementOptions.BottomRight); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.BottomLeft); + VerifyPlacement("BottomRight"); + SetPreferredPlacement(PlacementOptions.LeftTop); + VerifyPlacement("RightTop"); + SetPreferredPlacement(PlacementOptions.LeftBottom); + VerifyPlacement("RightBottom"); + SetPreferredPlacement(PlacementOptions.RightTop); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.RightBottom); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.Center); + VerifyPlacement("Center"); + + // Eliminate top of the target + UseTestBounds(targetRect.W - 500, targetRect.X - 1, targetRect.Y + 1000, targetRect.Z + 1000, targetRect, true); + + SetPreferredPlacement(PlacementOptions.Top); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.Bottom); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.Left); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.Right); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.TopRight); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.TopLeft); + VerifyPlacement("Bottom"); + SetPreferredPlacement(PlacementOptions.BottomRight); + VerifyPlacement("BottomLeft"); + SetPreferredPlacement(PlacementOptions.BottomLeft); + VerifyPlacement("BottomRight"); + SetPreferredPlacement(PlacementOptions.LeftTop); + VerifyPlacement("RightTop"); + SetPreferredPlacement(PlacementOptions.LeftBottom); + VerifyPlacement("RightBottom"); + SetPreferredPlacement(PlacementOptions.RightTop); + VerifyPlacement("LeftTop"); + SetPreferredPlacement(PlacementOptions.RightBottom); + VerifyPlacement("LeftBottom"); + SetPreferredPlacement(PlacementOptions.Center); + VerifyPlacement("Center"); + + // Eliminate right of the target + UseTestBounds(targetRect.W - 500, targetRect.X - 500, targetRect.Y + 500, targetRect.Z + 1000, targetRect, true); + + SetPreferredPlacement(PlacementOptions.Top); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.Bottom); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.Left); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.Right); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.TopRight); + VerifyPlacement("TopLeft"); + SetPreferredPlacement(PlacementOptions.TopLeft); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.BottomRight); + VerifyPlacement("BottomLeft"); + SetPreferredPlacement(PlacementOptions.BottomLeft); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.LeftTop); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.LeftBottom); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.RightTop); + VerifyPlacement("LeftTop"); + SetPreferredPlacement(PlacementOptions.RightBottom); + VerifyPlacement("LeftBottom"); + SetPreferredPlacement(PlacementOptions.Center); + VerifyPlacement("Left"); + + // Eliminate bottom of target + UseTestBounds(targetRect.W - 500, targetRect.X - 500, targetRect.Y + 1000, targetRect.Z + 501, targetRect, true); + + SetPreferredPlacement(PlacementOptions.Top); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.Bottom); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.Left); + VerifyPlacement("Right"); + SetPreferredPlacement(PlacementOptions.Right); + VerifyPlacement("Left"); + SetPreferredPlacement(PlacementOptions.TopRight); + VerifyPlacement("TopLeft"); + SetPreferredPlacement(PlacementOptions.TopLeft); + VerifyPlacement("TopRight"); + SetPreferredPlacement(PlacementOptions.BottomRight); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.BottomLeft); + VerifyPlacement("Top"); + SetPreferredPlacement(PlacementOptions.LeftTop); + VerifyPlacement("RightTop"); + SetPreferredPlacement(PlacementOptions.LeftBottom); + VerifyPlacement("RightBottom"); + SetPreferredPlacement(PlacementOptions.RightTop); + VerifyPlacement("LeftTop"); + SetPreferredPlacement(PlacementOptions.RightBottom); + VerifyPlacement("LeftBottom"); + SetPreferredPlacement(PlacementOptions.Center); + VerifyPlacement("Center"); + + new CheckBox(FindElement.ByName("PageRTLCheckbox")).Uncheck(); + } + } + } + [TestMethod] public void NoIconDoesNotCrash() diff --git a/dev/TeachingTip/TeachingTip.cpp b/dev/TeachingTip/TeachingTip.cpp index 296bc3df1a..66d5e4bf88 100644 --- a/dev/TeachingTip/TeachingTip.cpp +++ b/dev/TeachingTip/TeachingTip.cpp @@ -313,7 +313,7 @@ bool TeachingTip::UpdateTail() UpdateSizeBasedTemplateSettings(); - switch (GetFlowDirectionAdjustedPlacement(m_currentEffectiveTailPlacementMode)) + switch (m_currentEffectiveTailPlacementMode) { // An effective placement of auto means the tip should not display a tail. case winrt::TeachingTipPlacementMode::Auto: @@ -459,7 +459,7 @@ bool TeachingTip::PositionTargetedPopup() { // Depending on the effective placement mode of the tip we use a combination of the tip's size, the target's position within the app, the target's // size, and the target offset property to determine the appropriate vertical and horizontal offsets of the popup that the tip is contained in. - switch (GetFlowDirectionAdjustedPlacement(m_currentEffectiveTipPlacementMode)) + switch (m_currentEffectiveTipPlacementMode) { case winrt::TeachingTipPlacementMode::Top: popup.VerticalOffset(m_currentTargetBoundsInCoreWindowSpace.Y - tipHeight - offset.Top); @@ -646,7 +646,7 @@ void TeachingTip::UpdateSizeBasedTemplateSettings() return std::make_tuple(0.0, 0.0); }(); - switch (GetFlowDirectionAdjustedPlacement(m_currentEffectiveTailPlacementMode)) + switch (m_currentEffectiveTailPlacementMode) { case winrt::TeachingTipPlacementMode::Top: templateSettings->TopRightHighlightMargin(OtherPlacementTopRightHighlightMargin(width, height)); @@ -1379,6 +1379,8 @@ winrt::TeachingTipPlacementMode TeachingTip::GetFlowDirectionAdjustedPlacement(c return winrt::TeachingTipPlacementMode::BottomLeft; case winrt::TeachingTipPlacementMode::BottomLeft: return winrt::TeachingTipPlacementMode::BottomRight; + case winrt::TeachingTipPlacementMode::Center: + return winrt::TeachingTipPlacementMode::Center; } } return winrt::TeachingTipPlacementMode::Auto; @@ -1709,7 +1711,7 @@ std::tuple TeachingTip::DetermineEffectiv // SetReturnTopForOutOfWindowBounds test hook. if (!ShouldConstrainToRootBounds() && m_returnTopForOutOfWindowPlacement) { - auto const placement = PreferredPlacement(); + auto const placement = GetFlowDirectionAdjustedPlacement(PreferredPlacement()); if (placement == winrt::TeachingTipPlacementMode::Auto) { return std::make_tuple(winrt::TeachingTipPlacementMode::Top, false); @@ -1934,7 +1936,8 @@ std::tuple TeachingTip::DetermineEffectiv availability[winrt::TeachingTipPlacementMode::RightBottom] = false; } - auto const priorities = GetPlacementFallbackOrder(PreferredPlacement()); + auto const wantedDirection = GetFlowDirectionAdjustedPlacement(PreferredPlacement()); + auto const priorities = GetPlacementFallbackOrder(wantedDirection); for (auto const mode : priorities) { diff --git a/dev/TeachingTip/TestUI/TeachingTipPage.xaml b/dev/TeachingTip/TestUI/TeachingTipPage.xaml index 39bb6423a5..279704b047 100644 --- a/dev/TeachingTip/TestUI/TeachingTipPage.xaml +++ b/dev/TeachingTip/TestUI/TeachingTipPage.xaml @@ -548,7 +548,7 @@ - + Visual tree teachingtip unloaded From 06931bb56ad1422cb6d8f89b9b1ab8664f2695c4 Mon Sep 17 00:00:00 2001 From: Marcel Wagner Date: Mon, 31 Aug 2020 23:22:01 +0200 Subject: [PATCH 3/3] CR feedback --- .../InteractionTests/TeachingTipTestPageElements.cs | 6 ++++++ dev/TeachingTip/InteractionTests/TeachingTipTests.cs | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs b/dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs index 03ce37569c..e225295e04 100644 --- a/dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs +++ b/dev/TeachingTip/InteractionTests/TeachingTipTestPageElements.cs @@ -412,6 +412,12 @@ public CheckBox GetTeachingTipContentUnloadedCheck() } private CheckBox effectiveTeachingTipContentUnloadedCheckbox; + public CheckBox GetPageRTLCheckbox() + { + return GetElement(ref effectivePageRTLCheckbox, "PageRTLCheckbox"); + } + private CheckBox effectivePageRTLCheckbox; + public Button GetRemoveOpenButtonFromVisualTreeButton() { return GetElement(ref effectiveRemoveOpenButton, "RemoveButtonFromVisualTreeButton"); diff --git a/dev/TeachingTip/InteractionTests/TeachingTipTests.cs b/dev/TeachingTip/InteractionTests/TeachingTipTests.cs index 41b357a95f..1f73126951 100644 --- a/dev/TeachingTip/InteractionTests/TeachingTipTests.cs +++ b/dev/TeachingTip/InteractionTests/TeachingTipTests.cs @@ -518,7 +518,7 @@ public void SpecifiedPlacementRTL() ScrollTargetIntoView(); ScrollBy(10); - new CheckBox(FindElement.ByName("PageRTLCheckbox")).Check(); + elements.GetPageRTLCheckbox().Check(); SetHeroContent(HeroContentOptions.NoContent); @@ -676,7 +676,7 @@ public void SpecifiedPlacementRTL() SetPreferredPlacement(PlacementOptions.Center); VerifyPlacement("Center"); - new CheckBox(FindElement.ByName("PageRTLCheckbox")).Uncheck(); + elements.GetPageRTLCheckbox().Uncheck(); } } }