Fix ToolStripDropDownMenu scroll boundary issues

src/System.Windows.Forms/PublicAPI.Unshipped.txt

@@ -1,3 +1,4 @@
+override System.Windows.Forms.ToolStripDropDownMenu.ProcessDialogKey(System.Windows.Forms.Keys keyData) -> bool
 static System.Windows.Forms.Application.SetColorMode(System.Windows.Forms.SystemColorMode systemColorMode) -> void
 static System.Windows.Forms.TaskDialog.ShowDialogAsync(nint hwndOwner, System.Windows.Forms.TaskDialogPage! page, System.Windows.Forms.TaskDialogStartupLocation startupLocation = System.Windows.Forms.TaskDialogStartupLocation.CenterOwner) -> System.Threading.Tasks.Task<System.Windows.Forms.TaskDialogButton!>!
 static System.Windows.Forms.TaskDialog.ShowDialogAsync(System.Windows.Forms.IWin32Window! owner, System.Windows.Forms.TaskDialogPage! page, System.Windows.Forms.TaskDialogStartupLocation startupLocation = System.Windows.Forms.TaskDialogStartupLocation.CenterOwner) -> System.Threading.Tasks.Task<System.Windows.Forms.TaskDialogButton!>!

src/System.Windows.Forms/System/Windows/Forms/Controls/ToolStrips/ToolStripDropDownMenu.cs

@@ -24,6 +24,7 @@ public partial class ToolStripDropDownMenu : ToolStripDropDown
     private ToolStripScrollButton? _downScrollButton;
     private int _scrollAmount;
     private int _indexOfFirstDisplayedItem = -1;
+    private bool _alignSelectionToTopOnNextChange;
 
     private BitVector32 _state;
 
@@ -464,10 +465,21 @@ internal override void ChangeSelection(ToolStripItem? nextItem)
         if (nextItem is not null)
         {
             Rectangle displayRect = DisplayRectangle;
+
+            // If _alignSelectionToTopOnNextChange is set (e.g., after Down arrow),
+            // and the item is below the current viewport top, scroll it to the top.
+            if (_alignSelectionToTopOnNextChange && displayRect.Y < nextItem.Bounds.Top)
+            {
+                int delta = nextItem.Bounds.Top - displayRect.Y;
+                ScrollInternal(delta);
+                UpdateScrollButtonStatus();
+                _alignSelectionToTopOnNextChange = false;
+            }
+
             // 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))
+            else if (!IsItemFullyVisible(displayRect, nextItem.Bounds))
             {
                 int delta;
                 if (displayRect.Y > nextItem.Bounds.Top)
@@ -518,6 +530,10 @@ internal override void ChangeSelection(ToolStripItem? nextItem)
                 ScrollInternal(delta);
                 UpdateScrollButtonStatus();
             }
+            else
+            {
+                _alignSelectionToTopOnNextChange = false;
+            }
         }
 
         base.ChangeSelection(nextItem);
@@ -763,31 +779,33 @@ internal void ScrollInternal(bool up)
         {
             if (up)
             {
-                if (_indexOfFirstDisplayedItem == 0)
+                int previousScrollableItemIndex = FindPreviousScrollableItemIndex(_indexOfFirstDisplayedItem);
+                if (previousScrollableItemIndex < 0)
                 {
-                    Debug.Fail("We're trying to scroll up, but the top item is displayed!!!");
+                    Debug.Fail("We're trying to scroll up, but there is no previous scrollable item.");
                     delta = 0;
                 }
                 else
                 {
-                    ToolStripItem itemTop = Items[_indexOfFirstDisplayedItem - 1];
+                    ToolStripItem itemTop = Items[previousScrollableItemIndex];
                     ToolStripItem itemBottom = Items[_indexOfFirstDisplayedItem];
                     // We use a delta between the tops, since it takes margin's and padding into account.
                     delta = itemTop.Bounds.Top - itemBottom.Bounds.Top;
                 }
             }
             else
             {
-                Debug.Assert(_indexOfFirstDisplayedItem < Items.Count - 1,
-                    "Down scroll button was enabled but _indexOfFirstDisplayedItem is at or past the last item.");
+                int nextScrollableItemIndex = FindNextScrollableItemIndex(_indexOfFirstDisplayedItem);
+                Debug.Assert(nextScrollableItemIndex >= 0,
+                    "Down scroll button was enabled but there is no next scrollable item.");
 
-                if (_indexOfFirstDisplayedItem >= Items.Count - 1)
+                if (nextScrollableItemIndex < 0)
                 {
                     return;
                 }
 
                 ToolStripItem itemTop = Items[_indexOfFirstDisplayedItem];
-                ToolStripItem itemBottom = Items[_indexOfFirstDisplayedItem + 1];
+                ToolStripItem itemBottom = Items[nextScrollableItemIndex];
                 // We use a delta between the tops, since it takes margin's and padding into account.
                 delta = itemBottom.Bounds.Top - itemTop.Bounds.Top;
             }
@@ -805,6 +823,7 @@ protected override void SetDisplayedItems()
             DisplayedItems.Insert(0, UpScrollButton);
             DisplayedItems.Add(DownScrollButton);
             UpdateScrollButtonLocations();
+            UpdateScrollButtonDisplayOrder();
             UpScrollButton.Visible = true;
             DownScrollButton.Visible = true;
         }
@@ -834,38 +853,49 @@ private void UpdateScrollButtonLocations()
         }
     }
 
+    private void UpdateScrollButtonDisplayOrder()
+    {
+        // Hit testing walks DisplayedItems from front to back. Keep the scroll buttons
+        // ahead of any partially visible menu items that overlap their bounds so the
+        // buttons win mouse hit testing near the viewport edges.
+        DisplayedItems.Remove(UpScrollButton);
+        DisplayedItems.Remove(DownScrollButton);
+
+        DisplayedItems.Insert(0, UpScrollButton);
+
+        int downInsertIndex = DisplayedItems.Count;
+        for (int i = 0; i < DisplayedItems.Count; i++)
+        {
+            ToolStripItem item = DisplayedItems[i];
+            if (item != UpScrollButton && item.Bounds.IntersectsWith(DownScrollButton.Bounds))
+            {
+                downInsertIndex = i;
+                break;
+            }
+        }
+
+        DisplayedItems.Insert(downInsertIndex, DownScrollButton);
+    }
+
     private void UpdateScrollButtonStatus()
     {
         Rectangle displayRectangle = DisplayRectangle;
 
-        // Track the first and last items that intersect the viewport.
+        // Track the first item that intersects 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;
 
         for (int i = 0; i < Items.Count; ++i)
         {
             ToolStripItem item = Items[i];
-            if (UpScrollButton == item)
-            {
-                continue;
-            }
-
-            if (DownScrollButton == item)
-            {
-                continue;
-            }
-
-            if (!item.Available)
+            if (!IsScrollableItem(item))
             {
                 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.
@@ -875,8 +905,6 @@ private void UpdateScrollButtonStatus()
                 {
                     _indexOfFirstDisplayedItem = i;
                 }
-
-                indexOfLastDisplayedItem = i;
             }
         }
 
@@ -888,13 +916,42 @@ private void UpdateScrollButtonStatus()
             return;
         }
 
-        // 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;
+        // ScrollInternal(bool up) scrolls by moving the first displayed item to its previous/next item.
+        // Therefore, enable buttons only when such a move is possible. This keeps boundary behavior
+        // correct even when the viewport is too small to fully show a whole item.
+        UpScrollButton.Enabled = FindPreviousScrollableItemIndex(_indexOfFirstDisplayedItem) >= 0;
+        DownScrollButton.Enabled = FindNextScrollableItemIndex(_indexOfFirstDisplayedItem) >= 0;
     }
 
+    private int FindPreviousScrollableItemIndex(int startIndex)
+    {
+        for (int i = startIndex - 1; i >= 0; i--)
+        {
+            if (IsScrollableItem(Items[i]))
+            {
+                return i;
+            }
+        }
+
+        return -1;
+    }
+
+    private int FindNextScrollableItemIndex(int startIndex)
+    {
+        for (int i = startIndex + 1; i < Items.Count; i++)
+        {
+            if (IsScrollableItem(Items[i]))
+            {
+                return i;
+            }
+        }
+
+        return -1;
+    }
+
+    private bool IsScrollableItem(ToolStripItem item)
+        => item != UpScrollButton && item != DownScrollButton && item.Available;
+
     /// <summary>
     ///  Returns <see langword="true"/> when <paramref name="itemBounds"/> is entirely
     ///  contained within <paramref name="displayRectangle"/> (no clipping on either edge).
