vuematerial / vue-material

Vue.js Framework - ready-to-use Vue components with Material Design, free forever.
https://www.creative-tim.com/vuematerial
MIT License
9.9k stars 1.15k forks source link

[MdInput] causes v-model lag on mobile #1835

Open FossPrime opened 6 years ago

FossPrime commented 6 years ago

Version

vue 2.5.16 vue-material 1.0.0-beta-10.2

Reproduction link

https://codesandbox.io/s/7o3qm76x6j

The top field is an MdInput field, the bottom ones are normal fields... this all works fine on desktop, but the MdInput one is unusably slow to update on Chrome on Android emulator and real phones. It infact, won't update at all unless space is pressed or focus is lost

Working version with vue-material, but md-input swapped out https://codesandbox.io/s/2z1k6wnwqr

Steps to reproduce

type two words on Chrome 66 on an Android phone or emulator. The pixel and pixel images work.

What is expected?

When you click "create todo" validateForm reads form.todo. Unfortunately, it only updates after every space bar press.

What is actually happening?

Why do we need a cache? why not just pass around the model directly as a property? Seems a bit overkill for an input field.


I tried v-model.lazy and initializing form to form: todo: ''... same issue

Workaround

Use nextTick with the promise syntax on your submit

 validateTodo () {
        this.$nextTick().then(() => {
          this.todos.push({ name: this.form.todo })
          this.form.todo = ''
        })
      }

kapture 2018-06-29 at 12 35 08

FossPrime commented 6 years ago

@Samuell1 not surprising... disabling spell check makes the problem unnoticeable. This is a race condition.

My first guess is @click is processed before nextTick, but that doesn't explain why the <input> works. <MaterialInput> brute force fixes the issue. Any ideas what I should be looking at? My first inclination is to try to get rid of the watch's and try to use the property more directly

FossPrime commented 6 years ago

Good news and bad news:

MdInput: screen shot 2018-06-29 at 3 09 04 pm

validateTodo () {
      // MdInput bug on Phones only https://github.com/vuematerial/vue-material/issues/1835
      console.log({
        miv: this.$refs.yolo.value,
        milv: this.$refs.yolo.localValue,
        mim: this.$refs.yolo.model,
        elv: this.$refs.yolo.$el.value,
        m: this.form.todo
      })
      this.$nextTick().then(() => {
        console.log({
          miv: this.$refs.yolo.value,
          milv: this.$refs.yolo.localValue,
          mim: this.$refs.yolo.model,
          elv: this.$refs.yolo.$el.value,
          m: this.form.todo
        })
      })
      this.create({
        name: this.form.todo
      })
      this.form.todo = ''
      console.log({
        miv: this.$refs.yolo.value,
        milv: this.$refs.yolo.localValue,
        mim: this.$refs.yolo.model,
        elv: this.$refs.yolo.$el.value,
        m: this.form.todo
      })
    }

Native input: screen shot 2018-06-29 at 3 05 59 pm

validateTodo () {
      // MdInput bug on Phones only https://github.com/vuematerial/vue-material/issues/1835
      console.log({
        v: this.$refs.ylfe.value,
        m: this.form.todo
      })
      this.$nextTick().then(() => {
        console.log({
          v: this.$refs.ylfe.value,
          m: this.form.todo
        })
      })
      this.create({
        name: this.form.todo
      })
      this.form.todo = ''
      console.log({
        v: this.$refs.ylfe.value,
        m: this.form.todo
      })
    }
Samuell1 commented 6 years ago

@rayfoss can you define your v-model in data? because form.todo doesnt not exists in your object, that is maybe causing issue

FossPrime commented 6 years ago

@Samuell1 Yes, I tried that... turns out an event handler that changes data triggers reactivity which is really convenient. v-model emit's are event handlers. In the submit I changed data. I updated the example with your suggestion. It's still broken.

The best solution I've found so far is to do change the MdInput template to

<template>
  <input
    class="md-input"
    v-model="model"
    v-bind="attributes"
    v-on="listeners"
    @input="$emit('input', $event.target.value)"
    @focus="onFocus"
    @blur="onBlur">
</template>

The downside is that MdField also emits changes on nextTick... so on desktop that would be twice the events as there are now. Setting the MdFieldMixin model in data instead of a computer setter will get it updated faster, but that won't do us any good as we still need to emit somehow. I tried emitting from the setter, that won't reach us in time.

I think the best solution is to get rid of MdFieldMixin emit's and make the individual components do the heavy lifting where possible. If not possible, take a "slow" property to make it clear you'll need to wait till nextTick to use the data.

That emit has some very pleasant side effects... everything works better with it... the computed properties fire more frequently, all our values are updated by input time. It's actually faster than a regular input field when using spellcheck on mobile.

Even asking everyone to use nextTick still leaves us open to a very tedious hard to debug race condition, where you can modify the model in your component and then have the model be updated again by a latent emit.

I think @input="$emit('input', $event.target.value)" is the way to go, no one is going to complain about it performing faster then input... people will complain about it not being a drop in replacement to input. Afterall, md-input was always supposed to be a super heroic version of input, if it works better than input, that's by design.

FossPrime commented 6 years ago

alternative solution looks like the computed model setter does get the message in time, and an emit from there will find it's way to the parent in time. But the race condition is still a problem. There are still several watches around that won't react till nextTick, and they'll set the model back to the original value.

computed: {
    model: {
      get () {
        return this.localValue
      },
      set (value) {
        this.$emit('input', value)
        console.log(value)
        this.localValue = value
      }
    },

Question remains... why are we currently waiting till nextTick anyway? @Samuell1 What does this do? why wait till next tick, allowing for the race condition to exist... why are we waiting for a slow watch to emit, why not emit here?

  if (value.constructor.toString().match(/function (\w*)/)[1].toLowerCase() !== 'inputevent') {
          this.$nextTick(() => {
            this.localValue = value
          })
        }

Update: The PR I submitted reacts exactly as fast as the normal input, without giving it any super powers unlike an "in element" emit.

FossPrime commented 6 years ago

Before I forget... the best way to test for this, is to check the race condition, which should happen on desktop. Wrapping this in a form, making a direct $el.value change and then triggering submit should reproduce it in a test.

Samuell1 commented 6 years ago

@rayfoss iam not sure why is that there you need to ask more about this from @marcosmoura But i think this is because some performance issues?

FossPrime commented 6 years ago

Well I didn't break any tests so it must not be important, jk. Maybe were over thinking this... It's clearly to make all changes happen on the same tick, as there are a lot of slow watches.

The purpose of the regex is not important to us for this issue. We just needed to update fater, which the PR focuses on.

mismith commented 6 years ago

Just released a Cordova app to the wild and this is causing the app to crash on even modern iOS devices.. :/ anything I can do to help? I'm currently thinking of just using a regular input inside the md-field, but then I lose some of the niceties like md-clearable

FossPrime commented 6 years ago

@mismith while you shouldn't have to do this, it's probably a good idea to automatically require all submit handlers to have a next tick wrapper in them... no idea how to automate the enforcement of that.

mismith commented 6 years ago

In my case I'm not even using one, I'm filtering a list of results (after minor debouncing), so that won't fix much for me.

FossPrime commented 6 years ago

@mismith now that you mention it... it may explain some weird issues I was having with a list filtering input field on mobile... checkout the PR... I get it's a frustrating bug.

FossPrime commented 6 years ago

@Samuell1 this is very very much a confirmed bug that haunts me daily, can we add the bug and Pull-Request-available label to this

FossPrime commented 6 years ago

@marcosmoura any plans to fix this? I can pitch in more as well. I run into this bug almost daily. See my PR... if you'd like me to refactor it for another branch, I can

agr1277 commented 6 years ago

I am also experiencing this behavior, and it is causing severe issues. In fact, I have at least one instance where I have to wrap the submit handler in two $nextTick callbacks before it picks up the input. If @rayfoss's solution is valid, I'd really appreciate it being merged in.