Bug fixes on v-text-field #2099

azaars · 2017-10-09T12:29:01Z

1. Resolves vuetifyjs#2057 2. Resolves vuetifyjs#2096 3. Resolves vuetifyjs#2097

azaars · 2017-10-09T12:36:44Z

Ups... It breaks some of the tests.

I think those tests are no longer applicable and should be replaced with something else.

KaelWD · 2017-10-09T12:38:51Z

It breaks one of the tests. Have you checked that clearable still works after these changes?

azaars · 2017-10-09T12:40:30Z

Ok, will do so.

KaelWD · 2017-10-09T14:48:27Z

src/components/VTextField/VTextField.js

+        // maskText() masks and removes dirty alphanum
+        // Outer unmaskText() provides a clean lazyValue
+        this.lazyValue = this.unmaskText(this.maskText(this.unmaskText(val)))
+        this.setSelectionRange()


What about non-masked inputs? This will probably break text fields with type="number" as they don't support setSelectionRange().

I did look at non-mask input. I even did some tests on first with a mask then later removed it on the fly.

Yeah you're probably right about type="number". Well, it's not really related to the issues this PR is trying to resolve. There are several other issues that exist even before this PR, which are overlooked yet to surface sooner or later. IMHO lets handle these separately as different issues.

I mean type="number" in general. Currently setSelectionRange is only called if a mask is set:

this.mask && this.setSelectionRange()

What happens with this patch if you just have a number input without a mask?

<v-text-field v-model="number" type="number">

Now I see what you mean.

You can probably just put the this.mask && back in.

Placing back merely the this.mask && caused the clearable test to fail once again. This time it's expecting an emit().

Admittedly, I did not give much attention to the totally valid null value for input field before. So now I've fixed some more lines here and there. For the same reason, the extra check needs to be there once again @jacekkarczmarczyk :

if (!masked.length || !text || !text.length) return text

azaars · 2017-10-09T17:47:41Z

Yeay the test passed :-)

jacekkarczmarczyk · 2017-10-09T17:54:18Z

src/util/mask.js

