NumberBox: Fix deselecting text when opening context menu

[Felix-Dev]

Description

This PR fixes an issue with the NumberBox where right-clicking to bring up the context menu would deselect the selected NumberBox text, thus preventing context menu operations such as Cut and Copy from working as expected.

Motivation and Context

Fixes #1978.

How Has This Been Tested?

Tested manually.

Gif:

Additional Note

I noticed that there is a missing function definition for

microsoft-ui-xaml/dev/NumberBox/NumberBox.h

Line 56 in 865e4fc

void OnLargeChangePropertyChanged(const winrt::DependencyPropertyChangedEventArgs& args);

in NumberBox.cpp. Looking at the overall NumberBox API I don't think we will have to do anything special when the NumberBox.LargeChange value is changed any time soon - so I think we can just remove it. Your thoughts, @teaP?

[teaP]

Agreed, the stub for OnLargeChangePropertyChanged can be removed.

Code change looks good, but could we add an automated test to reduce risk of regression in the future? If testing the context menu is difficult, a test to verify that losing focus doesn't change selection state would probably be good enough. Thanks!

[StephenLPeters]

revoking review

[StephenLPeters]

textBox.Select(static_cast<int32_t>(newText.size()), 0); [](start = 8, length = 56)

Does this mean we have no method for setting the caret's position without selecting? seems like it might be an API gap on text box.

[teaP]

That's correct. Selection and the caret are kind of the same thing, so I get it, but it is a little hidden if you're just looking to set caret position.

[StephenLPeters]

Does that mean you don't think there is an API gap on text box? I'm wondering if a feature proposal is warranted

[StephenLPeters]

Agreed, the stub for OnLargeChangePropertyChanged can be removed.

Code change looks good, but could we add an automated test to reduce risk of regression in the future? If testing the context menu is difficult, a test to verify that losing focus doesn't change selection state would probably be good enough. Thanks!

I agree a test seems warranted here.

[Felix-Dev]

@StephenLPeters @teaP Will work on a test as soon as I can (feels like I'm having to finish a bunch of PRs all at the same time, haha. I'm just joking though, there is no pressure coming from you guys 🙂 )

[StephenLPeters]

@StephenLPeters @teaP Will work on a test as soon as I can (feels like I'm having to finish a bunch of PRs all at the same time, haha. I'm just joking though, there is no pressure coming from you guys 🙂 )

We appreciate all the work! While the majority of the team works on WinUI 3 your and the rest of the community contributions have been so, so helpful. No rush, no pressure, let us know if we can assist with anything :)

[Felix-Dev]

@teaP So I wrote a test which explicitly tests that text selection remains intact when right-clicking to bring up the context menu:

[TestMethod]
public void VerifyRightClickForContextMenuDoesNotDeselectText()
{
    using (var setup = new TestSetupHelper("NumberBox Tests"))
    {
        RangeValueSpinner numBox = FindElement.ByName<RangeValueSpinner>("TestNumberBox");
        numBox.SetValue(0);

        Log.Comment("Verify that focusing the NumberBox selects the text");
        numBox.SetFocus();
        Wait.ForIdle();
        Wait.ForSeconds(3);
        Edit edit = FindTextBox(numBox);
        Verify.AreEqual("0", edit.GetTextSelection());

        Log.Comment("Verify that right-clicking on the NumberBox's input field (to bring up the context menu) does not clear the selection");
        // (15, 15): Just some offset large enough to make sure the right-click below will actually bring up the context menu
        InputHelper.RightClick(edit, 15, 15);
        Wait.ForIdle();

        Verify.AreEqual("0", edit.GetTextSelection());
    }
}

This test follows the NumberBox interaction test here to setup and check for NumberBox text selection:

microsoft-ui-xaml/dev/NumberBox/InteractionTests/NumberBoxTests.cs

Line 164 in 3834f4d

public void ValueTextTest()

The issue here is that my added test and the test I linked above - ValueTextTest() - appear as unreliable on my local machine. The unreliable part of both tests is the same - sometimes the initial text selection of the NumberBox just fails, causing the first text selection verify check to fail as well. Is it known that ValueTextTest() is an unreliable test? If not, could those tests only be unreliable on my local machine but not on the testing machines used by the team?

[StephenLPeters]

