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

conditional change of <input> type attribute not working. #6313

Closed
trusktr opened this issue Aug 8, 2017 · 16 comments · Fixed by #6344
Closed

conditional change of <input> type attribute not working. #6313

trusktr opened this issue Aug 8, 2017 · 16 comments · Fixed by #6344

Comments

@trusktr
Copy link

trusktr commented Aug 8, 2017

Version

2.4.2

Reproduction link

https://jsfiddle.net/trusktr/50wL7mdz/52198/

Steps to reproduce

Try to type in the text field. It will keep clearing your input every second.

What is expected?

It should toggle between showing and hiding the password.

What is actually happening?

It is destroying the input (or clearing its value) each time the value of hidden changes.

@trusktr
Copy link
Author

trusktr commented Aug 8, 2017

Using v-if/v-else also doesn't work as would be intuitively expected: https://jsfiddle.net/trusktr/50wL7mdz/52199

Why is it not dom-diffing and applying the change only to the attribute value like one would expect?

@trusktr trusktr changed the title https://github.com/vuejs/vue/issues/new [bug] conditional change of input type clears input value! Aug 8, 2017
@trusktr trusktr changed the title [bug] conditional change of input type clears input value! [bug] conditional change of <input> type not working? Aug 8, 2017
@trusktr trusktr changed the title [bug] conditional change of <input> type not working? [bug] conditional change of <input> type attribute not working? Aug 8, 2017
@trusktr
Copy link
Author

trusktr commented Aug 8, 2017

Here's the same thing, broken, using a self-closing input tag: https://jsfiddle.net/trusktr/50wL7mdz/52202/

@trusktr
Copy link
Author

trusktr commented Aug 8, 2017

Here's a fiddle showing that it works with vanilla HTML/JS: https://jsfiddle.net/trusktr/50wL7mdz/52205/

This raises the concern that Vue is not dom diffing like it claims. If it were, then only the type attribute would end up modified, and the value inputted text would remain inside the input.

@trusktr
Copy link
Author

trusktr commented Aug 8, 2017

@syropian suggested to add a dummy v-model as a workaround, which works, but that's not intuitive and is wasteful: https://jsfiddle.net/50wL7mdz/52203

@nickmessing
Copy link
Member

@trusktr, because type attribute on input is very special this is expected behaviour without v-model. You cannot carry same value around (think of input[type="file"] or even input[type="number"]).

@trusktr
Copy link
Author

trusktr commented Aug 8, 2017

@nickmessing That's not intuitive, please fix.

This is the sort of thing that should work in Vue because it works in the browser.

Sure, switching from password to file won't work, and in that case you could re-render. I would do whatever the browser actually does. Otherwise Vue isn't dom diffing like one would assume.

But switching to text should work. Vue can make this intuitively easy. (Isn't it a framework like Vue's goal to make these things easy?).

@trusktr
Copy link
Author

trusktr commented Aug 8, 2017

Do you not see the fact that someone telling me to unituitively use v-model as a workaround is a problem?

It is frustrating when a library's issue gets closed if the library author can easily improve the library for the benefit of all the users at the cost of a small one-time extra effort on the library author's side, and when the library author chooses perhaps author convenience over true benefit of the user.

You could instead leave this open and suggest a PR he made. That's much better a choice.

@nickmessing
Copy link
Member

nickmessing commented Aug 8, 2017

@trusktr, Well, re-rendering diffs indeed, let's imagine a simple scenario.
Step 1. First render, DOM looks like:

<input type="text" />

Step 2. You add some text, DOM looks like (consider that value is describing a domProp and not attr here)

<input type="text" domProp-value="asdasd" />

Step 3. You change hidden so now vue should calculate the diff and apply it between this 2:

<input type="text" domProp-value="asdasd" />
<input type="password" />

Result: value gets removed. This is intuitive if you understand virtual dom based re-rendering. It does remove value because there is no value in new input. v-model helps only because it is replaced by :value="str" @input="str = $event.target.value and as you see we have value.

@trusktr
Copy link
Author

trusktr commented Aug 9, 2017

That example isn't really applicable to my case, and you also mentioned some implementation details that end users don't need to care about.

The case is simple: if the only thing changing is type password to type text or vice versa, then you can optimize by DOM diffing as expected in that case, and end users won't run into unexpected behavior.

