Fix ToolStripDropDownMenu scroll boundary handling to prevent out-of-range and unintended dropdown dismissal

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,13 @@ private void UpdateScrollButtonStatus()
     {
         Rectangle displayRectangle = DisplayRectangle;
 
+        // Track the first and last items that intersect the viewport.
         _indexOfFirstDisplayedItem = -1;
-        int minY = int.MaxValue, maxY = 0;
+        int indexOfLastDisplayedItem = -1;
+
+        // 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 +861,48 @@ 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 (IsItemIntersectingDisplayRectangle(displayRectangle, item.Bounds))
             {
-                _indexOfFirstDisplayedItem = i;
+                if (_indexOfFirstDisplayedItem == -1)
+                {
+                    _indexOfFirstDisplayedItem = i;
+                }
+
+                indexOfLastDisplayedItem = 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 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;
     }
+
+    /// <summary>
+    ///  Returns <see langword="true"/> when <paramref name="itemBounds"/> is entirely
+    ///  contained within <paramref name="displayRectangle"/> (no clipping on either edge).
+    /// </summary>
+    private static bool IsItemFullyVisible(Rectangle displayRectangle, Rectangle itemBounds)
+        => itemBounds.Top >= displayRectangle.Top && itemBounds.Bottom <= displayRectangle.Bottom;
+
+    /// <summary>
+    ///  Returns <see langword="true"/> when <paramref name="itemBounds"/> overlaps
+    ///  <paramref name="displayRectangle"/> by at least one pixel (partial visibility counts).
+    /// </summary>
+    private static bool IsItemIntersectingDisplayRectangle(Rectangle displayRectangle, Rectangle itemBounds)
+        => itemBounds.Bottom > displayRectangle.Top && itemBounds.Top < displayRectangle.Bottom;
 }

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;
     }
 

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,126 @@ 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_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);
     }
 }