From 613cf766263bdc5159c68833c0234f25daba6d71 Mon Sep 17 00:00:00 2001 From: v-leafshi Date: Thu, 23 Apr 2026 16:26:44 +0800 Subject: [PATCH 1/2] Fix ToolStripDropDownMenu scroll boundary handling to prevent out-of-range and unintended dropdown dismissal --- .../ToolStrips/ToolStripDropDownMenu.cs | 91 +++++++++++++--- .../ToolStrips/ToolStripScrollButton.cs | 5 + .../ToolStripDropDownMenuTests.cs | 102 +++++++++++++++++- 3 files changed, 177 insertions(+), 21 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 d4f967fecbb..25176dc54ba 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,8 +464,10 @@ internal override void ChangeSelection(ToolStripItem? nextItem) if (nextItem is not null) { Rectangle displayRect = DisplayRectangle; - if (!displayRect.Contains(displayRect.X, nextItem.Bounds.Top) - || !displayRect.Contains(displayRect.X, nextItem.Bounds.Bottom)) + // 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)) { int delta; if (displayRect.Y > nextItem.Bounds.Top) @@ -479,8 +481,15 @@ 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 && displayRect.Contains(displayRect.X, Items[index].Bounds.Top - delta)) + if ((Items[index].Visible && IsItemIntersectingDisplayRectangle(displayRect, adjustedItemBounds)) || !Items[index].Visible) { --index; @@ -493,10 +502,15 @@ internal override void ChangeSelection(ToolStripItem? nextItem) if (index >= 0) { - if (displayRect.Contains(displayRect.X, Items[index].Bounds.Bottom - delta)) + // 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) { // We found an item which is truncated at the top. - delta += (Items[index].Bounds.Bottom - delta) - displayRect.Top; + delta += adjustedItemBounds.Bottom - displayRect.Top; } } } @@ -662,8 +676,10 @@ internal void RestoreScrollPosition() { Rectangle adjustedLastItemBounds = Items[Items.Count - 1].Bounds; adjustedLastItemBounds.Y -= deltaToScroll; - if (displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Top) - && displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Bottom)) + // 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)) { // Scrolling this amount would make the last item visible, so don't scroll any more. break; @@ -689,8 +705,7 @@ internal void RestoreScrollPosition() { Rectangle adjustedLastItemBounds = Items[0].Bounds; adjustedLastItemBounds.Y -= deltaToScroll; - if (displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Top) - && displayRectangle.Contains(displayRectangle.X, adjustedLastItemBounds.Bottom)) + if (IsItemFullyVisible(displayRectangle, adjustedLastItemBounds)) { // Scrolling this amount would make the last item visible, so don't scroll any more. break; @@ -722,8 +737,18 @@ 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; @@ -753,9 +778,9 @@ internal void ScrollInternal(bool up) } else { - if (_indexOfFirstDisplayedItem == Items.Count - 1) + if (_indexOfFirstDisplayedItem >= Items.Count - 1) { - Debug.Fail("We're trying to scroll down, but the top item is displayed!!!"); + return; } ToolStripItem itemTop = Items[_indexOfFirstDisplayedItem]; @@ -810,8 +835,12 @@ private void UpdateScrollButtonStatus() { Rectangle displayRectangle = DisplayRectangle; + // Track the first item that intersects the viewport. _indexOfFirstDisplayedItem = -1; - int minY = int.MaxValue, maxY = 0; + + // Track the first and last available (non-scroll-button, non-hidden) items. + int firstAvailableItemIndex = -1; + int lastAvailableItemIndex = -1; for (int i = 0; i < Items.Count; ++i) { @@ -831,16 +860,44 @@ private void UpdateScrollButtonStatus() continue; } - if (_indexOfFirstDisplayedItem == -1 && displayRectangle.Contains(displayRectangle.X, item.Bounds.Top)) + 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 (_indexOfFirstDisplayedItem == -1 + && IsItemIntersectingDisplayRectangle(displayRectangle, item.Bounds)) { _indexOfFirstDisplayedItem = i; } + } - minY = Math.Min(minY, item.Bounds.Top); - maxY = Math.Max(maxY, item.Bounds.Bottom); + if (_indexOfFirstDisplayedItem == -1 || firstAvailableItemIndex == -1) + { + // No available items are visible at all — disable both buttons. + UpScrollButton.Enabled = false; + DownScrollButton.Enabled = false; + return; } - UpScrollButton.Enabled = !displayRectangle.Contains(displayRectangle.X, minY); - DownScrollButton.Enabled = !displayRectangle.Contains(displayRectangle.X, maxY); + // Up is enabled only when the first visible item is not already the first available item. + // Down is enabled only when the first visible item is not already the last available item. + // This ensures both buttons are disabled once there is no further content to scroll to. + UpScrollButton.Enabled = _indexOfFirstDisplayedItem > firstAvailableItemIndex; + DownScrollButton.Enabled = _indexOfFirstDisplayedItem < lastAvailableItemIndex; } + + /// + /// 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 67d8fdb7423..aa20cd4132d 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,6 +32,11 @@ 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 1a99157ad9f..18da4e9b0bd 100644 --- a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs +++ b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs @@ -3,6 +3,8 @@ #nullable disable +using System.Drawing; + namespace System.Windows.Forms.Tests; public class ToolStripDropDownMenuTests @@ -12,7 +14,7 @@ public void ToolStripDropDownMenu_Constructor() { using ToolStripDropDownMenu menu = new(); - Assert.NotNull(menu); + menu.Should().NotBeNull(); } [WinFormsFact] @@ -23,8 +25,100 @@ public void ToolStripDropDownMenu_ConstructorOwnerItemBool() using ToolStripDropDownMenu menu = new(owner, isAutoGenerated); - Assert.NotNull(menu); - Assert.Equal(owner, menu.OwnerItem); - Assert.True(menu.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_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); + + internal void UpdateDisplayedItems() => base.SetDisplayedItems(); } } From ca1974c60f3cfe849412fd3fd1fbd643738afac3 Mon Sep 17 00:00:00 2001 From: v-leafshi Date: Fri, 24 Apr 2026 16:29:00 +0800 Subject: [PATCH 2/2] Fix ToolStripDropDownMenu scroll boundary handling --- .../ToolStrips/ToolStripDropDownMenu.cs | 17 +++++++---- .../ToolStripDropDownMenuTests.cs | 30 +++++++++++++++++-- 2 files changed, 39 insertions(+), 8 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 25176dc54ba..f50aad7ee04 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 @@ -835,8 +835,9 @@ private void UpdateScrollButtonStatus() { Rectangle displayRectangle = DisplayRectangle; - // Track the first item that intersects the viewport. + // 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; @@ -865,10 +866,14 @@ private void UpdateScrollButtonStatus() // 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 (_indexOfFirstDisplayedItem == -1 - && IsItemIntersectingDisplayRectangle(displayRectangle, item.Bounds)) + if (IsItemIntersectingDisplayRectangle(displayRectangle, item.Bounds)) { - _indexOfFirstDisplayedItem = i; + if (_indexOfFirstDisplayedItem == -1) + { + _indexOfFirstDisplayedItem = i; + } + + indexOfLastDisplayedItem = i; } } @@ -881,10 +886,10 @@ private void UpdateScrollButtonStatus() } // Up is enabled only when the first visible item is not already the first available item. - // Down is enabled only when the first visible item is not already the last 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 = _indexOfFirstDisplayedItem < lastAvailableItemIndex; + DownScrollButton.Enabled = indexOfLastDisplayedItem < lastAvailableItemIndex; } /// diff --git a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs index 18da4e9b0bd..b18eb5e449e 100644 --- a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs +++ b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs @@ -57,6 +57,34 @@ public void ToolStripDropDownMenu_ScrollInternalDown_LastItemDisplayed_DisablesD 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() { @@ -118,7 +146,5 @@ internal override void ScrollInternal(int delta) internal void InvokeChangeSelection(ToolStripItem item) => base.ChangeSelection(item); internal void ScrollDown() => ScrollInternal(up: false); - - internal void UpdateDisplayedItems() => base.SetDisplayedItems(); } }