@@ -90,7 +90,7 @@ const maskValidates = (mask, char) => {
 * @return {String}
 */
 export const maskText = (text, masked, dontFillMaskBlanks) => {
-  if (!masked.length || !text.length) return text
+  if (!masked.length || !text || !text.length) return text


if (!masked.length || !text) return text should be enough

Really? I'm already too deep into strongly typed language hahaa.

Thanks @jacekkarczmarczyk

johnleider · 2017-10-09T18:06:55Z

Tests need to be written for each issue.

azaars · 2017-10-09T18:09:57Z

I'm sorry. I'm not familiar with the test tools at all. Will surely explore but for now it would take me ages to do that.

johnleider · 2017-10-09T18:10:35Z

Sorry, response was mainly for myself to remember to do it :)

KaelWD · 2017-10-11T12:54:00Z

src/mixins/maskable.js

+      if (oldText) {
+        for (const char of oldText.substr(0, selection)) {
+          isMaskDelimiter(char) || position++
+        }


oldText will always be a string, this is redundant

Well, that's because I expect null to be a valid value, and that this.lazyValue has to be able to accept null.

oldText is directly from the html element, which is always a string, even if you set it to null first.

This is still redundant

Indeed it is. input.value never becomes null. Even if it's explicitly set to null, it turns to an empty string. At least in the latest Firefox. I'm changing it right away.

- Dev should be allowed to set input to null and expect v-model to retain the null value. Requesting a reconsideration against returning a string as in commit 95b7f83 - Handle v-model.number modifier correctly. Whether the input is string, number or null, the resulting emit() should return the same type or null. Requesting this as an alternative to resolve vuetifyjs#2137. - Handle input of type number. Likewise, the resulting emit() should return the same type. Requesting this as an alternative to resolve vuetifyjs#2136.

azaars · 2017-10-11T19:29:38Z

src/util/mask.js

-  if (text == null) return ''
-  text = String(text)
-  if (!masked.length || !text.length) return text
+  if (!masked.length || !text) return text


I just found out that !'' === true, so !text.length not only is redundant but will fail when text === null. Still learning 😊

KaelWD · 2017-10-12T15:01:18Z

src/components/VTextField/VTextField.js

+        this.lazyValue = typeof val === 'number' ? +value : value
+        this.mask ? this.setSelectionRange()
+          : this.$emit('input', this.returnMaskedValue
+            ? this.$refs.input.value : this.lazyValue)


Why are we emitting different values based on returnMaskedValue when there is no mask?

Also, this whole function should probably be split by an if statement so none of this casting is happening on non-masked inputs. The same applies for the value watcher too.

KaelWD · 2017-10-12T15:12:11Z

src/util/mask.js

@@ -90,14 +90,15 @@ const maskValidates = (mask, char) => {
 * @return {String}


This docblock needs updating if you're returning arbitrary values. Does jsdoc have generics?

You know better than I do.

http://usejsdoc.org/tags-param.html#multiple-types-and-repeatable-parameters

Fix vuetifyjs#2158

azaars · 2017-10-13T21:47:42Z

Please let me know if I miss anything else.

KaelWD · 2017-10-14T12:40:33Z

src/util/mask.js

@@ -90,14 +90,15 @@ const maskValidates = (mask, char) => {
 * @return {String}
 */
 export const maskText = (text, masked, dontFillMaskBlanks) => {
-  if (text == null) return ''
-  text = String(text)


These lines were here for a reason. The value should still be cast to a string before calculating the cursor position, because that's what's going to be set on the input in the end.

Removing if (text == null) return was intentional because I asked to reconsider having lazyValue to accept null. I described my point here 95b7f83 . Does this mean you not buying the idea?

As for line text = String(text), I didn't remove it. You can find at in line 100, which I think looks more tidy without affecting the logic.

I think accepting null is fine, but completely removing this means that the cursor position is wrong if the mask changes when the value is falsy (0 with v-model.number for example). You should be casting newText to string (with a proper null check) in updateRange() and watch.mask() before the loop. This will also allow the removal of the ifs, because for..of on an empty string just gets skipped.

I can see your point now. All codes that requires the return value of maskText() are expecting a string. So, converting the falsy to '0' is indeed correct. I'm reverting to your last commit.

if (text == null) return text text = String(text)

Might work to keep null values

Well, now I prefer your last commit. Like I said, all readers of maskText() including newValue and domProps.value are expecting string.

KaelWD · 2017-10-14T12:43:07Z

src/components/VTextField/VTextField.js

+      this.lazyValue = val
+      this.$emit('input', this.lazyValue)
+    },
+    getLazyValue (val) {


This name is bad, it doesn't accurately describe what the function is doing. Is this method even needed? You only use it in one place.

It's used in 2 places. The other one is at line 92 of VTextField.js. Do you prefer not to have this this function? I'll just put the content where it was before.

What is it actually doing anyway? It looks like it just casts val to a number when it already is one.

Actually it casts to the type that the dev enters in props.value

It was here, but now it does nothing.

OMG what was I thinking when I did that.

You're damn sharp man. I salute

KaelWD · 2017-10-14T12:45:33Z

src/components/VTextField/VTextField.js

+    },
+    updateOnValueChange (val) {
+      this.lazyValue = val
+      this.$emit('input', this.lazyValue)


This is causing input to be emitted twice, as you're also doing it in both inputValue.set() and updateRange()

Well, actually I didn't quite get it when you said:

Also, this whole function should probably be split by an if statement so none of this casting is happening on non-masked inputs. The same applies for the value watcher too.

I understood it as having if statement to call different functions. Maybe because I didn't understand what you meant about casting issue anywhere in that function. Frankly I also don't like the changes I made and in fact I don't like the new function names I gave. Perhaps you would want to elaborate your suggestion.

On multiple emit problem, I realize that the watch.value has never been completed for what it's supposed to function as. From the comment, it should handle external change but there is nothing in the function to detect whether the change is external or internal. So, there have always been multiple emits from internally. I'm going to address this in a later PR.

I said that because this.unmaskText(this.maskText(this.unmaskText(val))) was being run on every change, even on non-masked inputs.

there have always been multiple emits from internally

<v-text-field :mask="####" v-model.number="num"/>

Click inside, type 1, blur.

On dev:

On this branch:

KaelWD · 2017-10-14T12:47:37Z

src/components/VTextField/VTextField.js

+      this.lazyValue = this.getLazyValue(value)
+      this.$nextTick(() => {
+        this.$refs.input.value = masked
+        this.$emit('input', this.lazyValue)


This one too.

azaars · 2017-10-14T16:12:45Z

What exactly does codeclimate means in the last commit? Where in the functions does it consider it as too complex?

johnleider · 2017-10-14T17:36:48Z

Don't worry about codeclimate for now

- Revert to last commit to undo unnecessary changes - Fix computed.inputValue to account for type of emitted value - Enhance watch.value to check for external/internal change - Prevent resetSelections() from causing runtime error when type="number' - Fix multiple emits bug - Minor change to local variable names to be consistent between functions

azaars · 2017-10-15T13:45:01Z

@KaelWD I've commited the post-review changes.

KaelWD · 2018-01-04T12:28:34Z

src/components/VTextField/VTextField.js

+        this.lazyValue = typeof val === 'number' ? +value : value
+
+        // Emit when the externally set value was modified internally
+        val !== this.lazyValue && this.$nextTick(() => {


why the nextTick here?

KaelWD · 2018-01-04T12:29:16Z

src/mixins/maskable.js

      }

-      this.setCaretPosition(selection)
+      this.$nextTick(() => {


and here too I guess

lock · 2019-04-15T17:58:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

Bug fixes on v-text-field

12de068

1. Resolves vuetifyjs#2057 2. Resolves vuetifyjs#2096 3. Resolves vuetifyjs#2097

KaelWD reviewed Oct 9, 2017

View reviewed changes

Make way for props.clearable=true

4fc4259

jacekkarczmarczyk reviewed Oct 9, 2017

View reviewed changes

Removes unneeded sanity check.

6f07e17

johnleider added the pending merge label Oct 9, 2017

azaars and others added 3 commits October 10, 2017 19:14

null check on externally set value

4b346a3

Add conditional setSelectionRange() & anticipate valid null value.

2fffdb2

Merge branch 'dev' into fix/vuetifyjs#2057

96a095a

KaelWD reviewed Oct 11, 2017

View reviewed changes

azaars commented Oct 11, 2017

View reviewed changes

KaelWD self-requested a review October 12, 2017 01:07

KaelWD reviewed Oct 12, 2017

View reviewed changes

Clean up codes involving conditional mask

5852457

Fix vuetifyjs#2158

KaelWD added pending review The issue is still pending disposition and removed pending merge labels Oct 14, 2017

KaelWD reviewed Oct 14, 2017

View reviewed changes

Removes redundant check on oldValue (input.value)

dc53240

Removes useless getLazyValue()

c5fdc83

KaelWD added this to the v0.17.x milestone Oct 15, 2017

johnleider removed the pending review The issue is still pending disposition label Oct 18, 2017

johnleider merged commit 54e2b78 into vuetifyjs:dev Oct 18, 2017

azaars deleted the fix/#2057 branch October 26, 2017 04:04

KaelWD mentioned this pull request Oct 29, 2017

fix: invalid textfield behaviour with number value #2360

Merged

KaelWD reviewed Jan 4, 2018

View reviewed changes

src/mixins/maskable.js

}

this.setCaretPosition(selection)

this.$nextTick(() => {

Copy link

Member

KaelWD Jan 4, 2018

Choose a reason for hiding this comment

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

and here too I guess

KaelWD mentioned this pull request Jan 4, 2018

[Bug Report] Input doesn't update correctly #2931

Closed

lock bot locked as resolved and limited conversation to collaborators Apr 15, 2019

		@@ -90,14 +90,15 @@ const maskValidates = (mask, char) => {
		* @return {String}

Bug fixes on v-text-field #2099

Bug fixes on v-text-field #2099

Conversation

azaars commented Oct 9, 2017 • edited

azaars commented Oct 9, 2017

KaelWD commented Oct 9, 2017

azaars commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaars commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnleider commented Oct 9, 2017

azaars commented Oct 9, 2017

johnleider commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaelWD Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaars commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaelWD Oct 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaelWD Oct 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaars commented Oct 14, 2017 • edited

johnleider commented Oct 14, 2017

azaars commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Apr 15, 2019

azaars commented Oct 9, 2017 •

edited

KaelWD Oct 12, 2017 •

edited

KaelWD Oct 14, 2017 •

edited

KaelWD Oct 15, 2017 •

edited

azaars commented Oct 14, 2017 •

edited