Skip to content

Fix Up and Down arrow buttons not disabling at first and last items in Collection Editor #14668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rabina4363sf wants to merge 4 commits into
base: main
Choose a base branch
from
+45 −2
Open

Fix Up and Down arrow buttons not disabling at first and last items in Collection Editor #14668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6f5d7b
Fix_Issue_3152: Disable Up and Down buttons correctly in CollectionEd…
Rabina4363sf May 26, 2026
267e52d
Fix_Issue_3152 - Added testcases for the CollectionEditor Form's Up a…
Rabina4363sf Jun 23, 2026
aac2f7f
Merge branch 'dotnet:main' into Fix_Issue_3152
Rabina4363sf Jun 23, 2026
257f147
Fix_Issue_3152 - Committed the Concerned changes
Rabina4363sf Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...esign/src/System/ComponentModel/Design/CollectionEditor.CollectionEditorCollectionForm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,9 +1037,11 @@ private void UpdateEnabled()
}

bool editEnabled = (_listBox.SelectedItem is not null) && CollectionEditable;
int count = _listBox.Items.Count;
int selectedIndex = _listBox.SelectedIndex;
_removeButton.Enabled = editEnabled && AllowRemoveInstance(((ListItem)_listBox.SelectedItem!).Value);
_upButton.Enabled = editEnabled && _listBox.Items.Count > 1;
_downButton.Enabled = editEnabled && _listBox.Items.Count > 1;
_upButton.Enabled = editEnabled && count > 1 && selectedIndex > 0;
_downButton.Enabled = editEnabled && count > 1 && selectedIndex < count - 1;
_propertyGrid.Enabled = editEnabled;
_addButton.Enabled = CollectionEditable;

Expand Down
41 changes: 41 additions & 0 deletions ...indows.Forms.Design/tests/UnitTests/System/ComponentModel/Design/CollectionEditorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,47 @@ namespace System.ComponentModel.Design.Tests;

public class CollectionEditorTests
{
[Fact]
public void CollectionEditor_CollectionForm_UpdateEnabled_FirstMiddleLastNone_SetsUpDownExpected()
{
SubCollectionEditor editor = new(typeof(List<int>));
using Form form = editor.CreateCollectionForm();

dynamic a = form.TestAccessor.Dynamic;
a.AddItems(new object[] { 1, 2, 3 });

@LeafShi1 LeafShi1 Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The test implicitly assumes AddItems selects the last added item (index 2). If AddItems behavior changes in the future, this test will fail without a clear reason.

Consider adding an explicit assertion on the initial state:

a.AddItems(new object[] { 1, 2, 3 });

// AddItems selects the last added item by default
listBox.SelectedIndex.Should().Be(2);
up.Enabled.Should().BeTrue();
down.Enabled.Should().BeFalse();

@Rabina4363sf Rabina4363sf Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve added an explicit assertion for the initial state after AddItems so the behavior is clearly verified. Updated in the latest commit.


ListBox listBox = (ListBox)a._listBox;
Button up = (Button)a._upButton;
Button down = (Button)a._downButton;

listBox.SelectedIndex = 2;
a.UpdateEnabled();
up.Enabled.Should().BeTrue();
down.Enabled.Should().BeFalse();

listBox.ClearSelected();
listBox.SelectedIndex = 0;
a.UpdateEnabled();
up.Enabled.Should().BeFalse();
down.Enabled.Should().BeTrue();

@LeafShi1 LeafShi1 Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This test relies on the SelectedIndexChanged event being wired up to call UpdateEnabled() automatically. In a test environment (where the form is never shown), the event wiring may not be fully initialized, making this test fragile.

Consider calling UpdateEnabled() explicitly after each selection change to make the test deterministic:

listBox.SelectedIndex = 0;
a.UpdateEnabled();  // Don't rely on event — call explicitly
up.Enabled.Should().BeFalse();

This also makes the test more readable — it's clear we're testing the logic of UpdateEnabled(), not the event wiring.

@Rabina4363sf Rabina4363sf Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I’ve updated the test to call UpdateEnabled() explicitly after selection changes to avoid relying on event wiring. Included in the latest commit.

listBox.ClearSelected();
listBox.SelectedIndex = 1;
a.UpdateEnabled();
up.Enabled.Should().BeTrue();
down.Enabled.Should().BeTrue();

listBox.ClearSelected();
up.Enabled.Should().BeFalse();
down.Enabled.Should().BeFalse();

listBox.Items.Clear();

@LeafShi1 LeafShi1 Jun 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: After listBox.Items.Clear(), SelectedIndex becomes -1. If UpdateEnabled() is not triggered by the clear operation, the button states here could be stale values from the previous assertions rather than reflecting the current state.

Same recommendation: call a.UpdateEnabled() explicitly after AddItems to ensure we're verifying the actual logic, not leftover state.

@Rabina4363sf Rabina4363sf Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with this. I’ve added an explicit UpdateEnabled() call after the state changes to ensure we’re validating the correct state. Fixed in the latest commit.

a.AddItems(new object[] { 42 });
a.UpdateEnabled();
up.Enabled.Should().BeFalse();
down.Enabled.Should().BeFalse();
}

[Theory]
[InlineData(typeof(object), typeof(object))]
[InlineData(typeof(string), typeof(object))]
Expand Down