From b450072b0bc811fe2d19ab698d09f528f571a564 Mon Sep 17 00:00:00 2001 From: v-leafshi Date: Tue, 12 May 2026 13:42:02 +0800 Subject: [PATCH 1/2] Revert "Fix ToolStripDropDownMenu scroll boundary handling to prevent out-of-range and unintended dropdown dismissal (#14490)" This reverts commit cc1a80576b460449fde3145040ff3fc1f0d4c27a. --- .../ToolStrips/ToolStripDropDownMenu.cs | 101 +++----------- .../ToolStrips/ToolStripScrollButton.cs | 5 - .../ToolStripDropDownMenuTests.cs | 128 +----------------- 3 files changed, 22 insertions(+), 212 deletions(-) diff --git a/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs b/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs index 84f0a338aa7..d4f967fecbb 100644 --- a/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs +++ b/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs @@ -464,10 +464,8 @@ internal override void ChangeSelection(ToolStripItem? nextItem) if (nextItem is not null) { Rectangle displayRect = DisplayRectangle; - // Use IsItemFullyVisible so that an item whose top or bottom edge lies exactly - // on the display boundary is not treated as out-of-view, preventing an - // unnecessary scroll when the item is already fully shown. - if (!IsItemFullyVisible(displayRect, nextItem.Bounds)) + if (!displayRect.Contains(displayRect.X, nextItem.Bounds.Top) + || !displayRect.Contains(displayRect.X, nextItem.Bounds.Bottom)) { int delta; if (displayRect.Y > nextItem.Bounds.Top) @@ -481,15 +479,8 @@ internal override void ChangeSelection(ToolStripItem? nextItem) int index = Items.IndexOf(nextItem); while (index >= 0) { - // Adjust the item's bounds by the prospective scroll delta so we can - // test whether it would still overlap the display area after scrolling. - // Using intersection (not full-containment) so that partially-visible - // items are correctly identified as still being in view. - Rectangle adjustedItemBounds = Items[index].Bounds; - adjustedItemBounds.Y -= delta; - // we need to roll back to the index which is visible - if ((Items[index].Visible && IsItemIntersectingDisplayRectangle(displayRect, adjustedItemBounds)) + if ((Items[index].Visible && displayRect.Contains(displayRect.X, Items[index].Bounds.Top - delta)) || !Items[index].Visible) { --index; @@ -502,15 +493,10 @@ internal override void ChangeSelection(ToolStripItem? nextItem) if (index >= 0) { - // Apply the same delta-adjusted bounds used in the loop above so the - // truncation check is consistent with the visibility check. - Rectangle adjustedItemBounds = Items[index].Bounds; - adjustedItemBounds.Y -= delta; - - if (adjustedItemBounds.Bottom > displayRect.Top && adjustedItemBounds.Bottom <= displayRect.Bottom) + if (displayRect.Contains(displayRect.X, Items[index].Bounds.Bottom - delta)) { // We found an item which is truncated at the top. - delta += adjustedItemBounds.Bottom - displayRect.Top; + delta += (Items[index].Bounds.Bottom - delta) - displayRect.Top; } } } @@ -676,10 +662,8 @@ internal void RestoreScrollPosition() { Rectangle adjustedLastItemBounds = Items[Items.Count - 1].Bounds; adjustedLastItemBounds.Y -= deltaToScroll; - // Use IsItemFullyVisible (top >= display.Top && bottom <= display.Bottom) so - // that we stop accumulating scroll distance as soon as the last item would be - // completely inside the viewport — not just partially touching the edge. - if (IsItemFullyVisible(displayRectangle, adjustedLastItemBounds)) + if (displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Top) + && displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Bottom)) { // Scrolling this amount would make the last item visible, so don't scroll any more. break; @@ -705,7 +689,8 @@ internal void RestoreScrollPosition() { Rectangle adjustedLastItemBounds = Items[0].Bounds; adjustedLastItemBounds.Y -= deltaToScroll; - if (IsItemFullyVisible(displayRectangle, adjustedLastItemBounds)) + if (displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Top) + && displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Bottom)) { // Scrolling this amount would make the last item visible, so don't scroll any more. break; @@ -737,18 +722,8 @@ internal override void ScrollInternal(int delta) internal void ScrollInternal(bool up) { - // Refresh button state first so _indexOfFirstDisplayedItem is current. UpdateScrollButtonStatus(); - // If the corresponding scroll button is already disabled, there is - // nothing more to scroll. This prevents the out-of-range access that - // would otherwise occur when the last item is already at the top of the - // display area and the down-scroll path tries to read Items[index + 1]. - if ((up && !UpScrollButton.Enabled) || (!up && !DownScrollButton.Enabled)) - { - return; - } - // calling this to get ScrollWindowEx. In actuality it does nothing // to change the display rect! int delta; @@ -778,12 +753,9 @@ internal void ScrollInternal(bool up) } else { - Debug.Assert(_indexOfFirstDisplayedItem < Items.Count - 1, - "Down scroll button was enabled but _indexOfFirstDisplayedItem is at or past the last item."); - - if (_indexOfFirstDisplayedItem >= Items.Count - 1) + if (_indexOfFirstDisplayedItem == Items.Count - 1) { - return; + Debug.Fail("We're trying to scroll down, but the top item is displayed!!!"); } ToolStripItem itemTop = Items[_indexOfFirstDisplayedItem]; @@ -838,13 +810,8 @@ private void UpdateScrollButtonStatus() { Rectangle displayRectangle = DisplayRectangle; - // Track the first and last items that intersect the viewport. _indexOfFirstDisplayedItem = -1; - int indexOfLastDisplayedItem = -1; - - // Track the first and last available (non-scroll-button, non-hidden) items. - int firstAvailableItemIndex = -1; - int lastAvailableItemIndex = -1; + int minY = int.MaxValue, maxY = 0; for (int i = 0; i < Items.Count; ++i) { @@ -864,48 +831,16 @@ private void UpdateScrollButtonStatus() continue; } - firstAvailableItemIndex = firstAvailableItemIndex == -1 ? i : firstAvailableItemIndex; - lastAvailableItemIndex = i; - - // An item is "displayed" when it intersects the viewport (even partially), - // so the first such item is a reliable scroll anchor regardless of height. - if (IsItemIntersectingDisplayRectangle(displayRectangle, item.Bounds)) + if (_indexOfFirstDisplayedItem == -1 && displayRectangle.Contains(displayRectangle.X, item.Bounds.Top)) { - if (_indexOfFirstDisplayedItem == -1) - { - _indexOfFirstDisplayedItem = i; - } - - indexOfLastDisplayedItem = i; + _indexOfFirstDisplayedItem = i; } - } - if (_indexOfFirstDisplayedItem == -1 || firstAvailableItemIndex == -1) - { - // No available items are visible at all — disable both buttons. - UpScrollButton.Enabled = false; - DownScrollButton.Enabled = false; - return; + minY = Math.Min(minY, item.Bounds.Top); + maxY = Math.Max(maxY, item.Bounds.Bottom); } - // Up is enabled only when the first visible item is not already the first available item. - // Down is enabled only when there is at least one available item below the last displayed item. - // This ensures both buttons are disabled once there is no further content to scroll to. - UpScrollButton.Enabled = _indexOfFirstDisplayedItem > firstAvailableItemIndex; - DownScrollButton.Enabled = indexOfLastDisplayedItem < lastAvailableItemIndex; + UpScrollButton.Enabled = !displayRectangle.Contains(displayRectangle.X, minY); + DownScrollButton.Enabled = !displayRectangle.Contains(displayRectangle.X, maxY); } - - /// - /// Returns when is entirely - /// contained within (no clipping on either edge). - /// - private static bool IsItemFullyVisible(Rectangle displayRectangle, Rectangle itemBounds) - => itemBounds.Top >= displayRectangle.Top && itemBounds.Bottom <= displayRectangle.Bottom; - - /// - /// Returns when overlaps - /// by at least one pixel (partial visibility counts). - /// - private static bool IsItemIntersectingDisplayRectangle(Rectangle displayRectangle, Rectangle itemBounds) - => itemBounds.Bottom > displayRectangle.Top && itemBounds.Top < displayRectangle.Bottom; } diff --git a/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripScrollButton.cs b/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripScrollButton.cs index aa20cd4132d..67d8fdb7423 100644 --- a/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripScrollButton.cs +++ b/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripScrollButton.cs @@ -32,11 +32,6 @@ public ToolStripScrollButton(bool up) stickyLabel.OwnerScrollButton = this; } - // Scroll buttons are navigation affordances, not actionable menu items. - // Suppress item-click routing so ToolStripDropDown.AutoClose doesn't dismiss - // the dropdown when the user scrolls to (or at) the boundary. - SupportsItemClick = false; - _up = up; } diff --git a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs index b18eb5e449e..1a99157ad9f 100644 --- a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs +++ b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs @@ -3,8 +3,6 @@ #nullable disable -using System.Drawing; - namespace System.Windows.Forms.Tests; public class ToolStripDropDownMenuTests @@ -14,7 +12,7 @@ public void ToolStripDropDownMenu_Constructor() { using ToolStripDropDownMenu menu = new(); - menu.Should().NotBeNull(); + Assert.NotNull(menu); } [WinFormsFact] @@ -25,126 +23,8 @@ public void ToolStripDropDownMenu_ConstructorOwnerItemBool() using ToolStripDropDownMenu menu = new(owner, isAutoGenerated); - menu.Should().NotBeNull(); - menu.OwnerItem.Should().Be(owner); - menu.IsAutoGenerated.Should().BeTrue(); - } - - [WinFormsFact] - public void ToolStripDropDownMenu_ScrollInternalDown_LastItemDisplayed_DisablesDownScrollButton() - { - using ToolStripButton owner = new(); - using SubToolStripDropDownMenu menu = new(owner, isAutoGenerated: true) - { - // Use a full-item viewport to make the boundary condition deterministic: - // first item is fully above the viewport, last item is fully visible. - TestDisplayRectangle = new Rectangle(0, 20, 100, 20) - }; - - ToolStripItem firstItem = menu.Items.Add("First"); - ToolStripItem lastItem = menu.Items.Add("Last"); - - // Enabling scroll buttons triggers internal position restoration logic. - // Do this first, then set explicit bounds for deterministic visibility checks. - menu.SetRequiresScrollButtons(true); - - firstItem.SetBounds(new Rectangle(0, 0, 80, 20)); - lastItem.SetBounds(new Rectangle(0, 20, 80, 20)); - - // The real scroll-button pipeline should leave the down button disabled - // when the last item is already visible, and a subsequent click should be a no-op. - Record.Exception(menu.ScrollDown).Should().BeNull(); - menu.DownScrollButton.Enabled.Should().BeFalse(); - } - - [WinFormsFact] - public void ToolStripDropDownMenu_ScrollInternalDown_LastItemVisibleInMultiItemViewport_DisablesDownScrollButton() - { - using ToolStripButton owner = new(); - using SubToolStripDropDownMenu menu = new(owner, isAutoGenerated: true) - { - // Make room for multiple full items so the last item can be visible - // while the first displayed item is not the last index. - TestDisplayRectangle = new Rectangle(0, 20, 100, 40) - }; - - ToolStripItem firstItem = menu.Items.Add("First"); - ToolStripItem middleItem = menu.Items.Add("Middle"); - ToolStripItem lastItem = menu.Items.Add("Last"); - - // Enabling scroll buttons triggers internal position restoration logic. - // Do this first, then set explicit bounds for deterministic visibility checks. - menu.SetRequiresScrollButtons(true); - firstItem.SetBounds(new Rectangle(0, 0, 80, 20)); - middleItem.SetBounds(new Rectangle(0, 20, 80, 20)); - lastItem.SetBounds(new Rectangle(0, 40, 80, 20)); - - // At this boundary the middle and last items are visible, so scrolling down - // should already be disabled even though the first displayed item is not last. - Record.Exception(menu.ScrollDown).Should().BeNull(); - menu.DownScrollButton.Enabled.Should().BeFalse(); - } - - [WinFormsFact] - public void ToolStripDropDownMenu_ChangeSelection_ItemAtDisplayBottom_DoesNotScroll() - { - using ToolStripButton owner = new(); - using SubToolStripDropDownMenu menu = new(owner, isAutoGenerated: true) - { - TestDisplayRectangle = new Rectangle(0, 0, 100, 40) - }; - - ToolStripItem firstItem = menu.Items.Add("First"); - ToolStripItem secondItem = menu.Items.Add("Second"); - - firstItem.SetBounds(new Rectangle(0, 0, 80, 20)); - secondItem.SetBounds(new Rectangle(0, 20, 80, 20)); - - menu.InvokeChangeSelection(secondItem); - - menu.ScrollCalled.Should().BeFalse(); - } - - [WinFormsFact] - public void ToolStripDropDownMenu_ScrollButtons_DoNotSupportItemClick() - { - using ToolStripButton owner = new(); - using SubToolStripDropDownMenu menu = new(owner, isAutoGenerated: true) - { - TestDisplayRectangle = new Rectangle(0, 0, 100, 40) - }; - - _ = menu.UpScrollButton; - _ = menu.DownScrollButton; - - // Scroll buttons are navigation UI and must not route ItemClick, otherwise - // ToolStripDropDown.AutoClose can dismiss the dropdown while scrolling. - menu.UpScrollButton.SupportsItemClick.Should().BeFalse(); - menu.DownScrollButton.SupportsItemClick.Should().BeFalse(); - } - - private class SubToolStripDropDownMenu : ToolStripDropDownMenu - { - internal SubToolStripDropDownMenu(ToolStripItem ownerItem, bool isAutoGenerated) - : base(ownerItem, isAutoGenerated) - { - } - - internal Rectangle TestDisplayRectangle { get; set; } - internal bool ScrollCalled { get; private set; } - - public override Rectangle DisplayRectangle => TestDisplayRectangle; - - internal void SetRequiresScrollButtons(bool value) => base.RequiresScrollButtons = value; - - internal override void ScrollInternal(int delta) - { - ScrollCalled = true; - base.ScrollInternal(delta); - } - - internal void InvokeChangeSelection(ToolStripItem item) => base.ChangeSelection(item); - - internal void ScrollDown() => ScrollInternal(up: false); + Assert.NotNull(menu); + Assert.Equal(owner, menu.OwnerItem); + Assert.True(menu.IsAutoGenerated); } } From 42bda212bdbb996f4bc9fdffb1ed01c3390168e2 Mon Sep 17 00:00:00 2001 From: v-leafshi Date: Wed, 13 May 2026 11:04:51 +0800 Subject: [PATCH 2/2] When the last item is already displayed, clicking "Scroll Down" simply returns without performing any action. --- .../Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs b/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs index d4f967fecbb..75965fc8ac8 100644 --- a/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs +++ b/src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs @@ -755,7 +755,8 @@ internal void ScrollInternal(bool up) { if (_indexOfFirstDisplayedItem == Items.Count - 1) { - Debug.Fail("We're trying to scroll down, but the top item is displayed!!!"); + Debug.Fail("We're trying to scroll down, but the last item is displayed!!!"); + return; } ToolStripItem itemTop = Items[_indexOfFirstDisplayedItem];