diff --git a/src/System.Windows.Forms/PublicAPI.Unshipped.txt b/src/System.Windows.Forms/PublicAPI.Unshipped.txt index 3348c917404..dc6e8fc8148 100644 --- a/src/System.Windows.Forms/PublicAPI.Unshipped.txt +++ b/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! 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! 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..9c6dc6dd360 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 @@ -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,14 +779,15 @@ 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; @@ -778,16 +795,17 @@ 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."); + 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; } @@ -838,34 +856,21 @@ 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 +880,6 @@ private void UpdateScrollButtonStatus() { _indexOfFirstDisplayedItem = i; } - - indexOfLastDisplayedItem = i; } } @@ -888,13 +891,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; + /// /// Returns when is entirely /// contained within (no clipping on either edge). @@ -908,4 +940,114 @@ private static bool IsItemFullyVisible(Rectangle displayRectangle, Rectangle ite /// private static bool IsItemIntersectingDisplayRectangle(Rectangle displayRectangle, Rectangle itemBounds) => itemBounds.Bottom > displayRectangle.Top && itemBounds.Top < displayRectangle.Bottom; + + /// + /// Overrides Up/Down arrow handling to use the logical order + /// rather than the geometry-based search over . + /// This prevents invisible items from causing the navigation to skip visible siblings. + /// + 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); + } + + /// + /// Walks the logical collection (not the transient + /// snapshot) to find the nearest keyboard-selectable + /// item in the requested direction. Wraps around when the boundary is reached. + /// is set to when the result came + /// from the wrap-around pass. + /// + 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; + } } diff --git a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs index b18eb5e449e..6369e66c153 100644 --- a/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs +++ b/src/test/unit/System.Windows.Forms/ToolStripDropDownMenuTests.cs @@ -4,6 +4,7 @@ #nullable disable using System.Drawing; +using System.Reflection; namespace System.Windows.Forms.Tests; @@ -58,31 +59,40 @@ public void ToolStripDropDownMenu_ScrollInternalDown_LastItemDisplayed_DisablesD } [WinFormsFact] - public void ToolStripDropDownMenu_ScrollInternalDown_LastItemVisibleInMultiItemViewport_DisablesDownScrollButton() + public void ToolStripDropDownMenu_ScrollInternalDown_LastItemVisibleInMultiItemViewport_EnablesDownScrollButton() { 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) + // Viewport starts at Y=0, so the anchor is always the first item. + TestDisplayRectangle = new Rectangle(0, 0, 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. + // SetRequiresScrollButtons triggers internal anchor logic; set bounds after for deterministic layout. 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)); + firstItem.SetBounds(new Rectangle(0, 0, 80, 18)); + middleItem.SetBounds(new Rectangle(0, 24, 80, 18)); + lastItem.SetBounds(new Rectangle(0, 46, 80, 18)); + + // After setting bounds, recompute scroll button status. + menu.RefreshScrollButtonStatus(); + menu.UpScrollButton.Enabled.Should().BeFalse("Up should be disabled when the first item is the anchor"); + menu.DownScrollButton.Enabled.Should().BeTrue("Down should be enabled when more items are below the anchor"); - // 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. + // First scroll: anchor moves to middle item, last is still not fully visible, so down remains enabled. Record.Exception(menu.ScrollDown).Should().BeNull(); - menu.DownScrollButton.Enabled.Should().BeFalse(); + menu.RefreshScrollButtonStatus(); + menu.UpScrollButton.Enabled.Should().BeTrue("Up should be enabled after scrolling down from the first item"); + menu.DownScrollButton.Enabled.Should().BeTrue("Down should still be enabled with last item below the anchor"); + + // Second scroll: anchor moves to last item, all items are now visible, so down should become disabled. + Record.Exception(menu.ScrollDown).Should().BeNull(); + menu.RefreshScrollButtonStatus(); + menu.DownScrollButton.Enabled.Should().BeFalse("Down should be disabled when the last item is fully visible"); } [WinFormsFact] @@ -123,6 +133,45 @@ public void ToolStripDropDownMenu_ScrollButtons_DoNotSupportItemClick() menu.DownScrollButton.SupportsItemClick.Should().BeFalse(); } + [WinFormsFact] + public void ToolStripDropDownMenu_ScrollButtonsStatus_WithUnavailableInterleavedItems_UsesAvailableItemsOnly() + { + using ToolStripButton owner = new(); + using SubToolStripDropDownMenu menu = new(owner, isAutoGenerated: true) + { + // Single-item viewport: only one item visible at a time. + TestDisplayRectangle = new Rectangle(0, 0, 100, 20) + }; + + ToolStripItem firstItem = menu.Items.Add("First"); + ToolStripItem hiddenMiddleItem = menu.Items.Add("HiddenMiddle"); + ToolStripItem lastItem = menu.Items.Add("Last"); + + menu.SetRequiresScrollButtons(true); + + firstItem.SetBounds(new Rectangle(0, 0, 80, 20)); + hiddenMiddleItem.SetBounds(new Rectangle(0, 20, 80, 20)); + lastItem.SetBounds(new Rectangle(0, 40, 80, 20)); + hiddenMiddleItem.Available = false; + + // Initial state: firstItem is visible, up is disabled, down is enabled (can scroll to lastItem) + menu.RefreshScrollButtonStatus(); + menu.UpScrollButton.Enabled.Should().BeFalse("Up should be disabled when first available item is visible"); + menu.DownScrollButton.Enabled.Should().BeTrue("Down should be enabled when more available items are below"); + + // Scroll down: lastItem becomes visible, up is enabled, down is disabled (no more items below) + Record.Exception(menu.ScrollDown).Should().BeNull(); + menu.RefreshScrollButtonStatus(); + menu.UpScrollButton.Enabled.Should().BeTrue("Up should be enabled after scrolling down to last available item"); + menu.DownScrollButton.Enabled.Should().BeFalse("Down should be disabled when last available item is visible"); + + // Scroll up: firstItem becomes visible again, up is disabled, down is enabled + Record.Exception(() => menu.ScrollInternal(up: true)).Should().BeNull(); + menu.RefreshScrollButtonStatus(); + menu.UpScrollButton.Enabled.Should().BeFalse("Up should be disabled after scrolling back to first available item"); + menu.DownScrollButton.Enabled.Should().BeTrue("Down should be enabled after scrolling up from last available item"); + } + private class SubToolStripDropDownMenu : ToolStripDropDownMenu { internal SubToolStripDropDownMenu(ToolStripItem ownerItem, bool isAutoGenerated) @@ -143,6 +192,15 @@ internal override void ScrollInternal(int delta) base.ScrollInternal(delta); } + internal void RefreshScrollButtonStatus() + { + MethodInfo updateScrollButtonStatus = typeof(ToolStripDropDownMenu) + .GetMethod("UpdateScrollButtonStatus", BindingFlags.Instance | BindingFlags.NonPublic); + + updateScrollButtonStatus.Should().NotBeNull(); + updateScrollButtonStatus.Invoke(this, null); + } + internal void InvokeChangeSelection(ToolStripItem item) => base.ChangeSelection(item); internal void ScrollDown() => ScrollInternal(up: false);