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

Fix bug in X-Mask where user input that matched the non wildcard value were being stripped #4253

Closed
wants to merge 4 commits into from

Conversation

robertmarney
Copy link

@robertmarney robertmarney commented Jun 4, 2024

Closes #4252

Found where a non wildcard value matched the user input the value was being stripped, I added an additional check before stripping and added a couple test cases to the jest tests.

@@ -137,7 +137,7 @@ export function stripDown(template, input) {
}

for (let j = 0; j < inputToBeStripped.length; j++) {
if (inputToBeStripped[j] === template[i]) {
if (inputToBeStripped[j] === template[i] && template[i] !== inputToBeStripped[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can never evaluate to true.

It's saying "if these match and if they also don't match" which is impossible.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @ekwoka - Thanks for the quick feedback.

I disagree, note that the new conditional is not using the same iterator for inputToBeStripped (we are using i not j)

From mask.spec.js:L11

    expect(stripDown('(999) 999-9999', '716 2256108')).toEqual('7162256108')

template = "(999) 999-9999"
input = "716 2256108"

// when i = 5 and j = 3 we see inputToBeStripped[j] = " " and template[i] = " " (so first condition match), 
// then we see inputToBeStripped[i] = 2, 

// This matches the condition the space is removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, you're right. That's hard to read.

@ekwoka
Copy link
Contributor

ekwoka commented Jun 5, 2024

There isn't actually a bug. It was behaving as expected.

But this actually breaks behavior, but seemingly there weren't appropriate tests to catch that breakage.

@@ -11,6 +11,8 @@ test('strip-down functionality', async () => {
expect(stripDown('(999) 999-9999', '716 2256108')).toEqual('7162256108')
expect(stripDown('(999) 999-9999', '(716) 2256108')).toEqual('7162256108')
expect(stripDown('(999) 999-9999', '(716) 2-25--6108')).toEqual('7162256108')
expect(stripDown('+1 (999) 999-9999', '7162256108')).toEqual('7162256108')
Copy link
Contributor

@ekwoka ekwoka Jun 5, 2024

Choose a reason for hiding this comment

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

Suggested change
expect(stripDown('+1 (999) 999-9999', '7162256108')).toEqual('7162256108')
expect(stripDown('+1 (999) 999-9999', '+1 (716) 225-6108')).toEqual('7162256108')

this would be a more accurate test to what you wanted to "fix". Which will likely fail, outputting 1716225610

Copy link
Author

@robertmarney robertmarney Jun 5, 2024

Choose a reason for hiding this comment

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

Yes you are correct that this solution does not handle that latter case, I think both your suggestion and what I introduced would be good introductions to the spec and would mimic behavior of other masking tools (e.g. maska https://codepen.io/Robert-Marney/pen/LYoLVRb)

In the pen above you can see the mask which is equivalent to the test case can handle both types of user input (e.g. a direct mask format as well as the numbers represented by the wildcard fields)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify, which bug qre you trying to fix here? The example you posted (maska) behaves exactly as x-mask.
If you type 1, this generates +1 and not +1 (1.

Copy link
Author

Choose a reason for hiding this comment

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

@SimoTod - The bug is the application of autocomplete values

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, that is the first time you mention it.
I think you need to describe the problem clearly otherwise when the maintainer looks at the issue, he will not understand what you are trying to solve.

Are we saying that the other plugins work differently depending on you entering the number vs copying vs using autoconplete? In that case, it looks like x-mask does a better job at staying consistent. It also sounds like it would be a lot of work to make it work the way you like.

If it's causing pains in your application, you can maybe disable the autocomplete?

Copy link
Author

Choose a reason for hiding this comment

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

Hey @SimoTod - Your feedback is fair, apologies for the lack of clarity.

If I can get a working solution would you accept a PR?

As @ekwoka mentioned, as tests are passed, we might want to align on the spec (preference for Jest) of what we expect the mask to do for some of these more complex cases?

@robertmarney
Copy link
Author

There isn't actually a bug. It was behaving as expected.

But this actually breaks behavior, but seemingly there weren't appropriate tests to catch that breakage.

To confirm provided a user input of 3133133113' the expectation is the output is +1(333)133-113?

Perhaps its the special nature of the phone number input but we definitely see a large number of users that have the more 'raw' format stored in autocomplete (and we would like to leverage if we can and do with other masking tools)

image

image

@robertmarney
Copy link
Author

Closing in favor of: #4260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants