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

Multiple input events on MdInput #1261

Closed
korylprince opened this issue Dec 4, 2017 · 8 comments
Closed

Multiple input events on MdInput #1261

korylprince opened this issue Dec 4, 2017 · 8 comments
Labels

Comments

@korylprince
Copy link
Contributor

korylprince commented Dec 4, 2017

Edit: See this comment for what I believe fixes this issue.

Steps to reproduce

Use vue-dev-tools on the vue-material docs. (I can't seem to get the dev tools to activate on vuematerial.io, so I'm using a local copy a la npm run dev.)

Watch the events and type in an input, textarea or change a select.

Which browser?

Latest Vue, git dev version of vue-material, latest stable Chrome

What is expected?

I would expect to only receive one input event. In the case of the select, I would expect to receive one event with the value of the selection.

What is actually happening?

You'll see two or three events for each key stroke. In case of the select, you'll get one input event with the value of the selection and another with the inner text of the selection.

Investigation

In the case of MdInput, there's three sources that these events come from. First in listeners() in MdInput.vue.

In MdFieldMixin.js the set(value) for the computed model, and the watcher for value both set localValue which causes an emit.

I don't know enough about how MdInput is supposed to work to fix it. I did notice that set(value) for model checks if value is an InputEvent. In all my testing the only thing value ever is is a String.

@korylprince
Copy link
Contributor Author

korylprince commented Dec 5, 2017

Update: I'm realizing now, at least for the select problem, that one input event is coming from the MdSelect, and one is coming from the MdInput (built into the MdSelect?), which makes sense. But I still wouldn't think you'd want both events firing...

Edit: I'm dumb. Just because the the Vue dev tools shows the event doesn't mean it get's passed up to the parent. So the MdSelect is working just fine.

But the original issue with MdInput is still the same. The parent is seeing multiple input events.

@korylprince
Copy link
Contributor Author

Okay with the above in mind, I've isolated the events to two places. MdInput's $listeners (i.e. @input on the actual input) and to the localValue watcher in MdFieldMixin.

I can't find a time when localValue doesn't emit on a change, so it seems like the input listener in MdInput is redundant. Perhaps it should become something like:

  listeners () {
    var l =  this.$listeners
    delete l.input
    return l
  }

or something similar? If someone really wanted to get the native input event, they could just use @input.native.

If this is acceptable, I can do a pull request to fix this issue with MdInput and MdTextarea.

@VdustR
Copy link
Member

VdustR commented Dec 6, 2017

  listeners () {
    var l =  this.$listeners
    delete l.input
    return l
  }

This way would modify the $listener object from Vue API which is read only therefore I prefer to construct another object. And it's really not good to change any data in a computed.

@korylprince
Copy link
Contributor Author

@VdustR Yeah, that was just example what I think listeners should output. I didn't want to complicate my example with object-cloning code.

@VdustR
Copy link
Member

VdustR commented Dec 6, 2017

try

  listeners () {
    return {
        ...this.$listeners,
        input: undefined
    }
  }

@korylprince
Copy link
Contributor Author

Oh, that's much more elegant than I was thinking. I'm just waiting on some input from the devs before I do a pull request.

@Samuell1
Copy link
Member

Samuell1 commented Dec 8, 2017

@korylprince make PR and we can look at it. Its faster then waiting for response in issue.

@korylprince
Copy link
Contributor Author

@Samuell1 👍

marcosmoura pushed a commit that referenced this issue Dec 22, 2017
…irectly (#1285)

* (Fix #1261) Don't emit input directly in MdInput and MdTextarea

* use spread operator (+babel) instead of Object.assign for browser compatiblity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants