fixes for case adding data at the end of the textbox

[rizamarhaban]

Fixes #3335

When using the TextBoxMask extension, the TextBox text not updated if the text with the same string length changes, unless if the string length is less than the current Text length.

PR Type

What kind of change does this PR introduce?

• Bugfix

What is the current behavior?

Example, a TextBox with Mask 99:59:59 where cutom mask 5:[0-5], if the current Text is 10:20:30 and we update with 20:30:40, the Text did not changed/updated. The UI will show the same '10:20:30' because of this code: textbox.Text = oldText;

What is the new behavior?

Example, if the current Text is 10:20:30 and we update with 20:30:40, the Text changed/updated. The fix is to let the text updated on this regards and no other behavior changes.

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

Other information

[ghost]

Thanks rizamarhaban for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[michael-hawker]

@rizamarhaban I dug into this a bit further. I think the problem here is that the original code assumed the Text was being manipulated via keyboard/user interaction with the TextBox and not via Binding.

You can see that in the Sample App with the XAML only repro I provided with this code:

                <TextBox Grid.Row="3"
                x:Name="TestText"
                         Text="10:20:30"
                         Height="56"
                         Header="Source" />

                <TextBox Grid.Row="4"
                         extensions:TextBoxMask.CustomMask="5:[0-5]"
                         extensions:TextBoxMask.Mask="99:59:59"
                         Header="Target"
                         Text="{Binding Text, ElementName=TestText, Mode=TwoWay}"
                         Style="{StaticResource MaskedTextBoxStyle}" />

If you paste in the Source box new text like 20:30:40 then your fix happens to resolve that issue. However, if you clear the source textbox, then the Target is left with 10:20:3_ instead.

Would you be up for re-evaluating the logic at the top of this function? I don't know if there's a way we can detect the binding update instead vs. manipulation. Though ideally I think we find logic that covers all those cases instead?

Thoughts?

[rizamarhaban]

@michael-hawker I see. In my usage, I need to update the textbox by feeding with data via binding. Not from UI interaction. That did not worked with the current version. Let me try to see with more scenarios to test and fix the code logic. If we need to add Tests, do I need to create another Issue, or I just manually tests with different scenarios?

[rizamarhaban]

@michael-hawker I see what you mean. Let me try fixing this and do more testing scenario.

[rizamarhaban]

@michael-hawker I may found the issue on the logic here. Trying to find another way to capture the binding behavior when the textbox value cleared. On the mask textbox, the logic is if we press the X to clear, it will only clear one char at a time. I'm thinking to just clear the mask textbox entirely as well? similar to normal textbox and it will show the placeholder when it's cleared. May I know if there are specs behavior docs for the TextBoxMask for all possible scenario?

[UPDATE] I found it. Will update with the fix and try more tests cases first.

[rizamarhaban]

I have fixed the binding issue if the text value is cleared. Here is my test scenario. In my test, the first row, the TextBoxMask TextBox updated via ViewModel binding. The second row is update via XAML element. Hopefully this test covers the scenario.

[azchohfi]

Build was not passing. I retriggered it merging master.
It would be nice to have this for 7.0.

[michael-hawker]

@azchohfi yup, it's a Stylecop issue not a merge issue. Going to find the line and add a comment here.

[michael-hawker]

@rizamarhaban looks like you had a test app. Was that just something separate you did? I know we've been wanting to try and do more integration tests in the future, but we don't have that setup yet.

In the meantime, would you be up for sharing the code for your test? We could maybe leverage it later when we get test infrastructure setup?

Or feel free to improve the sample in the sample app to exercise some of these cases. Let me know.

Otherwise, looking good. I'll test this out quick again once we're good to go.

[michael-hawker]

@rizamarhaban would you be up for taking your cases from your test app and converting them to unit tests we can have in our CI pipeline? Thanks!

[Rosuavio]

Adding a UI test in #3512

[ghost]

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.