Input `onchange` doesn't fire if container has `onmousedown` handler

[dwaltrip]

Hello! I've really enjoyed using mithril.js, thanks for all the hard work everyone has put into it. I may have run into a troublesome bug for mithril version 0.2.5.

Description:

The onchange handler for an <input type='text'> element does not fire if the following three conditions are met:

1) A parent div has an onmousedown handler set 2) The value attr of the input is manually specified in the view function 3) Focus for the input element is lost by clicking anywhere in the parent div

Setting onclick for the parent container does not cause this problem. I didn't test any other mouse events.

Steps to Reproduce:

var App = {
  controller: function() {
    this.text = '';
  },
  view: function(ctrl) {
    return m('.container', {
      // Setting this handler seems to enable the bug
      onmousedown: function noop() {}
    }, [
      m('input', {
        type: 'text',
        // This handler won't fire if we lose focus by clicking inside the container
        onchange: function(e) {
          ctrl.text = e.target.value;
          console.log('input onchange just fired');
        },
        // Removing this line does cause the `onchange` to fire properly, but that's a non-solution
        value: ctrl.text || ''
      })
    ]);
  }
};

Here is a fiddle that is built around the above code example: https://jsfiddle.net/cyLvcqpo/8/

Expected:

The onchange handler should fire when we enter text into the input and then click on the parent div.

Actual:

The onchange handler does not fire when we enter text into the input and then click anywhere in the parent div.

Importantly, this problem doesn't occur in vanilla JS. Setting an onmousedown handler on a parent element does not prevent onchange handlers from firing for input elements when focus is lost by clicking inside the parent.

[dead-claudia]

Does it repro with next? Also, note that 0.2.x won't likely see many updates soon, since 1.x is nearing release.

[dead-claudia]

Not declining this, though.

[tivac]

This does not repro in mithril@1.0.0-rc.8

https://jsbin.com/neguju/1/edit?js,console,output

[dwaltrip]

Yeah it looks like the most recent of commit of next still has this issue as well.

I figured that might be the case, but thought it was still worth making an issue, in case the fix is not too difficult.

I haven't tried migrating yet, but it looks like it might not be too bad (based on skimming the new docs).

[dwaltrip]

@tivac Hmmm I just checked.

If you set the value attr on the input as in the original example, it reproduces in 1.0.0-rc.8.

https://jsbin.com/zoqobasocu/edit?html,js,console,output

Which is unfortunate =/

[tivac]

Weird, digging into the mithril code it's doing the equivalent of this, more or less, and that's working fine.

[lhorie]

calling e.redraw = false from the mousedown makes it work again (in 1.0.0-rc.8). In 0.2.5, the equivalent would be m.redraw.strategy("none")

I think what's happening is that the mousedown event is triggering a redraw, which updates the value of the input, so by the time an onchange would happen in the event loop, there's no change detected.

[tivac]

Oh, found it.

The onmousedown handler runs before onchange, and the default in mithril is for a redraw to occur after an event handler. So the redraw happens, the value of the <input> gets changed back, and it appears that the pending change event gets lost (maybe it's aborted?).

https://jsbin.com/fuvujof/1/edit?html,js,console,output

If you abort the redraw in the onmousedown handler mithril will skip the redraw after that event handler runs and everything is fine-ish.

EDIT: fuck, beaten

[dwaltrip]

That seems reasonable. However, I'm confused why this doesn't happen with onclick or onmouseup? I just tested both of those on 0.2.5.

Is this something to do with the order in which things fire? Perhaps the order is onmousedown -> onchange -> onmouseup -> onclick?

[tivac]

Seems likely, mousedown probably triggers blur which triggers change, then mouseup fires when you release the mouse button and click happens immediately after that.

I'd probably use oninput for this myself so the field/data-model were kept in sync on every change of the <input> field's value. Haven't tested it but I'm pretty confident that it wouldn't have the same event ordering issue.

@lhorie is going to experiment w/ making the automatic redraw after DOM events run asynchronously (in 1.0.0) which may or may not alleviate this particular situation.

[dwaltrip]

That makes a ton of sense. Thanks all for chiming in.

I should be able to come up with a sensible solution (the actual app is a bit more complex than than the repo example).

[barneycarroll]

👍 for making autoredraw async. The impression I got from the motion to remove sync redraw from the API was that synchronous draw was generally full of danger which should be kept away from unwitting users - going by that logic it should definitely not be the default for event handler autoredraw.

[pygy]

👍 for making autoredraw async.

With the force of a thousand Shia Labeouf, @lhorie DO IT!

(with due apologies for being insistent...)

[lhorie]

Another option is to use onblur instead of onchange. Generally speaking though, you don't want to bind value AND use the element in an uncontrolled manner at the same time ("uncontrolled" being react-speak for components that retain form state in the DOM rather than syncing to a state layer continuously via oninput), because if something else were to redraw (e.g. from a setTimeout or a socket event) before setting the source-of-truth for value, you'd lose the input value.

[dwaltrip]

Yeah my final solution was to use oninput to update the source of the truth (the data model) for the input value without redrawing, and then ensuring a redraw was initiated from onchange.

I didn't want to redraw from oninput, as updates to the model were validated and the UX would be a little weird if invalid characters were aggressively validated via oninput.

lhorie, you make a great point. I actually had another bug which was exactly what you describe: other sources triggering a redraw (a keyboard shortcut, for example) while editing the input. Using oninput to update the source of truth fixed this as well, as the model was then always in sync with visible text on in the input field.

For any google searchers, if redrawing on every oninput is causing UX or performance issues, perhaps the approach of updating the model via oninput while preventing redraws, and then finally ensuring a redraw occurs with an onchange handler will help out.

I'd be curious to learn more how async redraw would play out in this case, and whether there would ever be "timing" issues that would cause inconsistent behavior. Perhaps timing issues would only come up with expensive handlers (e.g. something that takes at least a few milliseconds)? I don't have a solid mental model of this yet.