Skip to content
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.

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

TextBoxMask TextChanging event handler bug #3279

Closed
oenarap opened this issue May 13, 2020 · 7 comments · Fixed by #4048
Closed

TextBoxMask TextChanging event handler bug #3279

oenarap opened this issue May 13, 2020 · 7 comments · Fixed by #4048
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior extensions ⚡ good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities In-PR 🚀
Projects
Milestone

Comments

@oenarap
Copy link

oenarap commented May 13, 2020

TextBoxMask leaves an artifact (i.e., old Text value) when a new value is set. (in my case, thru data binding)

since this is just a subtle bug, please allow me to not comply with your required details for this report.

and let me get you directly where i found the offending part of the code:

public partial class TextBoxMask
{
    ...

   private static void Textbox_TextChanging(TextBox textbox, TextBoxTextChangingEventArgs args) 
   {
   ...

    // case adding data at the end of the textbox
    if (oldSelectionStart >= oldText.Length && !isDeleteOrBackspace)
    {
        textbox.Text = oldText;
        if (oldText.Length >= 0)
        {
            textbox.SelectionStart = oldText.Length;
        }
            return;
        }
    ...
    }
}

1. within the if condition, textbox.Text will revert to old value even if you actually intended to assign a completely new/different value to it.

in the subsequent part of the same event handler:

        // Case change due to Text property is assigned a value (Ex Textbox.Text="value")
        if (textbox.SelectionStart == 0 && textbox.FocusState == FocusState.Unfocused)
        {
            var displayText = textbox.GetValue(DefaultDisplayTextProperty) as string ?? string.Empty;
           
            if (string.IsNullOrEmpty(textbox.Text))
            {
                textbox.Text = displayText;
            }
            else
            {
                ...
            }
        }
        ...
        textbox.Text = new string(textArray);
        ...
    }

2. the nested if condition will allow execution of the remaining part of the code which ends up in textbox.Text getting assigned with the old value, even if you actually intend to assign a null/emptly string to it.

here's the quick fix i came up with:

        ...
        // case adding data at the end of the textbox
        if (oldSelectionStart >= oldText.Length && !isDeleteOrBackspace)
        {
            // ignore change(s) if oldtext is a substring of new text value
            if (textbox.Text.Contains(oldText, StringComparison.OrdinalIgnoreCase))
            {
                textbox.Text = oldText;

                if (oldText.Length >= 0)
                {
                    textbox.SelectionStart = oldText.Length;
                }
                
                return;
            }
        }
        ...

        // Case change due to Text property is assigned a value (Ex Textbox.Text="value")
        if (textbox.SelectionStart == 0 && textbox.FocusState == FocusState.Unfocused)
        {
            string displayText = textbox.GetValue(DefaultDisplayTextProperty) as string ?? string.Empty;
            
            if (string.IsNullOrEmpty(textbox.Text))
            {
                textbox.SetValue(OldTextProperty, displayText);
                textbox.SetValue(OldSelectionStartProperty, 0);
                textbox.SetValue(OldSelectionLengthProperty, 0);
                textbox.Text = displayText;
                return;
            }
            else
            {
                ...
            }
        }
        ...

THANKS!

@oenarap oenarap added the bug 🐛 An unexpected issue that highlights incorrect behavior label May 13, 2020
@ghost ghost added the needs triage 🔍 label May 13, 2020
@ghost
Copy link

ghost commented May 13, 2020

Hello oenarap, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@michael-hawker
Copy link
Member

Thanks @oenarap for the report! If you have a fix, do you want to make a PR?

@michael-hawker michael-hawker added this to the 6.1 milestone May 13, 2020
@michael-hawker michael-hawker added extensions ⚡ good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities and removed needs triage 🔍 labels May 13, 2020
@michael-hawker michael-hawker added this to To do in Bugs 6.1 via automation May 13, 2020
@oenarap
Copy link
Author

oenarap commented May 14, 2020

Thanks @michael-hawker!

PR would be fine with me. For the meantime though, changes i can make could be just a copy of my quick fix included in the bug report. It worked for me, but i'm sure you guys can come up with a better one.

All best!

@oenarap
Copy link
Author

oenarap commented May 16, 2020

hi @michael-hawker. any particular branch that i may be allowed to make a PR?

@Kyaa-dost
Copy link
Contributor

@oenarap you can create a new branch of your own and we can merge that later in the master once the PR is approved 🙂

@Kyaa-dost Kyaa-dost removed this from To do in Bugs 6.1 Jun 22, 2020
@Kyaa-dost Kyaa-dost added this to To do in Bugs 7.0 via automation Jun 22, 2020
@Kyaa-dost Kyaa-dost modified the milestones: 6.1, 7.0 Jun 22, 2020
@Kyaa-dost
Copy link
Contributor

@oenarap not sure if you had a chance but once you have established all the changes in the forked branch then sync your PR with the main branch of WCT and feel free to submit the PR.

Also, feel free to refer to the "Create/Submit the PullRequest" options in our new WIki page for any further guidance you may require.

@Kyaa-dost
Copy link
Contributor

@oenarap please let us know of any updates.

@Kyaa-dost Kyaa-dost added this to To do in Bugs 7.1 via automation Feb 18, 2021
@Kyaa-dost Kyaa-dost removed this from To do in Bugs 7.0 Feb 18, 2021
@Kyaa-dost Kyaa-dost modified the milestones: 7.0, 7.1 Feb 18, 2021
@ghost ghost added the In-PR 🚀 label May 27, 2021
Bugs 7.1 automation moved this from To do to Done Jul 16, 2021
michael-hawker added a commit that referenced this issue Jul 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior extensions ⚡ good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities In-PR 🚀
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants