From 8056ce54875258e63879d38d8333db3e35f3bf52 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Wed, 10 Jun 2026 22:28:09 +0200 Subject: [PATCH] Stop filter-popup clicks from reaching the column header; use a Flyout Clicking inside the open flags filter popup could act on the DataGridColumnHeader underneath: unhandled press/release pairs bubbled out of the popup into the header (which treats them as a sort click and flashes its pressed visual), and on X11 overlay popups the light-dismiss machinery could even re-target a click's press directly at the header while the popup stayed open. The filter UI is now hosted in a Flyout attached to the funnel icon instead of a hand-rolled Popup parked in the header's panel. Avalonia positions Popup as the low-level primitive and recommends Flyout for attached pickers: light dismiss, Escape-to-close, focus handling, and theme-correct presenter chrome come built in (the old hard-coded white background was also wrong in dark mode). The flyout content swallows unhandled wheel and press/release events, since its internal popup is still logically parented to the funnel inside the header. A headless end-to-end regression test drives a real DataGrid with overlay popup hosts (the X11 OverlayPopups=true configuration the app runs with) and raw input through the funnel and every visible flyout control, asserting the header never sorts and never receives an unhandled press or release. Assisted-by: Claude:claude-fable-5[1m]:Claude Code --- .../MetadataFilterRowEndToEndTests.cs | 139 ++++++++++++++++++ ILSpy/Metadata/MetadataColumnBuilder.cs | 97 ++++++------ 2 files changed, 181 insertions(+), 55 deletions(-) diff --git a/ILSpy.Tests/Metadata/MetadataFilterRowEndToEndTests.cs b/ILSpy.Tests/Metadata/MetadataFilterRowEndToEndTests.cs index 9496003c7..b11a6270e 100644 --- a/ILSpy.Tests/Metadata/MetadataFilterRowEndToEndTests.cs +++ b/ILSpy.Tests/Metadata/MetadataFilterRowEndToEndTests.cs @@ -17,10 +17,18 @@ // DEALINGS IN THE SOFTWARE. using System.Linq; +using System.Reflection; +using Avalonia; using Avalonia.Controls; +using Avalonia.Controls.Primitives; +using Avalonia.Headless; using Avalonia.Headless.NUnit; +using Avalonia.Input; using Avalonia.LogicalTree; +using Avalonia.Styling; +using Avalonia.Threading; +using Avalonia.VisualTree; using AwesomeAssertions; @@ -40,6 +48,137 @@ public class MetadataFilterRowEndToEndTests public string Name { get; set; } = ""; } + sealed class FlagsRow + { + public int RID { get; set; } + public TypeAttributes Attributes { get; set; } + } + + /// + /// The funnel icon hosting the attached filter flyout for the Attributes column. + /// + static Border FindAttributesFunnel(Visual headerPanel) => + headerPanel.GetVisualDescendants().OfType() + .First(b => ToolTip.GetTip(b) as string == "Filter Attributes"); + + /// + /// Forces the flyout's internal popup into the owner window's overlay layer — the + /// configuration the app actually runs with (Program.cs sets + /// X11PlatformOptions.OverlayPopups = true), where all popup input flows through the + /// owner window's pipeline. The style reaches the popup through the logical tree. + /// + static void ForceOverlayPopups(Window window) => + window.Styles.Add(new Style(x => x.OfType()) { + Setters = { new Setter(Popup.ShouldUseOverlayLayerProperty, true) }, + }); + + [AvaloniaTest] + public void Clicking_Inside_The_Popup_Never_Sorts_The_Column_Of_A_Real_DataGrid() + { + // Full assembly of the real parts: an actual DataGrid with CanUserSortColumns + // (as MetadataTablePage.axaml configures it), the builder's columns, the overlay + // popup host the app runs with, and real input. Opening the popup via the funnel + // and clicking a chip inside it must not raise DataGrid.Sorting. + var page = new MetadataTablePageModel(); + MetadataColumnBuilder.Populate(page); + + var grid = new DataGrid { + ItemsSource = new System.Collections.Generic.List { + new() { RID = 1, Attributes = TypeAttributes.Public }, + new() { RID = 2, Attributes = TypeAttributes.Sealed }, + }, + CanUserSortColumns = true, + }; + foreach (var column in page.Columns) + grid.Columns.Add(column); + var window = new Window { Content = grid, Width = 1000, Height = 600 }; + ForceOverlayPopups(window); + window.Show(); + window.UpdateLayout(); + + int sortingRaised = 0; + grid.Sorting += (_, _) => sortingRaised++; + // An unhandled press reaching a column header is user-visible even without a + // sort: DataGridColumnHeader sets IsPressed and flashes its :pressed background. + // These listeners mirror the header's own subscriptions (bubble, skip handled), + // so any hit here is a press the header would visibly react to. + int headerUnhandledPresses = 0, headerUnhandledReleases = 0; + foreach (var header in grid.GetVisualDescendants().OfType()) + { + header.AddHandler(InputElement.PointerPressedEvent, (_, _) => headerUnhandledPresses++); + header.AddHandler(InputElement.PointerReleasedEvent, (_, _) => headerUnhandledReleases++); + } + + var attributesColumn = page.Columns.Single(c => (string?)c.Tag == "Attributes"); + var headerPanel = (StackPanel)attributesColumn.Header!; + + // Open the flyout with a real click on the funnel icon (the Border carrying the + // "Filter Attributes" tooltip). + var funnel = FindAttributesFunnel(headerPanel); + var flyout = (Flyout)FlyoutBase.GetAttachedFlyout(funnel)!; + var funnelCenter = funnel.TranslatePoint(new Point(funnel.Bounds.Width / 2, funnel.Bounds.Height / 2), window)!.Value; + window.MouseDown(funnelCenter, MouseButton.Left); + window.MouseUp(funnelCenter, MouseButton.Left); + flyout.IsOpen.Should().BeTrue("setup precondition — the funnel click must open the flyout"); + window.UpdateLayout(); + Dispatcher.UIThread.RunJobs(); + sortingRaised.Should().Be(0, "the funnel click that opens the flyout must not sort the column"); + + // Click every interactive control inside the flyout: mutex chips (ToggleButton), + // tri-state pills and Clear (Button), plus the non-interactive hint TextBlock. + // The mode ComboBox goes last because clicking it opens its own dropdown over + // the other controls. + // The flyout body scrolls (MaxHeight 400) and TypeAttributes has more chip groups + // than fit, so restrict the sweep to controls whose center actually lies within + // the flyout's on-screen rect — clicking a scrolled-out control's nominal position + // is a click outside the flyout and legitimately light-dismisses. + var popupContent = (Visual)flyout.Content!; + Rect PopupRect() + { + var topLeft = popupContent.TranslatePoint(default, window)!.Value; + return new Rect(topLeft, popupContent.Bounds.Size); + } + var targets = popupContent.GetVisualDescendants() + .Where(v => v is Button || (v is TextBlock { Text: { } hintText } && hintText.StartsWith("Click a chip"))) + .Concat(popupContent.GetVisualDescendants().Where(v => v is ComboBox)) + .Cast() + .ToList(); + targets.Should().NotBeEmpty("setup precondition — the popup must expose clickable controls"); + int clicked = 0; + foreach (var target in targets) + { + // Recompute at click time: a previous click may have re-flowed the summary + // line and shifted everything below it. + window.UpdateLayout(); + var center = target.TranslatePoint(new Point(target.Bounds.Width / 2, target.Bounds.Height / 2), window); + if (center is not { } point || !PopupRect().Contains(point)) + continue; + clicked++; + window.MouseDown(point, MouseButton.Left); + window.MouseUp(point, MouseButton.Left); + flyout.IsOpen.Should().BeTrue( + $"clicking the {target.GetType().Name} inside the flyout must not close it"); + // ProcessSort is dispatched via Dispatcher.UIThread.Post — drain per click so + // a leaked sort is attributed to the control that caused it. + Dispatcher.UIThread.RunJobs(); + sortingRaised.Should().Be(0, + $"clicking the {target.GetType().Name} inside the popup must not sort the column"); + headerUnhandledPresses.Should().Be(0, + $"clicking the {target.GetType().Name} inside the popup must not deliver an unhandled press to a column header (the header would flash its pressed visual)"); + headerUnhandledReleases.Should().Be(0, + $"clicking the {target.GetType().Name} inside the popup must not deliver an unhandled release to a column header"); + } + clicked.Should().BeGreaterThan(2, "setup precondition — the sweep must actually click several visible controls"); + + // ProcessSort is dispatched via Dispatcher.UIThread.Post — drain the queue so a + // leaked sort would actually fire before the assertion. + Dispatcher.UIThread.RunJobs(); + + sortingRaised.Should().Be(0, + "neither opening the filter popup nor clicking controls inside it may sort the column"); + } + + [AvaloniaTest] public void Typing_Into_The_Header_TextBox_Drives_The_Matching_ColumnFilter_Text() { diff --git a/ILSpy/Metadata/MetadataColumnBuilder.cs b/ILSpy/Metadata/MetadataColumnBuilder.cs index fb68f12c9..818b6b229 100644 --- a/ILSpy/Metadata/MetadataColumnBuilder.cs +++ b/ILSpy/Metadata/MetadataColumnBuilder.cs @@ -135,8 +135,7 @@ namespace ILSpy.Metadata // asks for it — clicking the funnel focuses the textbox, after which it stays // visible while focused or while a filter is set — so brushing the cursor // across the column name does not swap the label out. For [Flags] columns the - // input is just the dropdown trigger; the popup is parked invisibly so it can - // remain anchored to the trigger button. + // funnel is just the trigger for the filter flyout attached to it. var label = new TextBlock { Text = columnName, FontWeight = FontWeight.SemiBold, @@ -182,16 +181,17 @@ namespace ILSpy.Metadata bool isFlagsColumn = propertyType.IsEnum && Attribute.IsDefined(propertyType, typeof(FlagsAttribute)); - Popup? popup = null; + Flyout? flyout = null; TextBox? textBox = null; Control headerContent; if (isFlagsColumn) { // Flags columns: label stays visible always — the funnel icon alone is the - // popup trigger, no separate dropdown chevron needed since the popup carries - // the entire filter UI. PlacementTarget is the funnel itself so the popup + // flyout trigger, no separate dropdown chevron needed since the flyout + // carries the entire filter UI. Attached to the funnel itself so the flyout // drops below the icon's hit area. - popup = BuildFlagsPopup(filter, propertyType, filterIconHost, pageKey, columnName); + flyout = BuildFlagsFlyout(filter, propertyType, pageKey, columnName); + FlyoutBase.SetAttachedFlyout(filterIconHost, flyout); headerContent = label; } else @@ -217,14 +217,14 @@ namespace ILSpy.Metadata headerRow.Children.Add(headerContent); filterIconHost.PointerPressed += (_, e) => { - if (popup != null) + if (flyout != null) { - // Flag columns: the funnel always opens the popup. Modifying the filter - // requires the chip UI inside the popup, and clearing is owned by the - // popup's own Clear button — calling FlagsState.Clear() from out here + // Flag columns: the funnel always opens the flyout. Modifying the filter + // requires the chip UI inside the flyout, and clearing is owned by the + // flyout's own Clear button — calling FlagsState.Clear() from out here // would leave the chip IsChecked state stale because we'd skip the // SyncFromState() pass that FlagsFilterPopup.Clear() does. - popup.IsOpen = true; + FlyoutBase.ShowAttachedFlyout(filterIconHost); } else { @@ -240,13 +240,8 @@ namespace ILSpy.Metadata e.Handled = true; }; - // The popup needs to live inside the visual tree but doesn't contribute layout. - // Park it in a Panel sibling so the header row's height stays driven solely by - // label / input. var root = new StackPanel { Orientation = Orientation.Vertical }; root.Children.Add(headerRow); - if (popup != null) - root.Children.Add(popup); bool popupOpen = false; bool focusInside = false; @@ -269,9 +264,9 @@ namespace ILSpy.Metadata // Icon affordance: the X form indicates "click to clear". That only matches // the click behaviour on text columns (where clicking when active does clear); - // for flag columns the click always opens the popup, so we keep the funnel + // for flag columns the click always opens the flyout, so we keep the funnel // shape and just tint it SteelBlue to signal the filter is active. - bool useXForm = active && popup is null; + bool useXForm = active && flyout is null; if (useXForm) { filterIcon.Data = xGeometry; @@ -296,10 +291,10 @@ namespace ILSpy.Metadata textBox.LostFocus += (_, _) => { focusInside = false; Update(); }; } filter.PropertyChanged += (_, _) => Update(); - if (popup != null) + if (flyout != null) { - popup.Opened += (_, _) => { popupOpen = true; Update(); }; - popup.Closed += (_, _) => { popupOpen = false; Update(); }; + flyout.Opened += (_, _) => { popupOpen = true; Update(); }; + flyout.Closed += (_, _) => { popupOpen = false; Update(); }; } Update(); @@ -331,62 +326,54 @@ namespace ILSpy.Metadata return box; } - static Popup BuildFlagsPopup(ColumnFilter filter, Type enumType, Control placementTarget, string? pageKey, string columnName) + static Flyout BuildFlagsFlyout(ColumnFilter filter, Type enumType, string? pageKey, string columnName) { - // Schema-driven popup: FlagsFilterPopup distinguishes mutex sub-ranges + // Schema-driven flyout: FlagsFilterPopup distinguishes mutex sub-ranges // (multi-select chips) from independent flags (tri-state pills) and drives - // ColumnFilter.FlagsState. The funnel icon owns the open gesture, so this - // helper only assembles the Popup itself — no separate trigger button needed. + // ColumnFilter.FlagsState. A Flyout (not a raw Popup) supplies light dismiss, + // Escape-to-close, focus handling, and theme-correct presenter chrome. var schema = ILSpy.Metadata.Filters.FlagsSchemaInferer.For(enumType); bool freshlyCreated = filter.FlagsState is null; filter.FlagsState ??= new ILSpy.Metadata.Filters.FilterState(schema); - // On first-popup-open, restore any persisted state for this (table, column). + // On first build, restore any persisted state for this (table, column). // Subsequent opens already carry the live state in-memory. Then subscribe so // later mutations write back to SessionSettings. if (freshlyCreated && pageKey != null) ApplyPersistedFilterState(filter.FlagsState, pageKey, columnName); var popupContent = new ILSpy.Views.Filters.FlagsFilterPopup(filter.FlagsState); - var popupRoot = new Border { - BorderBrush = Brushes.Gray, - BorderThickness = new Thickness(1), - Background = Brushes.White, - // Force the arrow cursor on the popup surface — without this, the + var flyoutContent = new ScrollViewer { + MaxHeight = 400, + Content = popupContent, + // Force the arrow cursor on the flyout surface — without this, the // EW-resize cursor set on the DataGrid column header's drag-grip can - // leak into the popup if the pointer enters from there before Avalonia + // leak into the flyout if the pointer enters from there before Avalonia // recomputes the cursor for the new hit-test target. Cursor = new global::Avalonia.Input.Cursor(global::Avalonia.Input.StandardCursorType.Arrow), - Child = new ScrollViewer { - MaxHeight = 400, - Content = popupContent, - }, }; - // Stop wheel events from bubbling out of the popup. Without this, scrolling + // Stop wheel events from bubbling out of the flyout. Without this, scrolling // inside the dropdown also scrolls the underlying DataGrid because the // PointerWheelChanged event keeps bubbling up the routed-event tree once the // inner ScrollViewer has consumed (or ignored) it. - popupRoot.AddHandler(global::Avalonia.Input.InputElement.PointerWheelChangedEvent, + flyoutContent.AddHandler(global::Avalonia.Input.InputElement.PointerWheelChangedEvent, (_, e) => e.Handled = true, handledEventsToo: true); - var popup = new Popup { - PlacementTarget = placementTarget, + // Likewise for pointer clicks: the flyout's internal popup is logically + // parented to the funnel inside the column header, so its routed events bubble + // on into the DataGridColumnHeader, which treats an unhandled left-button + // press/release pair as a sort click. Interactive children (chips, the Clear + // button) handle their own pointer events; swallow whatever reaches the + // content root unhandled — clicks on padding, hint, and summary text. + flyoutContent.AddHandler(global::Avalonia.Input.InputElement.PointerPressedEvent, + (_, e) => e.Handled = true); + flyoutContent.AddHandler(global::Avalonia.Input.InputElement.PointerReleasedEvent, + (_, e) => e.Handled = true); + + return new Flyout { + Content = flyoutContent, Placement = PlacementMode.BottomEdgeAlignedLeft, - IsLightDismissEnabled = true, - Child = popupRoot, + ShowMode = FlyoutShowMode.Transient, }; - // Escape closes the popup — matches the standard popup convention. Tunnel - // routing on the popup root so the key is intercepted before any inner control - // (e.g. the TextBox in a flag-name search field) tries to consume it. - popupRoot.AddHandler(global::Avalonia.Input.InputElement.KeyDownEvent, - (_, e) => { - if (e.Key == global::Avalonia.Input.Key.Escape) - { - popup.IsOpen = false; - e.Handled = true; - } - }, - global::Avalonia.Interactivity.RoutingStrategies.Tunnel); - return popup; } ///