Fix ComboBox text not clearing on last Item removal

[jbhensley]

Fixes #6621

Proposed changes

ComboBox.Text does not clear when the last item is removed. This was tested on Win32 and confirmed to be native behavior. PR addresses by checking for last item removal and updating Text accordingly.

It should be noted that this likely goes back to the beginning of .NET, so this is a change to longstanding behavior. It does, however, seem to be correct when measured by the principle of least astonishment.

Separate issue: existing behavior does not raise TextChanged or SelectedIndexChanged when SelectedItem is removed. Consideration should be given as to whether this should be changed.

Modified to raise events. Unit tests ensure that events are raised in the correct order and only raised once.

There is an edge which is also covered. TextChanged does not fire if changing between items with the same text. This includes adding an item with string.Emtpy as its value, selecting it, and then setting SelectedIndex to -1. To match that behavior, we ensure that TextChanged does not fire when SelectedItem is removed if SelectedItem is string.Emtpy.

Customer Impact

Text will clear on last item removal, consistent with what it does when a non-last item is removed.

Regression?

• No

Risk

• It is likely that apps have been designed to compensate for this already by clearing Text on last item removal in an event handler. That should not create a conflict
• It is possible that apps exist which were designed to depend on this behavior, in which case it could be a breaking change. Admittedly, this would be a very odd dependency

Test methodology

• Unit tests added

Microsoft Reviewers: Open in CodeFlow

[dreddy-work]

Thank you @jbhensley for sending this out. We will have to check if any existing scenarios will be broken here. Especially when we are not raising events that we would normally raise when a text changes. Will update the PR after we discussed this within the team.

Also, this block should go inside IF on line 382?

[jbhensley]

Agreed regarding events. Really feels like we should break with existing behavior and raise when items are removed as that would be technically correct. As is, we're just perpetuating the error. Not sure the implications for existing applications.

[SergeySmirnov-Akvelon]

@jbhensley, looks good. Great job, thank you.

@dreddy-work, should it be sent for testing to the CTI team?

[dreddy-work]

We reviewed this yesterday. It seems we should look at the associated events as well here. This is going to be a breaking change but will be documented for 7.0. @jbhensley , will you be able to look into events as well to make this new behavior complete?

[jbhensley]

We reviewed this yesterday. It seems we should look at the associated events as well here. This is going to be a breaking change but will be documented for 7.0. @jbhensley , will you be able to look into events as well to make this new behavior complete?

Yes, I'll take a look.

[dreddy-work]

Thanks. will get this reviewed soon.

[jbhensley]

I realized that I missed raising events on Items.Clear. That has been fixed now.

Here's an interesting edge case. Add items to the ComboBox and select one:

comboBox1.Items.Add("Item 1");
comboBox1.Items.Add("Item 2");
comboBox1.Items.Add("Item 3");

comboBox1.SelectedIndex = 1;

Now insert at the selected index: comboBox1.Items.Insert(1, "Test");

If the handle has been created then SelectedItem will not change. If the handle has not been created then it will. It will be set to the newly inserted item. It's actually slightly worse than that because, while SelectedItem will be the newly inserted item, Text will not change until handle creation. Here's what it looks like right after insert:

I feel like a separate issue should be added for this internally inconsistent behavior.

[jbhensley]

Sorry for all of the comments, but I think we should determine how to handle events when an item is inserted at index < SelectedIndex also. In that scenario, SelectedIndex will increment while SelectedItem and Text remain the same. Should SelectedIndexChanged be raised?

[RussKie]

Thank you for doing all these investigations, and blowing the dust off nooks and crannies. It's great to give some care to the code that has been written some 20 odd years ago, but there is a real risk of accidentally breaking consumers. who may have come to depend on existing behaviours. Please understand the team's hesitation with these kinds of changes.
We can certainly document certain existing idiosyncrasies (you can do it too from https://docs.microsoft.com/ by clicking "Edit" button in the top right hand side corner). We can also accept certain changes that may be considered breaking but have tangible benefits (e.g., a increased performance) or that aimed at addressing issues with real life scenarios.

In this case, if we can settle on the least invasive changes to achieve the goal - the better.

Hope this makes sense.

[jbhensley]

Understood. Just trying to be thorough in the analysis.

My feeling is that raising events when SelectedItem is removed makes sense because the selection changes. However, raising events when SelectedIndex changes due to an insert doesn't quite fit with what most people think the event is supposed to signify, which is a change of selection.

[Tanya-Solyanik]

@dreddy-work, @RussKie - please remind me what our decision was, are we clearing text but are not sending any events? Or "won't fix" all aspects of this issue?

FYI - @SergeySmirnov-Akvelon

[RussKie]

I don't have a firm position - on one hand this change fixes the inconsistency, one the other hand this may result in unexpected behaviour...
As an alternative to discarding this change - I'll be content with putting the change under a quirk (enabled by default), that can be opted-out if it breaks existing consumer.

[Tanya-Solyanik]

Quirks are hard to maintain, and this fix had not been requested by customers (at least recently), I would wait until we get a customer request.

[merriemcgaw]

I have to agree with @Tanya-Solyanik on this one - if we get a customer request we will have this fix in our back pocket. But since it will result in unexpected behavior, I am reluctant to take this without having the customer request. I also want to avoid getting in the situation where we're trying to manage dozens and dozens of quirks from proactive fixes.

[RussKie]

Hi @jbhensley, the team had a number of offline conversations discussing pros and cons, and we've concluded that the risk of an accidental regression outweighs the perceived benefits. Coupled with the fact that we don't have a real customer behind the original issue (it was raised by our team member), and there is a relatively trivial workaround, we decided not to take this change. We can update the docs, and if you're willing you can propose an update here.

Apologies for taking time, and thank you for your work.