@@ -908,4 +965,114 @@ private static bool IsItemFullyVisible(Rectangle displayRectangle, Rectangle ite
     /// </summary>
     private static bool IsItemIntersectingDisplayRectangle(Rectangle displayRectangle, Rectangle itemBounds)
         => itemBounds.Bottom > displayRectangle.Top && itemBounds.Top < displayRectangle.Bottom;
+
+    /// <summary>
+    ///  Overrides Up/Down arrow handling to use the logical <see cref="ToolStrip.Items"/> order
+    ///  rather than the geometry-based search over <see cref="ToolStrip.DisplayedItems"/>.
+    ///  This prevents invisible items from causing the navigation to skip visible siblings.
+    /// </summary>
+    internal override bool ProcessArrowKey(Keys keyCode)
+    {
+        if (keyCode is not Keys.Up and not Keys.Down)
+        {
+            return base.ProcessArrowKey(keyCode);
+        }
+
+        bool down = keyCode == Keys.Down;
+
+        if (RequiresScrollButtons)
+        {
+            UpdateScrollButtonStatus();
+        }
+
+        ToolStripItem? current = GetSelectedItem();
+        ToolStripItem? next = GetAdjacentKeyboardSelectableItem(current, down, out _);
+        if (next is not null)
+        {
+            // When scroll buttons are visible, align the focused item to the top of the viewport
+            // so focus progresses visually through the list in both directions (Up/Down).
+            // When all items fit (no scroll buttons), only update focus without repositioning.
+            if (RequiresScrollButtons)
+            {
+                _alignSelectionToTopOnNextChange = true;
+            }
+
+            ChangeSelection(next);
+        }
+
+        // Always consume the key.
+        return true;
+    }
+
+    protected override bool ProcessDialogKey(Keys keyData)
+    {
+        // Map Tab to Down and Shift+Tab to Up when scroll buttons are visible,
+        // with the same alignment behavior as arrow keys.
+        if (RequiresScrollButtons && (keyData == Keys.Tab || keyData == (Keys.Tab | Keys.Shift)))
+        {
+            bool down = keyData == Keys.Tab;
+            UpdateScrollButtonStatus();
+
+            ToolStripItem? current = GetSelectedItem();
+            ToolStripItem? next = GetAdjacentKeyboardSelectableItem(current, down, out _);
+            if (next is not null)
+            {
+                _alignSelectionToTopOnNextChange = true;
+                ChangeSelection(next);
+                return true;
+            }
+        }
+
+        return base.ProcessDialogKey(keyData);
+    }
+
+    /// <summary>
+    ///  Walks the logical <see cref="ToolStrip.Items"/> collection (not the transient
+    ///  <see cref="ToolStrip.DisplayedItems"/> snapshot) to find the nearest keyboard-selectable
+    ///  item in the requested direction. Wraps around when the boundary is reached.
+    ///  <paramref name="wrapped"/> is set to <see langword="true"/> when the result came
+    ///  from the wrap-around pass.
+    /// </summary>
+    private ToolStripItem? GetAdjacentKeyboardSelectableItem(ToolStripItem? start, bool forward, out bool wrapped)
+    {
+        wrapped = false;
+
+        if (Items.Count == 0)
+        {
+            return null;
+        }
+
+        int startIndex = start is null ? -1 : Items.IndexOf(start);
+        int delta = forward ? 1 : -1;
+
+        // First pass: search from current position toward the boundary.
+        for (int i = startIndex + delta; i >= 0 && i < Items.Count; i += delta)
+        {
+            ToolStripItem item = Items[i];
+            if (item.CanKeyboardSelect && item.Available)
+            {
+                return item;
+            }
+        }
+
+        // Boundary reached — wrap around from the opposite end.
+        wrapped = true;
+        int wrapStart = forward ? 0 : Items.Count - 1;
+        for (int i = wrapStart; i >= 0 && i < Items.Count; i += delta)
+        {
+            // Stop before we reach the original item again.
+            if (i == startIndex)
+            {
+                break;
+            }
+
+            ToolStripItem item = Items[i];
+            if (item.CanKeyboardSelect && item.Available)
+            {
+                return item;
+            }
+        }
+
+        return null;
+    }
 }