From 97dd7857fd6c686cc831ed9dff26df96ed05d9d0 Mon Sep 17 00:00:00 2001 From: Max Katz Date: Wed, 26 Aug 2026 22:01:41 +0000 Subject: [PATCH 1/2] Implement TopLevel.OpenedPopups (#22074) * Implement TopLevel.OpenedPopups * Delete Headless `GetOpenPopups` * Reorganize popup tests between Headless/Primitives layers Co-Authored-By: Claude Opus 5 * Add Popup.OpenedPopups to keep opened popups in tree structure Co-Authored-By: Claude Opus 5 * Help NUnit suppressing CS8777 * Update suppresions --------- Co-authored-by: Claude Opus 5 --- api/Avalonia.Headless.nupkg.xml | 16 +++ src/Avalonia.Controls/Primitives/Popup.cs | 40 +++++- src/Avalonia.Controls/Primitives/PopupRoot.cs | 3 + src/Avalonia.Controls/TopLevel.cs | 14 +++ .../AvaloniaHeadlessPlatform.cs | 2 - .../HeadlessWindowExtensions.cs | 12 -- .../Avalonia.Headless/HeadlessWindowImpl.cs | 21 ---- .../Avalonia.Headless/IHeadlessWindow.cs | 1 - .../Primitives/PopupTests.cs | 114 ++++++++++++++++- .../AssertHelper.cs | 16 ++- .../MouseDeviceTests.cs | 6 +- .../Avalonia.Headless.UnitTests/PopupTests.cs | 115 +++++++++++------- .../MockWindowingPlatform.cs | 1 + 13 files changed, 276 insertions(+), 85 deletions(-) create mode 100644 api/Avalonia.Headless.nupkg.xml diff --git a/api/Avalonia.Headless.nupkg.xml b/api/Avalonia.Headless.nupkg.xml new file mode 100644 index 0000000000..96c70a7793 --- /dev/null +++ b/api/Avalonia.Headless.nupkg.xml @@ -0,0 +1,16 @@ + + + + + CP0002 + M:Avalonia.Headless.HeadlessWindowExtensions.GetOpenPopups(Avalonia.Controls.TopLevel) + baseline/Avalonia.Headless/lib/net10.0/Avalonia.Headless.dll + current/Avalonia.Headless/lib/net10.0/Avalonia.Headless.dll + + + CP0002 + M:Avalonia.Headless.HeadlessWindowExtensions.GetOpenPopups(Avalonia.Controls.TopLevel) + baseline/Avalonia.Headless/lib/net8.0/Avalonia.Headless.dll + current/Avalonia.Headless/lib/net8.0/Avalonia.Headless.dll + + diff --git a/src/Avalonia.Controls/Primitives/Popup.cs b/src/Avalonia.Controls/Primitives/Popup.cs index 75dd723fe3..ba9ca5b572 100644 --- a/src/Avalonia.Controls/Primitives/Popup.cs +++ b/src/Avalonia.Controls/Primitives/Popup.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.ComponentModel; using System.Diagnostics.CodeAnalysis; using Avalonia.Reactive; @@ -153,6 +154,7 @@ namespace Avalonia.Controls.Primitives private bool _isUsingOverlayLayer; private PopupOpenState? _openState; private Action? _popupHostChangedHandler; + private List? _openedPopups; /// /// Initializes static members of the class. @@ -177,6 +179,11 @@ namespace Avalonia.Controls.Primitives internal IPopupHost? Host => _openState?.PopupHost; + /// + /// Gets the popups that are currently open directly inside this popup, in the order they were opened. + /// + public IReadOnlyList OpenedPopups => _openedPopups ?? (IReadOnlyList)[]; + /// /// Gets or sets a hint to the window manager that a shadow should be added to the popup. /// @@ -575,7 +582,12 @@ namespace Avalonia.Controls.Primitives } } - _openState = new PopupOpenState(placementTarget, topLevel, popupHost, cleanupPopup); + _openState = new PopupOpenState(placementTarget, topLevel, popupHost, cleanupPopup, FindParentPopup(placementTarget)); + + if (_openState.ParentPopup is { } parentPopup) + parentPopup.AddOpenedPopup(this); + else + topLevel.AddOpenedPopup(this); WindowManagerAddShadowHintChanged(popupHost, WindowManagerAddShadowHint); @@ -844,6 +856,11 @@ namespace Avalonia.Controls.Primitives return; } + if (_openState.ParentPopup is { } parentPopup) + parentPopup.RemoveOpenedPopup(this); + else + _openState.TopLevel.RemoveOpenedPopup(this); + _openState.Dispose(); _openState = null; @@ -1063,22 +1080,41 @@ namespace Avalonia.Controls.Primitives } } + internal void AddOpenedPopup(Popup popup) => (_openedPopups ??= new List(capacity: 2)).Add(popup); + + internal void RemoveOpenedPopup(Popup popup) => _openedPopups?.Remove(popup); + + private static Popup? FindParentPopup(Visual placementTarget) + { + foreach (var visual in placementTarget.GetSelfAndVisualAncestors()) + { + if (visual is IPopupHost) + return (visual as StyledElement)?.Parent as Popup; + } + + return null; + } + private class PopupOpenState : IDisposable { private readonly IDisposable _cleanup; private IDisposable? _presenterCleanup; private Control _placementTarget; - public PopupOpenState(Control placementTarget, TopLevel topLevel, IPopupHost popupHost, IDisposable cleanup) + public PopupOpenState(Control placementTarget, TopLevel topLevel, IPopupHost popupHost, IDisposable cleanup, + Popup? parentPopup) { PlacementTarget = placementTarget; TopLevel = topLevel; + ParentPopup = parentPopup; PopupHost = popupHost; _cleanup = cleanup; } public TopLevel TopLevel { get; } + public Popup? ParentPopup { get; } + public Control PlacementTarget { get => _placementTarget; diff --git a/src/Avalonia.Controls/Primitives/PopupRoot.cs b/src/Avalonia.Controls/Primitives/PopupRoot.cs index 234f47258d..890bc09cb7 100644 --- a/src/Avalonia.Controls/Primitives/PopupRoot.cs +++ b/src/Avalonia.Controls/Primitives/PopupRoot.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using Avalonia.Automation.Peers; using Avalonia.Controls.Primitives.PopupPositioning; using Avalonia.Diagnostics; @@ -122,6 +123,8 @@ namespace Avalonia.Controls.Primitives public TopLevel ParentTopLevel { get; } + public override IReadOnlyList OpenedPopups => (Parent as Popup)?.OpenedPopups ?? []; + /// public void Dispose() { diff --git a/src/Avalonia.Controls/TopLevel.cs b/src/Avalonia.Controls/TopLevel.cs index 2fd1524cef..721b20ccab 100644 --- a/src/Avalonia.Controls/TopLevel.cs +++ b/src/Avalonia.Controls/TopLevel.cs @@ -132,6 +132,7 @@ namespace Avalonia.Controls private TargetWeakEventSubscriber? _resourcesChangesSubscriber; private IStorageProvider? _storageProvider; private Screens? _screens; + private List? _openedPopups; private readonly PresentationSource _source; private readonly TopLevelHost _topLevelHost; internal TopLevelHost TopLevelHost => _topLevelHost; @@ -559,6 +560,14 @@ namespace Avalonia.Controls // TODO: Un-private private IPlatformSettings? PlatformSettings => AvaloniaLocator.Current.GetService(); + /// + /// Gets the popups that are currently open directly in this top level, in the order they were opened. + /// + /// + /// Use for nested popups. + /// + public virtual IReadOnlyList OpenedPopups => _openedPopups ?? (IReadOnlyList)[]; + /// /// Gets the for which the given is hosted in. /// @@ -708,6 +717,7 @@ namespace Avalonia.Controls LayoutManager.Dispose(); _platformImplBindings.Clear(); + _openedPopups = null; } /// @@ -724,6 +734,10 @@ namespace Avalonia.Controls Renderer.Resized(clientSize); } + internal void AddOpenedPopup(Popup popup) => (_openedPopups ??= new List(capacity: 2)).Add(popup); + + internal void RemoveOpenedPopup(Popup popup) => _openedPopups?.Remove(popup); + /// /// Handles a window scaling change notification from /// . diff --git a/src/Headless/Avalonia.Headless/AvaloniaHeadlessPlatform.cs b/src/Headless/Avalonia.Headless/AvaloniaHeadlessPlatform.cs index 4e14dc0f0b..2d39a9bc6b 100644 --- a/src/Headless/Avalonia.Headless/AvaloniaHeadlessPlatform.cs +++ b/src/Headless/Avalonia.Headless/AvaloniaHeadlessPlatform.cs @@ -121,8 +121,6 @@ namespace Avalonia.Headless /// /// Embeds popups to the window when set to true. The default value is true. - /// When disabled, popups are hosted in dedicated headless top-levels that are not part of - /// the parent's visual tree; use to access them. /// // TODO13: Change the default to false to match the other desktop platforms. public bool OverlayPopups { get; set; } = true; diff --git a/src/Headless/Avalonia.Headless/HeadlessWindowExtensions.cs b/src/Headless/Avalonia.Headless/HeadlessWindowExtensions.cs index 17fe5aa199..7f7232359b 100644 --- a/src/Headless/Avalonia.Headless/HeadlessWindowExtensions.cs +++ b/src/Headless/Avalonia.Headless/HeadlessWindowExtensions.cs @@ -115,18 +115,6 @@ public static class HeadlessWindowExtensions DragDropEffects effects, RawInputModifiers modifiers = RawInputModifiers.None) => RunJobsOnImpl(topLevel, w => w.DragDrop(point, type, data, effects, modifiers)); - /// - /// Returns the popups currently open directly above this toplevel, in z-order (bottom to top). - /// For popups nested in another popup, call this method on that popup's toplevel. - /// - /// - /// Only popups hosted in dedicated headless top-levels are returned, which requires disabling - /// . Popups hosted in the overlay - /// layer are part of the parent's visual tree and are not tracked by the platform. - /// - public static IReadOnlyList GetOpenPopups(this TopLevel topLevel) => - GetImpl(topLevel).GetOpenPopups(); - /// /// Changes the render scaling (DPI) of the headless window/toplevel. /// This simulates a DPI change, triggering scaling changed notifications and a layout pass. diff --git a/src/Headless/Avalonia.Headless/HeadlessWindowImpl.cs b/src/Headless/Avalonia.Headless/HeadlessWindowImpl.cs index 97a24d66b0..1ddaa4165e 100644 --- a/src/Headless/Avalonia.Headless/HeadlessWindowImpl.cs +++ b/src/Headless/Avalonia.Headless/HeadlessWindowImpl.cs @@ -27,7 +27,6 @@ namespace Avalonia.Headless private readonly AvaloniaHeadlessPlatformOptions _options; private readonly HeadlessWindowImpl? _popupParent; private readonly IPopupPositioner? _popupPositioner; - private readonly List _openPopups = new(); public bool IsPopup { get; } public HeadlessWindowImpl(AvaloniaHeadlessPlatformOptions options) @@ -60,7 +59,6 @@ namespace Avalonia.Headless public void Dispose() { - _popupParent?._openPopups.Remove(this); Closed?.Invoke(); _lastRenderedFrame?.Dispose(); _lastRenderedFrame = null; @@ -101,9 +99,6 @@ namespace Avalonia.Headless public void Show(bool activate, bool isDialog) { - if (_popupParent != null && !_popupParent._openPopups.Contains(this)) - _popupParent._openPopups.Add(this); - if (activate) { ZOrder = _nextGlobalZOrder++; @@ -113,7 +108,6 @@ namespace Avalonia.Headless public void Hide() { - _popupParent?._openPopups.Remove(this); Dispatcher.UIThread.Post(() => Deactivated?.Invoke(), DispatcherPriority.Input); } @@ -403,21 +397,6 @@ namespace Avalonia.Headless public IPopupImpl? CreatePopup() => _options.OverlayPopups ? null : new HeadlessWindowImpl(this); - public IReadOnlyList GetOpenPopups() - { - if (_openPopups.Count == 0) - return Array.Empty(); - - var result = new List(_openPopups.Count); - foreach (var popup in _openPopups) - { - if (popup.InputRoot is PresentationSource { FocusRoot: TopLevel topLevel }) - result.Add(topLevel); - } - - return result; - } - public void SetWindowManagerAddShadowHint(bool enabled) { diff --git a/src/Headless/Avalonia.Headless/IHeadlessWindow.cs b/src/Headless/Avalonia.Headless/IHeadlessWindow.cs index 5e2dd6a13d..3fe06d619c 100644 --- a/src/Headless/Avalonia.Headless/IHeadlessWindow.cs +++ b/src/Headless/Avalonia.Headless/IHeadlessWindow.cs @@ -19,6 +19,5 @@ namespace Avalonia.Headless void MouseWheel(Point point, Vector delta, RawInputModifiers modifiers = RawInputModifiers.None); void DragDrop(Point point, RawDragEventType type, IDataTransfer data, DragDropEffects effects, RawInputModifiers modifiers = RawInputModifiers.None); void SetRenderScaling(double scaling); - IReadOnlyList GetOpenPopups(); } } diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs index 3cca86d3b4..3548cd614f 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/PopupTests.cs @@ -1392,6 +1392,109 @@ namespace Avalonia.Controls.UnitTests.Primitives } } + [Fact] + public void Opened_Popup_Should_Be_In_OpenedPopups() + { + using (CreateServices()) + { + var target = new Popup(); + var window = PreparedWindow(target); + + target.Open(); + + Assert.Equal(new[] { target }, window.OpenedPopups); + + target.Close(); + + Assert.Empty(window.OpenedPopups); + } + } + + [Fact] + public void Closing_Popup_With_IsOpen_Should_Remove_It_From_OpenedPopups() + { + using (CreateServices()) + { + var target = new Popup(); + var window = PreparedWindow(target); + + target.IsOpen = true; + + Assert.Equal(new[] { target }, window.OpenedPopups); + + target.IsOpen = false; + + Assert.Empty(window.OpenedPopups); + } + } + + [Fact] + public void Closing_Window_Should_Clear_OpenedPopups() + { + using (CreateServices()) + { + var target = new Popup(); + var window = PreparedWindow(target); + + target.Open(); + window.Close(); + + Assert.Empty(window.OpenedPopups); + } + } + + [Fact] + public void Nested_Popup_Should_Be_In_Parent_Popup_OpenedPopups() + { + using (CreateServices()) + { + var nestedTarget = new Border { Width = 20, Height = 20 }; + var nestedPopup = new Popup + { + PlacementTarget = nestedTarget, + Child = new Border { Width = 10, Height = 10 } + }; + var target = new Border(); + var popup = new Popup + { + PlacementTarget = target, + Child = new Panel { Children = { nestedTarget, nestedPopup } } + }; + var window = PreparedWindow(new Panel { Children = { target, popup } }); + + popup.Open(); + + if (popup.Host is OverlayPopupHost host) + { + //Need to measure/arrange for visual children to show up + //in OverlayPopupHost + host.Measure(Size.Infinity); + host.Arrange(new Rect(host.DesiredSize)); + } + + nestedPopup.Open(); + + Assert.Equal([popup], window.OpenedPopups); + Assert.Equal([nestedPopup], popup.OpenedPopups); + Assert.Empty(nestedPopup.OpenedPopups); + + if (popup.Host is PopupRoot popupRoot) + { + // A popup root exposes the popups opened by its own popup. + Assert.Equal([nestedPopup], popupRoot.OpenedPopups); + } + + nestedPopup.Close(); + + Assert.Equal([popup], window.OpenedPopups); + Assert.Empty(popup.OpenedPopups); + + popup.Close(); + + Assert.Empty(window.OpenedPopups); + } + } + private IDisposable CreateServices() { return UnitTestApplication.Start(TestServices.StyledWindow.With( @@ -1430,13 +1533,22 @@ namespace Avalonia.Controls.UnitTests.Primitives { if (UsePopupHost) return null; - return MockWindowingPlatform.CreatePopupMock(mock.Object).Object; + return CreatePopupMock(mock.Object); }); return mock.Object; }, null); } + private static IPopupImpl CreatePopupMock(IWindowBaseImpl parent) + { + var mock = MockWindowingPlatform.CreatePopupMock(parent); + + mock.Setup(x => x.CreatePopup()).Returns(() => CreatePopupMock(mock.Object)); + + return mock.Object; + } + private static Window PreparedWindow(object? content = null) { var w = new Window { Content = content }; diff --git a/tests/Avalonia.Headless.UnitTests/AssertHelper.cs b/tests/Avalonia.Headless.UnitTests/AssertHelper.cs index 1c98afcb9a..c3ba56d05f 100644 --- a/tests/Avalonia.Headless.UnitTests/AssertHelper.cs +++ b/tests/Avalonia.Headless.UnitTests/AssertHelper.cs @@ -1,5 +1,7 @@ #nullable enable +using System.Diagnostics.CodeAnalysis; + namespace Avalonia.Headless.UnitTests; internal static class AssertHelper @@ -22,14 +24,26 @@ internal static class AssertHelper #endif } - public static void NotNull(object? value) + public static void Null(object? value) + { +#if NUNIT + Assert.That(value, Is.Null); +#elif XUNIT + Assert.Null(value); +#endif + } + + public static void NotNull([NotNull] object? value) { #if NUNIT Assert.That(value, Is.Not.Null); #elif XUNIT Assert.NotNull(value); #endif + // NUnit doesn't suppress CS8777 warning on its own +#pragma warning disable CS8777 // Parameter must have a non-null value when exiting. } +#pragma warning restore CS8777 // Parameter must have a non-null value when exiting. public static void Equal(T expected, T actual) { diff --git a/tests/Avalonia.Headless.UnitTests/MouseDeviceTests.cs b/tests/Avalonia.Headless.UnitTests/MouseDeviceTests.cs index d501635fbe..8c2fa92bb7 100644 --- a/tests/Avalonia.Headless.UnitTests/MouseDeviceTests.cs +++ b/tests/Avalonia.Headless.UnitTests/MouseDeviceTests.cs @@ -65,7 +65,11 @@ public class MouseDeviceTests // Pressing captures the pointer implicitly on the window's border. window.MouseDown(new Point(50, 50), MouseButton.Left); - window.GetOpenPopups()[0].MouseMove(new Point(40, 15)); + + var popupRoot = PopupTests.GetPopupTopLevel(popup); + AssertHelper.NotNull(popupRoot); + + popupRoot.MouseMove(new Point(40, 15)); AssertHelper.Same(TestApplication.UsesSharedMouseDevice ? target : popupChild, moveTarget); diff --git a/tests/Avalonia.Headless.UnitTests/PopupTests.cs b/tests/Avalonia.Headless.UnitTests/PopupTests.cs index 2a1536ce50..2eef10a0c4 100644 --- a/tests/Avalonia.Headless.UnitTests/PopupTests.cs +++ b/tests/Avalonia.Headless.UnitTests/PopupTests.cs @@ -32,7 +32,7 @@ public class PopupTests #elif XUNIT [AvaloniaFact] #endif - public void Popup_Uses_Dedicated_TopLevel_And_Is_Discoverable() + public void Popup_Uses_Dedicated_TopLevel() { var target = new Border { Background = Brushes.Red }; var popup = new Popup @@ -49,22 +49,22 @@ public class PopupTests window.Show(); Dispatcher.UIThread.RunJobs(); - AssertHelper.Equal(0, window.GetOpenPopups().Count); + AssertHelper.False(popup.IsOpen); + AssertHelper.False(popup.IsUsingOverlayLayer); + AssertHelper.Null(GetPopupTopLevel(popup)); popup.Open(); Dispatcher.UIThread.RunJobs(); AssertHelper.True(popup.IsOpen); AssertHelper.False(popup.IsUsingOverlayLayer); - AssertHelper.Equal(1, window.GetOpenPopups().Count); - AssertHelper.True(window.GetOpenPopups()[0] is PopupRoot); - - popup.Close(); - Dispatcher.UIThread.RunJobs(); - - AssertHelper.Equal(0, window.GetOpenPopups().Count); + AssertHelper.True(GetPopupTopLevel(popup) is PopupRoot); window.Close(); + + AssertHelper.False(popup.IsOpen); + AssertHelper.False(popup.IsUsingOverlayLayer); + AssertHelper.Null(GetPopupTopLevel(popup)); } #if NUNIT @@ -72,31 +72,40 @@ public class PopupTests #elif XUNIT [AvaloniaFact] #endif - public void Can_Click_Button_Inside_Platform_Popup() + public void Popup_Placement_Respects_Window_Position() { - var clickCount = 0; - var button = new Button { Width = 80, Height = 30 }; - button.Click += (_, _) => clickCount++; - - var target = new Border { Background = Brushes.Red }; - var popup = new Popup { PlacementTarget = target, Child = button }; + var target = new Border + { + Width = 20, + Height = 20, + HorizontalAlignment = HorizontalAlignment.Left, + VerticalAlignment = VerticalAlignment.Top, + Background = Brushes.Red + }; + var popup = new Popup + { + PlacementTarget = target, + Placement = PlacementMode.Bottom, + Child = new Border { Width = 20, Height = 20 } + }; var window = new Window { Width = 100, Height = 100, Content = new Panel { Children = { target, popup } } }; + window.Position = new PixelPoint(100, 200); window.Show(); Dispatcher.UIThread.RunJobs(); popup.Open(); Dispatcher.UIThread.RunJobs(); - var popupRoot = window.GetOpenPopups()[0]; - popupRoot.MouseDown(new Point(40, 15), MouseButton.Left); - popupRoot.MouseUp(new Point(40, 15), MouseButton.Left); + var popupRoot = GetPopupTopLevel(popup); + AssertHelper.NotNull(popupRoot); - AssertHelper.Equal(1, clickCount); + var expected = target.PointToScreen(new Point(0, target.Bounds.Height)); + AssertHelper.Equal(expected, popupRoot.PointToScreen(default)); window.Close(); } @@ -106,38 +115,33 @@ public class PopupTests #elif XUNIT [AvaloniaFact] #endif - public void Popup_Placement_Respects_Window_Position() + public void Can_Click_Button_Inside_Platform_Popup() { - var target = new Border - { - Width = 20, - Height = 20, - HorizontalAlignment = HorizontalAlignment.Left, - VerticalAlignment = VerticalAlignment.Top, - Background = Brushes.Red - }; - var popup = new Popup - { - PlacementTarget = target, - Placement = PlacementMode.Bottom, - Child = new Border { Width = 20, Height = 20 } - }; + var clickCount = 0; + var button = new Button { Width = 80, Height = 30 }; + button.Click += (_, _) => clickCount++; + + var target = new Border { Background = Brushes.Red }; + var popup = new Popup { PlacementTarget = target, Child = button }; var window = new Window { Width = 100, Height = 100, Content = new Panel { Children = { target, popup } } }; - window.Position = new PixelPoint(100, 200); window.Show(); Dispatcher.UIThread.RunJobs(); popup.Open(); Dispatcher.UIThread.RunJobs(); - var popupRoot = window.GetOpenPopups()[0]; - var expected = target.PointToScreen(new Point(0, target.Bounds.Height)); - AssertHelper.Equal(expected, popupRoot.PointToScreen(default)); + var popupRoot = GetPopupTopLevel(popup); + AssertHelper.NotNull(popupRoot); + + popupRoot.MouseDown(new Point(40, 15), MouseButton.Left); + popupRoot.MouseUp(new Point(40, 15), MouseButton.Left); + + AssertHelper.Equal(1, clickCount); window.Close(); } @@ -147,7 +151,7 @@ public class PopupTests #elif XUNIT [AvaloniaFact] #endif - public void Nested_Popup_Is_Child_Of_Popup_Root() + public void Nested_Popup_Is_Owned_By_Parent_Popup() { var nestedTarget = new Border { Width = 20, Height = 20, Background = Brushes.Green }; var nestedPopup = new Popup @@ -175,11 +179,34 @@ public class PopupTests nestedPopup.Open(); Dispatcher.UIThread.RunJobs(); - var popupRoot = window.GetOpenPopups()[0]; - AssertHelper.Equal(1, window.GetOpenPopups().Count); - AssertHelper.Equal(1, popupRoot.GetOpenPopups().Count); - AssertHelper.True(popupRoot.GetOpenPopups()[0] is PopupRoot); + AssertHelper.Equal(1, window.OpenedPopups.Count); + AssertHelper.Same(popup, window.OpenedPopups[0]); + + AssertHelper.Equal(1, popup.OpenedPopups.Count); + AssertHelper.Same(nestedPopup, popup.OpenedPopups[0]); + AssertHelper.Equal(0, nestedPopup.OpenedPopups.Count); + + // The nested popup is hosted in the parent popup's own top level. + AssertHelper.Same(GetPopupTopLevel(popup), TopLevel.GetTopLevel(nestedTarget)); + + nestedPopup.Close(); + Dispatcher.UIThread.RunJobs(); + + AssertHelper.Equal(0, popup.OpenedPopups.Count); + AssertHelper.Equal(1, window.OpenedPopups.Count); + + popup.Close(); + Dispatcher.UIThread.RunJobs(); + + AssertHelper.Equal(0, window.OpenedPopups.Count); window.Close(); } + + internal static TopLevel GetPopupTopLevel(Popup popup) + { + AssertHelper.NotNull(popup.Child); + var topLevel = TopLevel.GetTopLevel(popup.Child); + return topLevel; + } } diff --git a/tests/Avalonia.UnitTests/MockWindowingPlatform.cs b/tests/Avalonia.UnitTests/MockWindowingPlatform.cs index 298785df32..eef60186d3 100644 --- a/tests/Avalonia.UnitTests/MockWindowingPlatform.cs +++ b/tests/Avalonia.UnitTests/MockWindowingPlatform.cs @@ -101,6 +101,7 @@ namespace Avalonia.UnitTests popupImpl.Setup(x => x.Compositor).Returns(compositor); popupImpl.Setup(x => x.ClientSize).Returns(() => clientSize); popupImpl.Setup(x => x.MaxAutoSizeHint).Returns(s_screenSize); + popupImpl.Setup(x => x.DesktopScaling).Returns(1); popupImpl.Setup(x => x.RenderScaling).Returns(1); popupImpl.Setup(x => x.PopupPositioner).Returns(positioner); popupImpl.Setup(x => x.Position).Returns(()=>position); From 648edcdbc4ef0159973c0e4cfae6bc6e4fe56804 Mon Sep 17 00:00:00 2001 From: Benedikt Stebner Date: Wed, 26 Aug 2026 22:08:04 +0000 Subject: [PATCH 2/2] [Text] Do not shape the space after fallback text with the fallback font (#22067) * Add failing tests for whitespace absorbed into a fallback run A fallback run is extended for as long as the fallback font has glyphs instead of ending where the primary font regains coverage. Practically every font maps U+0020, so the space that follows fallback text is pulled into the fallback run and shaped with its space glyph. - itemization: a Hebrew letter followed by " b" must produce a 1-char fallback run, not a 2-char one that swallows the space - measurement: the space after an emoji must have the same advance as the same space elsewhere in the line * Merge abutting text bounds within the usual float tolerance The two edges being compared are reached by summing glyph advances along different paths, so abutting bounds can land an ULP apart and a single directional span gets reported as two rectangles. Compare them the way the rest of layout compares coordinates. * End a fallback run where the default typeface regains coverage Whitespace was exempt from the return-to-primary check so that a space wouldn't split a fallback run. Practically every font maps U+0020, so the exemption let a fallback run reach past the text the default typeface couldn't render and shape the following space with the fallback's own advance - a full em in most emoji fonts, which is the long space reported after an emoji. - the check now applies to spacing whitespace (Zs), so a fallback run ends at the first cluster the default typeface can render; control and format codepoints stay exempt - many fonts map the default-ignorable bidi controls, and a default typeface whose cmap merely has such a mark must not pull it out of the fallback run: the mark renders nothing either way, and the split cuts the run for no reason - a default typeface that cannot shape the run's script is still not a return target for text, but it does reclaim the spacing whitespace between the words, which carries no shaping - a run of pure whitespace no longer becomes the anti-thrashing bias for the next run, so the words on either side of a space keep resolving to the same fallback font Test expectations that encoded the old run structure move with it: a line of emoji separated by spaces is no longer a single run, glyph clusters are relative to each run's own text (the affected helpers now add the run's start), and the spaces of a right-to-left line measure with the primary font, which widens those lines. --- .../Media/TextFormatting/TextCharacters.cs | 63 ++++- .../Media/TextFormatting/TextLineImpl.cs | 7 +- .../TextFormatting/TextCharactersTests.cs | 216 ++++++++++++++++++ .../TextFormatting/TextFormatterTests.cs | 23 +- .../Media/TextFormatting/TextLayoutTests.cs | 26 ++- .../Media/TextFormatting/TextLineTests.cs | 41 ++-- 6 files changed, 332 insertions(+), 44 deletions(-) diff --git a/src/Avalonia.Base/Media/TextFormatting/TextCharacters.cs b/src/Avalonia.Base/Media/TextFormatting/TextCharacters.cs index 94fdc5d3e3..591189ae6e 100644 --- a/src/Avalonia.Base/Media/TextFormatting/TextCharacters.cs +++ b/src/Avalonia.Base/Media/TextFormatting/TextCharacters.cs @@ -67,10 +67,36 @@ namespace Avalonia.Media.TextFormatting text = text.Slice(shapeableRun.Length); - previousProperties = shapeableRun.Properties; + // Whitespace says nothing about which font the text around it wants, and it belongs to + // the default typeface whenever that covers it - so a run of pure whitespace must not + // become the anti-thrashing bias for what follows. Otherwise the words on either side + // of a space each resolve their fallback from scratch and can land on different fonts. + if (!IsWhiteSpaceOnly(shapeableRun.Text.Span)) + { + previousProperties = shapeableRun.Properties; + } } } + /// + /// Returns whether every codepoint in is whitespace. Returns on the + /// first codepoint that isn't, so a run of text costs a single lookup. + /// + private static bool IsWhiteSpaceOnly(ReadOnlySpan text) + { + var codepoints = new CodepointEnumerator(text); + + while (codepoints.MoveNext(out var codepoint)) + { + if (!codepoint.IsWhiteSpace) + { + return false; + } + } + + return true; + } + /// /// Creates a shapeable text run with unique properties. /// @@ -149,18 +175,19 @@ namespace Avalonia.Media.TextFormatting GlyphTypeface? fallbackGlyphTypeface = null; var fallbackResolved = false; - // A primary that cannot shape this tier's script is not a valid "return target": pass - // null so the return-to-primary check doesn't hand clusters back to it, which would - // otherwise block a shaping-capable fallback that merely shares the primary's cmap. + // A primary that cannot shape this tier's script is not a valid "return target" for + // text: handing clusters back to it would block a shaping-capable fallback that merely + // shares the primary's cmap. It still reclaims the spacing whitespace between the + // words, which needs no shaping - see TryGetShapeableLength. var defaultCanShape = !requireShapingCapability || defaultGlyphTypeface.CanShapeScript(firstScript); - var primaryForReturn = defaultCanShape ? defaultGlyphTypeface : null; for (var pass = 0; pass < 2; pass++) { var requireFullCluster = pass == 0; if (defaultCanShape && - TryGetShapeableLength(textSpan, defaultGlyphTypeface, null, requireFullCluster, out count)) + TryGetShapeableLength(textSpan, defaultGlyphTypeface, null, defaultCanShapeScript: false, + requireFullCluster, out count)) { // Primary font: the properties already carry this typeface, so reuse them // directly. This avoids a needless copy and preserves a custom @@ -170,7 +197,8 @@ namespace Avalonia.Media.TextFormatting if (allowPreviousTypeface && previousGlyphTypeface is not null && (!requireShapingCapability || previousGlyphTypeface.CanShapeScript(firstScript)) && - TryGetShapeableLength(textSpan, previousGlyphTypeface, primaryForReturn, requireFullCluster, out count)) + TryGetShapeableLength(textSpan, previousGlyphTypeface, defaultGlyphTypeface, defaultCanShape, + requireFullCluster, out count)) { return new UnshapedTextRun(text.Slice(0, count), defaultProperties.WithTypeface(previousTypeface!.Value), biDiLevel); @@ -200,7 +228,8 @@ namespace Avalonia.Media.TextFormatting } if (fallbackGlyphTypeface is not null && - TryGetShapeableLength(textSpan, fallbackGlyphTypeface, primaryForReturn, requireFullCluster, out count)) + TryGetShapeableLength(textSpan, fallbackGlyphTypeface, defaultGlyphTypeface, defaultCanShape, + requireFullCluster, out count)) { return new UnshapedTextRun(text.Slice(0, count), defaultProperties.WithTypeface(fallbackTypeface), biDiLevel); @@ -249,7 +278,12 @@ namespace Avalonia.Media.TextFormatting /// /// The characters to shape. /// The typeface that is used to find matching characters. - /// The default typeface. + /// The default typeface, or null when there is none to + /// return to (the probe for the default typeface itself). + /// + /// Whether the default typeface can shape this run's script. When false it only reclaims + /// spacing whitespace, which needs no shaping. + /// /// /// When true, a grapheme cluster only counts as supported when the typeface has a glyph /// for every scalar it contains (base plus combining marks); when false, only the base @@ -261,6 +295,7 @@ namespace Avalonia.Media.TextFormatting ReadOnlySpan text, GlyphTypeface glyphTypeface, GlyphTypeface? defaultGlyphTypeface, + bool defaultCanShapeScript, bool requireFullCluster, out int length) { @@ -287,8 +322,14 @@ namespace Avalonia.Media.TextFormatting var clusterText = text.Slice(currentGrapheme.Offset, currentGrapheme.Length); - if (!currentCodepoint.IsWhiteSpace - && defaultGlyphTypeface != null + // A fallback run ends where the default typeface regains coverage, spacing whitespace + // included - practically every font maps U+0020, so exempting it would let the run + // shape the following space with the fallback's own advance. A default typeface that + // cannot shape this script still reclaims that whitespace, which carries no shaping. + // Only Zs qualifies: control and format codepoints (bidi controls, prepended number + // signs) keep their cluster with the probed font. + if (defaultGlyphTypeface != null + && (defaultCanShapeScript || currentCodepoint.GeneralCategory == GeneralCategory.SpaceSeparator) && ClusterIsCovered(clusterText, currentCodepoint, defaultGlyphTypeface, requireFullCluster)) { break; diff --git a/src/Avalonia.Base/Media/TextFormatting/TextLineImpl.cs b/src/Avalonia.Base/Media/TextFormatting/TextLineImpl.cs index 2289f9508e..16f0e2690b 100644 --- a/src/Avalonia.Base/Media/TextFormatting/TextLineImpl.cs +++ b/src/Avalonia.Base/Media/TextFormatting/TextLineImpl.cs @@ -834,7 +834,10 @@ namespace Avalonia.Media.TextFormatting return false; } - if (currentBounds.Rectangle.Left == lastBounds.Rectangle.Right) + // The two edges are computed by summing glyph advances along different paths, so + // abutting bounds can land an ULP apart - compare them the way the rest of layout + // compares coordinates, or a single directional span gets reported as two. + if (MathUtilities.AreClose(currentBounds.Rectangle.Left, lastBounds.Rectangle.Right)) { foreach (var runBounds in currentBounds.TextRunBounds) { @@ -846,7 +849,7 @@ namespace Avalonia.Media.TextFormatting return true; } - if (currentBounds.Rectangle.Right == lastBounds.Rectangle.Left) + if (MathUtilities.AreClose(currentBounds.Rectangle.Right, lastBounds.Rectangle.Left)) { for (int i = 0; i < currentBounds.TextRunBounds.Count; i++) { diff --git a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextCharactersTests.cs b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextCharactersTests.cs index afa6be06af..b62ffa59c1 100644 --- a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextCharactersTests.cs +++ b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextCharactersTests.cs @@ -31,6 +31,13 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting private const string NotoSansScFont = "Avalonia.Skia.UnitTests.Fonts.NotoSansSC-Subset.ttf"; private const string NotoSansJpFont = "Avalonia.Skia.UnitTests.Fonts.NotoSansJP-Subset.ttf"; + // A colour emoji font, of the kind every platform ships: it covers the emoji block and, like + // practically every font, U+0020 - at an advance of its own that is not the primary's. + private const string EmojiFont = "Avalonia.Skia.UnitTests.Assets.TwitterColorEmoji-SVGinOT.ttf"; + + // U+1F642 ๐Ÿ™‚ โ€” covered by the emoji font only. + private const int EmojiCodepoint = 0x1F642; + // U+4E2D ไธญ โ€” a CJK ideograph covered by neither curated font, and with no platform fallback, // so it has no match at all. private const int NoMatchCodepoint = 0x4E2D; @@ -329,6 +336,215 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting } } + // A fallback run must end where the primary font regains coverage, whitespace included. + // Practically every font maps U+0020, so a run that is extended for as long as the fallback + // has glyphs swallows the space that follows the fallback text and shapes it with the + // fallback's space glyph - which is a full em in most emoji fonts. + [Fact] + public void GetShapeableCharacters_Does_Not_Absorb_Whitespace_Into_A_Fallback_Run() + { + using (Start(PrimaryFont, FallbackFont)) + { + var fontManager = FontManager.Current; + + var defaultProperties = new GenericTextRunProperties(Typeface.Default); + var defaultGlyphTypeface = defaultProperties.CachedGlyphTypeface; + var defaultFontFamily = defaultProperties.Typeface.FontFamily; + + // Preconditions: the primary lacks the Hebrew letter but covers both the space and the + // letter after it, and the fallback that covers the Hebrew letter maps the space too - + // which is what lets the fallback run reach past the letter today. + Assert.False(defaultGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(FallbackCodepoint, out _)); + Assert.True(defaultGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(' ', out _)); + Assert.True(defaultGlyphTypeface.CharacterToGlyphMap.TryGetGlyph('b', out _)); + + Assert.True(fontManager.TryMatchCharacter(FallbackCodepoint, FontStyle.Normal, FontWeight.Normal, + FontStretch.Normal, defaultFontFamily, null, out var fallbackTypeface)); + Assert.True(fontManager.TryGetGlyphTypeface(fallbackTypeface, out var fallbackGlyphTypeface)); + Assert.True(fallbackGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(' ', out _)); + + var text = (char.ConvertFromUtf32(FallbackCodepoint) + " b").AsMemory(); + + var textCharacters = new TextCharacters(text, defaultProperties); + + var results = FormattingObjectPool.Instance.TextRunLists.Rent(); + + try + { + TextRunProperties? previousProperties = null; + + textCharacters.GetShapeableCharacters(text, 0, fontManager, ref previousProperties, results); + + Assert.Equal(2, results.Count); + + // The fallback run covers the Hebrew letter only. Before the fix it was 2 characters + // long: the space was pulled into the fallback run and rendered with its metrics. + Assert.Equal(1, results[0].Length); + Assert.Equal(fallbackTypeface, results[0].Properties!.Typeface); + + // The space returns to the primary along with the rest of the text. + Assert.Equal(2, results[1].Length); + Assert.Equal(defaultProperties.Typeface, results[1].Properties!.Typeface); + } + finally + { + FormattingObjectPool.RentedList? toReturn = results; + FormattingObjectPool.Instance.TextRunLists.Return(ref toReturn); + } + } + } + + // The user-visible half of the same defect: the absorbed space is measured with the fallback + // font, so a space typed after an emoji has a different advance than the same space elsewhere + // in the line - a full em with the platform emoji fonts, and a narrower space with the emoji + // font bundled here. Either way it is not the primary's. + // https://github.com/AvaloniaUI/Avalonia/issues/14011 + [Fact] + public void FormatLine_Keeps_A_Space_After_A_Fallback_Run_At_The_Primary_Width() + { + using (Start(PrimaryFont, EmojiFont)) + { + var fontManager = FontManager.Current; + + var defaultProperties = new GenericTextRunProperties(Typeface.Default); + var defaultGlyphTypeface = defaultProperties.CachedGlyphTypeface; + + Assert.False(defaultGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(EmojiCodepoint, out _)); + + Assert.True(fontManager.TryMatchCharacter(EmojiCodepoint, FontStyle.Normal, FontWeight.Normal, + FontStretch.Normal, defaultProperties.Typeface.FontFamily, null, out var emojiTypeface)); + Assert.True(fontManager.TryGetGlyphTypeface(emojiTypeface, out var emojiGlyphTypeface)); + + // The whole point of the test: the two fonts disagree about how wide a space is, so + // whichever font shapes it is directly observable in the line width. + Assert.NotEqual(SpaceAdvanceInEm(defaultGlyphTypeface), SpaceAdvanceInEm(emojiGlyphTypeface), 3); + + var formatter = new TextFormatterImpl(); + + double Width(string text) + { + var textLine = formatter.FormatLine(new SingleBufferTextSource(text, defaultProperties), 0, + double.PositiveInfinity, new GenericTextParagraphProperties(defaultProperties)); + + Assert.NotNull(textLine); + + return textLine.WidthIncludingTrailingWhitespace; + } + + var emoji = char.ConvertFromUtf32(EmojiCodepoint); + + // Isolate the space by differencing, so the surrounding glyphs' advances cancel out. + var plainSpace = Width("a b") - Width("ab"); + var spaceAfterFallback = Width(emoji + " b") - Width(emoji + "b"); + + Assert.Equal(plainSpace, spaceAfterFallback, 3); + } + } + + private static double SpaceAdvanceInEm(GlyphTypeface glyphTypeface) + { + Assert.True(glyphTypeface.CharacterToGlyphMap.TryGetGlyph(' ', out var glyph)); + Assert.True(glyphTypeface.TryGetHorizontalGlyphAdvance(glyph, out var advance)); + + return (double)advance / glyphTypeface.Metrics.DesignEmHeight; + } + + // The previous run's font is reused as an anti-thrashing bias. A space belongs to the primary + // font, so it forms a run of its own between two fallback words - and that run must not become + // the bias, or each word re-runs the fallback search and the two can land on different fonts. + [Fact] + public void GetShapeableCharacters_Keeps_The_Previous_Fallback_Across_A_Space() + { + using (Start(PrimaryFont, NotoSansScFont, NotoSansJpFont)) + { + var fontManager = FontManager.Current; + + var defaultProperties = new GenericTextRunProperties(Typeface.Default); + + // The previous run resolved to the Simplified-Chinese font. + var scTypeface = new Typeface(new FontFamily("fonts:SystemFonts#Noto Sans SC")); + Assert.True(fontManager.TryGetGlyphTypeface(scTypeface, out var scGlyphTypeface)); + + const int han = 0x4E2D; // ไธญ, covered by both regional fonts. + + // Preconditions: the primary covers the space but not the ideograph, the previous font + // covers the ideograph, and a fresh search for it would pick the *other* font - so the + // font of the second run tells us whether the bias survived the space. + Assert.True(defaultProperties.CachedGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(' ', out _)); + Assert.False(defaultProperties.CachedGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(han, out _)); + Assert.True(scGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(han, out _)); + + Assert.True(fontManager.TryMatchCharacter(han, FontStyle.Normal, FontWeight.Normal, + FontStretch.Normal, defaultProperties.Typeface.FontFamily, null, out var freshMatch)); + Assert.True(fontManager.TryGetGlyphTypeface(freshMatch, out var freshGlyphTypeface)); + Assert.Equal("Noto Sans JP", freshGlyphTypeface.FamilyName); + + var text = (" " + char.ConvertFromUtf32(han)).AsMemory(); + + var textCharacters = new TextCharacters(text, defaultProperties); + + var results = FormattingObjectPool.Instance.TextRunLists.Rent(); + + try + { + TextRunProperties? previousProperties = new GenericTextRunProperties(scTypeface); + + textCharacters.GetShapeableCharacters(text, 0, fontManager, ref previousProperties, results); + + Assert.Equal(2, results.Count); + + Assert.Equal(1, results[0].Length); + Assert.Equal(defaultProperties.Typeface, results[0].Properties!.Typeface); + + Assert.True(fontManager.TryGetGlyphTypeface(results[1].Properties!.Typeface, out var runGlyphTypeface)); + Assert.Equal("Noto Sans SC", runGlyphTypeface.FamilyName); + } + finally + { + FormattingObjectPool.RentedList? toReturn = results; + FormattingObjectPool.Instance.TextRunLists.Return(ref toReturn); + } + } + } + + // Only spacing whitespace (Zs) returns to the default typeface. Codepoint.IsWhiteSpace also + // covers control and format codepoints - including the default-ignorable bidi controls, which + // many fonts map. A default typeface that cannot shape the script must not pull a + // right-to-left mark out of the fallback run just because its cmap has it: the mark renders + // nothing either way, and splitting there cuts the run for no reason. + [Fact] + public void TryGetShapeableLength_Does_Not_Reclaim_A_Bidi_Control_As_Whitespace() + { + using (Start(PrimaryFont, FallbackFont)) + { + // DejaVu Sans plays the default: its cmap has the Arabic letter, the right-to-left + // mark and the space, but the test probes the tier where it cannot shape Arabic. + // Cascadia Code plays the probed fallback; it has the letter and needs no glyph for + // the default-ignorable mark. + var defaultGlyphTypeface = new Typeface(FontFamily.Parse( + "resm:Avalonia.Skia.UnitTests.Fonts?assembly=Avalonia.Skia.UnitTests#DejaVu Sans")).GlyphTypeface; + var probedGlyphTypeface = new Typeface(FontFamily.Parse( + "resm:Avalonia.Skia.UnitTests.Fonts?assembly=Avalonia.Skia.UnitTests#Cascadia Code")).GlyphTypeface; + + const int alef = 0x0627; + const int rightToLeftMark = 0x200F; + + Assert.True(probedGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(alef, out _)); + Assert.True(defaultGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(rightToLeftMark, out _)); + Assert.True(defaultGlyphTypeface.CharacterToGlyphMap.TryGetGlyph(' ', out _)); + + // Letter, mark, letter, then a space: the mark stays inside the fallback run, the + // space still returns to the default. + var text = "ุงโ€ุง z"; + + Assert.True(TextCharacters.TryGetShapeableLength(text.AsSpan(), probedGlyphTypeface, + defaultGlyphTypeface, defaultCanShapeScript: false, requireFullCluster: true, + out var length)); + + Assert.Equal(3, length); + } + } + // A spread of combining marks (all grapheme-cluster Extend) likely present in a broad fallback // font but absent from a minimal monospace primary. The F1 test picks the first workable one. private static readonly int[] CombiningMarkCandidates = diff --git a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextFormatterTests.cs b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextFormatterTests.cs index 182c633418..0ffa620822 100644 --- a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextFormatterTests.cs +++ b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextFormatterTests.cs @@ -319,7 +319,7 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting } [Fact] - public void Should_Produce_A_Single_Fallback_Run() + public void Should_Not_Absorb_Whitespace_Into_A_Fallback_Run() { using (Start()) { @@ -337,7 +337,18 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting Assert.NotNull(textLine); - Assert.Equal(1, textLine.TextRuns.Count); + // Four emoji in a fallback font, separated by three spaces the primary font covers: + // the spaces keep the primary's metrics instead of the emoji font's, so they form + // runs of their own. + Assert.Equal(7, textLine.TextRuns.Count); + + for (var i = 0; i < textLine.TextRuns.Count; i++) + { + var isSpace = i % 2 == 1; + + Assert.Equal(isSpace ? 1 : 2, textLine.TextRuns[i].Length); + Assert.Equal(isSpace, defaultProperties.Typeface == textLine.TextRuns[i].Properties!.Typeface); + } } } @@ -410,11 +421,15 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting } } + // The expectations concatenate the run texts in visual run order, so where the spaces sit + // in the string depends on how the line is cut into runs. Each space is a run of its own + // now (the primary font owns it, not the Hebrew fallback), which regroups the same + // characters - the right-to-left rows below show the same content, differently split. [Theory] [InlineData("one ืฉืชื™ื™ื three ืืจื‘ืข", "one ืฉืชื™ื™ื thrโ€ฆ", FlowDirection.LeftToRight, false)] - [InlineData("one ืฉืชื™ื™ื three ืืจื‘ืข", "โ€ฆthrืฉืชื™ื™ื one", FlowDirection.RightToLeft, false)] + [InlineData("one ืฉืชื™ื™ื three ืืจื‘ืข", "โ€ฆthr ืฉืชื™ื™ื one", FlowDirection.RightToLeft, false)] [InlineData("one ืฉืชื™ื™ื three ืืจื‘ืข", "one ืฉืชื™ื™ืโ€ฆ", FlowDirection.LeftToRight, true)] - [InlineData("one ืฉืชื™ื™ื three ืืจื‘ืข", "โ€ฆืฉืชื™ื™ื one", FlowDirection.RightToLeft, true)] + [InlineData("one ืฉืชื™ื™ื three ืืจื‘ืข", "โ€ฆ ืฉืชื™ื™ื one", FlowDirection.RightToLeft, true)] public void TextTrimming_Should_Trim_Correctly(string text, string trimmed, FlowDirection direction, bool wordEllipsis) { const double Width = 160.0; diff --git a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLayoutTests.cs b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLayoutTests.cs index 1d33defa83..d8732ef8bc 100644 --- a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLayoutTests.cs +++ b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLayoutTests.cs @@ -475,7 +475,7 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting [Theory] [InlineData("โ˜๐Ÿฟ", new int[] { 0 })] - [InlineData("โ˜๐Ÿฟ ab", new int[] { 0, 3, 0, 1 })] + [InlineData("โ˜๐Ÿฟ ab", new int[] { 0, 0, 1, 2 })] [InlineData("ab โ˜๐Ÿฟ", new int[] { 0, 1, 2, 0 })] public void Should_Create_Valid_Clusters_For_Text(string text, int[] clusters) { @@ -985,9 +985,13 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting var textLine = layout.TextLines[0]; + // Runs come in visual order, so TextRuns[0] is the leftmost one - the last word of + // this right-to-left line. Its glyph clusters are relative to its own text, so the + // run's start has to be added to compare them with a text source index. var firstRun = (ShapedTextRun)textLine.TextRuns[0]; - var firstCluster = firstRun.ShapedBuffer[0].GlyphCluster; + var firstCluster = TextTestHelper.GetStartCharIndex(firstRun.Text) + + firstRun.ShapedBuffer[0].GlyphCluster; var characterHit = textLine.GetCharacterHitFromDistance(0); @@ -1059,13 +1063,13 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting return rawClusters; } - // Clusters can be either run-local or text-source relative depending on split history. - if (rawClusters.Min() < runStart) - { - return rawClusters.Select(cluster => cluster + runStart); - } + // A run's clusters are relative to the text it was shaped from, which is its + // own text for a freshly shaped run but the parent's for a split child. The + // smallest cluster is the run's first character either way, so rebasing on it + // maps both onto text source indices. + var baseCluster = rawClusters.Min(); - return rawClusters; + return rawClusters.Select(cluster => cluster - baseCluster + runStart); }).ToList(); var glyphAdvances = shapedRuns.SelectMany(x => x.ShapedBuffer, (_, glyph) => glyph.GlyphAdvance).ToList(); @@ -1101,8 +1105,10 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting [InlineData("mgfg๐Ÿงdf f sdf", "g๐Ÿงd", 20, 40)] [InlineData("ูˆู‡. ูˆู‚ุฏ ุชุนุฑุถ ู„ุงู†ุชู‚ุงุฏุงุช", "ุฏุงุช", 5, 30)] [InlineData("ูˆู‡. ูˆู‚ุฏ ุชุนุฑุถ ู„ุงู†ุชู‚ุงุฏุงุช", "ุชุนุฑุถ", 20, 50)] - [InlineData(" ุนู„ู…ูŠุฉ ๐Ÿ˜ฑูˆู…ุถู„ู„ุฉ ุŒ", " ุนู„ู…ูŠุฉ ๐Ÿ˜ฑูˆู…ุถู„ู„ุฉ ุŒ", 40, 100)] - [InlineData("ููŠ ุนุงู… 2018 ุŒ ุฑูุนุช ู„", "ููŠ ุนุงู… 2018 ุŒ ุฑูุนุช ู„", 100, 120)] + // The spaces of an Arabic line are drawn with the primary font rather than the Arabic + // fallback, which is wider at this size - hence the bands sit above where they used to. + [InlineData(" ุนู„ู…ูŠุฉ ๐Ÿ˜ฑูˆู…ุถู„ู„ุฉ ุŒ", " ุนู„ู…ูŠุฉ ๐Ÿ˜ฑูˆู…ุถู„ู„ุฉ ุŒ", 80, 120)] + [InlineData("ููŠ ุนุงู… 2018 ุŒ ุฑูุนุช ู„", "ููŠ ุนุงู… 2018 ุŒ ุฑูุนุช ู„", 120, 150)] [Theory] public void HitTestTextRange_Range_ValidLength(string text, string textToSelect, double minWidth, double maxWidth) { diff --git a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLineTests.cs b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLineTests.cs index 1ec027a12c..6dc0ac4f2e 100644 --- a/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLineTests.cs +++ b/tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLineTests.cs @@ -1413,11 +1413,14 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting foreach (var textRun in shapedTextRuns) { + // Glyph clusters are relative to the run's own text, so they only line up across a + // multi-run line once the run's start is added - same as BuildGlyphClusters. + var runOffset = TextTestHelper.GetStartCharIndex(textRun.Text); var shapedBuffer = textRun.ShapedBuffer; for (var index = 0; index < shapedBuffer.Length; index++) { - var currentCluster = shapedBuffer[index].GlyphCluster; + var currentCluster = shapedBuffer[index].GlyphCluster + runOffset; var advance = shapedBuffer[index].GlyphAdvance; @@ -1427,13 +1430,10 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting } else { - var rect = rects[index - 1]; - - rects.Remove(rect); - - rect = rect.WithWidth(rect.Width + advance); + // Another glyph of the cluster that produced the last rect: widen it. + var rect = rects[rects.Count - 1]; - rects.Add(rect); + rects[rects.Count - 1] = rect.WithWidth(rect.Width + advance); } currentX += advance; @@ -1571,32 +1571,36 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting Assert.NotNull(textLine); - var textBounds = textLine.GetTextBounds(0, 4); + // Runs come in visual order: the Latin word sits leftmost, then the space, then the + // Hebrew word. The space belongs to the primary font, so it is a run of its own. + var latinRun = Assert.IsType(textLine.TextRuns[0]); + var spaceRun = Assert.IsType(textLine.TextRuns[1]); + var hebrewRun = Assert.IsType(textLine.TextRuns[2]); - var secondRun = Assert.IsType(textLine.TextRuns[1]); + var hebrewAndSpaceWidth = hebrewRun.Size.Width + spaceRun.Size.Width; + + var textBounds = textLine.GetTextBounds(0, 4); Assert.Equal(1, textBounds.Count); - Assert.Equal(secondRun.Size.Width, textBounds.Sum(x => x.Rectangle.Width)); + Assert.Equal(hebrewAndSpaceWidth, textBounds.Sum(x => x.Rectangle.Width)); textBounds = textLine.GetTextBounds(4, 3); - var firstRun = Assert.IsType(textLine.TextRuns[0]); - Assert.Equal(1, textBounds.Count); Assert.Equal(3, textBounds[0].TextRunBounds.Sum(x => x.Length)); - Assert.Equal(firstRun.Size.Width, textBounds.Sum(x => x.Rectangle.Width)); + Assert.Equal(latinRun.Size.Width, textBounds.Sum(x => x.Rectangle.Width)); textBounds = textLine.GetTextBounds(0, 5); Assert.Equal(2, textBounds.Count); Assert.Equal(5, textBounds.Sum(x => x.TextRunBounds.Sum(x => x.Length))); - Assert.Equal(secondRun.Size.Width, textBounds[1].Rectangle.Width); + Assert.Equal(hebrewAndSpaceWidth, textBounds[1].Rectangle.Width); Assert.Equal(7.201171875, textBounds[0].Rectangle.Width); Assert.Equal(textLine.Start + 7.201171875, textBounds[0].Rectangle.Right, 2); - Assert.Equal(textLine.Start + firstRun.Size.Width, textBounds[1].Rectangle.Left, 2); + Assert.Equal(textLine.Start + latinRun.Size.Width, textBounds[1].Rectangle.Left, 2); textBounds = textLine.GetTextBounds(0, text.Length); @@ -1737,13 +1741,16 @@ namespace Avalonia.Skia.UnitTests.Media.TextFormatting Assert.Equal(1, bounds.Count); - Assert.Equal(71.165859375, bounds[0].Rectangle.Right); + // The space between the Hebrew word and the digits is drawn with the primary font + // rather than the Hebrew fallback, which is 4.08 wider at this size, so everything + // laid out after it sits that much further right. + Assert.Equal(75.247031249999992, bounds[0].Rectangle.Right); bounds = textLine.GetTextBounds(11, 1); Assert.Equal(1, bounds.Count); - Assert.Equal(71.165859375, bounds[0].Rectangle.Left); + Assert.Equal(75.247031249999992, bounds[0].Rectangle.Left); bounds = textLine.GetTextBounds(0, 25);