Another way to put it is: if I can simply change from type password to type text when using vanilla DOM+JS, then it should be as simple in Vue too, because Vue is(should be) an extension of what people expect from vanilla DOM.

Honestly this is good enough reason to make this optimization on your end, in this case and any other similar cases, to make it as intuitive as possible for end users.

@trusktr trusktr changed the title [bug] conditional change of <input> type attribute not working? [BUG] conditional change of <input> type attribute not working. Aug 10, 2017
@trusktr
Copy link
Author

trusktr commented Aug 10, 2017

I've updated the title to remove the question mark.

Even with the v-model hack, DOM Diffing is not working. Vue is destroying the existing input element and creating a whole new one (rather than just modifying the type attribute).


This is REALLY bad.


This destroys interoperability with outside code. For example, bootstrap validator stops working because it will have a reference to a no-longer-existing input element.


If Vue is supposed to be incrementally adoptable, and interoperable with outside code (f.e. jQuery plugins) then this simply is not acceptable.

@trusktr
Copy link
Author

trusktr commented Aug 10, 2017

@nickmessing

If Vue is always dom-diffing like it claims, here’s a fiddle that should work: https://jsfiddle.net/trusktr/50wL7mdz/52661/

Start typing in the input field, and you will see the values logged to console.

Then, when you toggle the show/hide of the password, if Vue were dom-diffing properly, it would be changing only the type attribute of the input. If this were the case, then you’d continue to see output in the console.

But this isn’t the case because now there’s no input in the console.

Each time the show/hide is toggled, Vue replaces the entire input element with a new one.

This broke the simple logValues tool, and it will lead to code that will leak un-used DOM into memory.

@trusktr
Copy link
Author

trusktr commented Aug 10, 2017

Here’s the same example, using React, and dom-diffing works as expected: https://jsfiddle.net/trusktr/69z2wepo/84211/

@nickmessing
Copy link
Member

@trusktr, It indeed makes sense to change the type only when it goes from one text-like input to another.

Vue was never claiming be interoperable with outside code. Rather then that, vue guarantees to keep in sync actual dom with virtual dom when using it's reactivity system and it's impossible to predict how it's going to do that since internal API can and will change over time. This is not a bug since it's keeping in sync the actual dom with virtual dom defined by template. It's probably sub-optimal to replace whole input when switching from one text-like to another and I will try to optimize that but Vue was never designed to "be friendly" with jQuery plugins.

@nickmessing nickmessing reopened this Aug 10, 2017
@yyx990803 yyx990803 changed the title [BUG] conditional change of <input> type attribute not working. conditional change of <input> type attribute not working. Aug 10, 2017
@trusktr
Copy link
Author

trusktr commented Aug 10, 2017

it's impossible to predict how it's going to do that since internal API can and will change over time.

True, but some guarantee of atomicity can be proposed regardless of implementation: if only an attribute is supposed to change based on the Vue template, then the only thing in DOM that should change is the corresponding real attribute.

sub-optimal to replace whole input when switching from one text-like to another

It's probably suboptimal to ever replace the input no matter what the type transition. When changing type from text to file, DOM changes value to empty string. I don't think a user is going to try that though. If they do, they can simply use either (or both) v-on:input and v-on:change as needed. They can make that choice the same way they would with vanilla DOM.

In any case, I don't think there's any need to replace the input element regardless of which type change happens, because you simply can't predict what the user is doing or why, and it is fair for Vue not to make those assumptions (even if unintentional).

@trusktr
Copy link
Author

trusktr commented Aug 10, 2017

Here's a fiddle that shows input and change events with a type change from text to file: https://jsfiddle.net/trusktr/02gvxyLv/1/

I think it is the interest of a tool to work with existing standardized DOM functionality, not working against expectations of how DOM works.

@yyx990803
Copy link
Member

yyx990803 commented Aug 11, 2017

FYI this is an intentional mechanism for dealing with <input> with v-model bindings + different type bindings. When you toggle between, say type="text" and type="checkbox", the event/value bindings (generated at compile time) would be different and it is more straightforward to replace the element in those cases.

Toggling between text and password is a use case that's not been considered before and is easy to fix.

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 a pull request may close this issue.

3 participants