We have retry logic built into the pipeline, so if the test was unreliable but failed less then say 30% of the time our pipeline would be unlikely to fail on it. Ideally we would fix this even if the fail case isn't very common, but if we can't figure it out in a timely fashion then its probably okay.

[teaP]

The issue here is that my added test and the test I linked above - ValueTextTest() - appear as unreliable on my local machine. The unreliable part of both tests is the same - sometimes the initial text selection of the NumberBox just fails, causing the first text selection verify check to fail as well. Is it known that ValueTextTest() is an unreliable test? If not, could those tests only be unreliable on my local machine but not on the testing machines used by the team?

I'm not aware of any unreliability in those tests, Is it also unreliable manually on your machine?

[Felix-Dev]

@teaP I'm not exactly sure what you mean here with "manually". The issue appears to be that the first numBox.SetFocus(); call sometimes fails (in both my test and in ValueTextTest()) causing the following Verify.AreEqual("0", edit.GetTextSelection()); to fail as there is no text selected. I'm not sure how I'm supposed to check this manually for now as there is no input control, like a button, yet on the NumberBox test page which sets focus on the NumberBox (and I could click). Of course. I can manually focus the NumberBox by simply clicking into it's input field with the mouse pointer - this always selects the text reliably which means the tests would not fail here.

I ran my test above and ValueTextTest() 10 times each here just now in the VS Text Explorer. Here are the results:
ValueTestText(): 5 succeeds/5 failures
VerifyRightClickForContextMenuDoesNotDeselectText: 8 succeeds/2 failures

In all cases when these tests failed it was always the initial text selection of the NumberBox for me.

[StephenLPeters]

Perhaps we can improve the reliability by adding a checkbox to the test page which has its check state bound to the focus state of the number box (while the numberbox is focused the checkbox is checked) and then we can have the test call numBox.SetFocus(); WaitForChecked("FocusCheckBox"). Some of our other test pages do something like this, see the teaching tip page or the swipe page. This would allow us to drop the WaitForSeconds in both tests which could be the cause of the instability.

I sort of doubt that is going to fix the issue, it implies that if we simply increased the wait the reliability would go up which seems unlikely given that the wait is already at 3(!!!) seconds. But regardless, wait for seconds is lame, I think a change like this would be goodness.

[Felix-Dev]

@StephenLPeters I will give it a shot. Will share an update in a bit.

[teaP]

50% failure is very high; I haven't seen that, for sure. Out of curiosity, ScrollTest also sets focus, do you get intermittent failures on that one too?

[Felix-Dev]

@StephenLPeters With the recent changes, I ran my test again 10 times and this time saw it succeed 6 times and failing 4 times. So as you expected, the change to WaitForChecked("FocusCheckBox") as done in the SwipeControl tests did not fix these test issues (NumberBox focus text selection still fails from time to time).

@teaP ScrollTest() worked 10/10 so this test seems reliable with focus setting, compared to the high rate of failures on the other two tests. Oddly, I tried to replicate that test in my test and ValueTestText() to set focus yet the tests still ended up failing sometimes.

I'm thinking anyway...instead of setting the focus on the NumberBox, how about walking the VisualTree, grab the TextBox and select the text programmatically? Then I can proceed with my test bringing up the context menu and verifying that selection wasn't lost.

[teaP]

Can you update your branch with the new test? I can pull it down and see if I get any of the same unreliability on my machine.

[Felix-Dev]

@teaP Done (the new test is named VerifyRightClickForContextMenuDoesNotDeselectText).

[teaP]

I synced to your change, took out the 3 second delay, and it passed 10/10 times. As strange as it is.... seeing as you have the same issue with the other test and it's already checked in and running cleanly, I'm personally ok with this test going in as is (minus the 3 second delay) although I am certainly wondering what's different about your machine.

[Felix-Dev]

@teaP Nice to see the test appears to work just fine for you 🙂I will gladly pass on looking further into my local setup here as long as the test appears to be stable anywhere else where checked 😁

Should I also remove the 3 second delay from ValueTestText() then?

[teaP]

Lol, ok, it turns out I put that 3 second delay in there. I did not realize. Well, let's just leave it for good luck then, I guess.

[teaP]

Looks good! Thank you!

[StephenLPeters]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).