Value of field is reset to original value on blur when using two-way binding in 4.0.0

[jameshfisher]

Here's a test case using the 0.4.0 commit of Ractive: http://jsfiddle.net/UyGfJ/

STR:

1. Click into the input box.
2. Click out of it.

Expected behavior: value stays the same, i.e. "somethingelse".

Actual behavior: value changes back to initial value before being programmatically changed, i.e. "fromractive".

Notice that this behavior differs from Ractive 0.3.7, as shown here: http://jsfiddle.net/2xuMP/1/

[jameshfisher]

This change in behavior is not obviously described in the CHANGELOG, which makes me think it's a bug.

[jameshfisher]

Also note this fiddle which is the same as the buggy one except for setting twoway to false: http://jsfiddle.net/UyGfJ/2/

This stops the buggy behavior, so it's certainly something to do with two-way binding.

[jameshfisher]

Present in 0.3.8; not present in 0.3.7.

[jameshfisher]

Can't see such behavior change described in the 0.3.8 section of the CHANGELOG.

[MartinKolarik]

Yes, it's two-way data binding. Ractive is bound to blur event, so when blur is fired, it syncs the model with the value from DOM. You are probably right, the DOM value should take precedence.

[jameshfisher]

git bisect tells me:

f38947098e7039663ed3eebb2d3e0daa6106ddef is the first bad commit
commit f38947098e7039663ed3eebb2d3e0daa6106ddef
Author: Rich Harris <richard.a.harris@gmail.com>
Date:   Sat Dec 14 15:18:05 2013 -0500

    merge 0.3.8 -> master

[martypdx]

Issue is here:

    GenericBinding = function ( attribute, node ) {
        inheritProperties( this, attribute, node );

        node.addEventListener( 'change', updateModel, false );

        if ( !this.root.lazy ) {
            node.addEventListener( 'input', updateModel, false );

            if ( node.attachEvent ) {
                node.addEventListener( 'keyup', updateModel, false );
            }
        }

        this.node.addEventListener( 'blur', update, false );
    };

It doesn't fail any tests if I change the blur handler to updateModel. Given that the teardown also uses update, I'll let @Rich-Harris chime in on what was intended.

[jameshfisher]

@martypdx since I'm not familiar with the codebase and you guys seem to be on it, I'll leave this to you. Thanks!

[Rich-Harris]

I can't remember the specific issue where this was discussed in more depth, but the blur behaviour was added for cases where data is being validated in some way (e.g. <input value='{{foo}}'> where foo is an adaptor with validation rules, or there's an observer:

ractive.observe( 'foo', function ( foo ) {
  // must be between 0 and 100
  this.set( 'foo', Math.max( 0, Math.min( 100, foo ) );
});

In those cases, you want the input's value to reflect the validated model - but you want to do it on blur because changing the value of a focused element leads to all sorts of craziness.

Here I would argue that the behaviour in the first fiddle is correct - Ractive's primary job is to keep the UI up to date with its view of the world. The ractive.updateModel() method is designed to accommodate cases like this, where an input has been changed programmatically - that's how to keep its view of the world up-to-date.

Part of me is tempted to say we should use @martypdx's fix, since that shouldn't (would have to check) prevent any validation rules from being applied - it just means another update-model-then-update-view round-trip.

But I'm not totally convinced - it seems arbitrary to suggest that a blur event should be the thing that updates the model. If the input is being modified programmatically it seems to me that Ractive's model should be updated programmatically as well, no?

[martypdx]

Ah yes, now I remember the thing about validation value.

And it makes sense that programmatically, you would do:

document.getElementsByTagName("input")[0].value = "somethingelse";
ractive.updateModel(); //this syncs the model

I'd have to play around with this some more to have a better opinion.

[jameshfisher]

Here's my use case which caused me to track down this bug: I am using bootstrap-datetimepicker with a text field to allow the user to modify the textual representation of a datetime in a field. Apparently, when picking dates using this library, it updates the field programmatically in the same way as in my example. The behavior was thus that the user would pick a date, it would appear in the I expect that most other widgets that wrap around a basic HTML form field will update the backing field in the same way.

What would be the recommended approach for integrating libraries like this into Ractive?

[Rich-Harris]

You can use something called decorators: http://docs.ractivejs.org/latest/writing-decorator-plugins

There's a couple of recent Stack Overflow answers which provide some more real-world examples: http://stackoverflow.com/questions/23083841/ractivejs-and-jquery-plugins and http://stackoverflow.com/questions/23265269/how-do-i-access-an-object-in-a-decorator-in-ractivejs.

Decorators are probably the best solution to this problem because they're reusable and can take care of all the two-way binding/lifecycle stuff.

[MartinKolarik]

In those cases, you want the input's value to reflect the validated model

I think you want to validate the input's value, not the model.

In normal case, ractive.get('foo') === input.value, so it doesn't matter which one you pass to the observer. When observer calls set both values will be changed.

In case there is a difference between ractive.get('foo') and input.value, the observer should be called with input.value as newValue. If there is a validation and observer calls set both input.value and ractive.data.foo should be updated.

it seems arbitrary to suggest that a blur event should be the thing that updates the model

The blur event already is used to update the model with lazy: true

If the input is being modified programmatically it seems to me that Ractive's model should be updated programmatically as well, no?

I think that Ractive shouldn't restore the original value on blur if it was changed. In most cases Ractive isn't able to detect that DOM was changed programatically and it will change it only on next model update. http://jsfiddle.net/8RCky/

[Rich-Harris]

The blur event already is used to update the model with lazy: true

Okay fair point!

It turns out that simply calling updateModel instead of update on blur doesn't cause a round trip - the view isn't updated. Which in the majority of cases is fine, but it means that input.value may be invalid. Instead we have to have something like updateModelThenView.

I'm in two minds about this. The fix is trivial enough, but something about it feels dangerous. Maybe it's because I'm part-way through watching a talk that claims two-way binding is evil because it destroys the notion of a single source of truth, and I sat there thinking 'but that's nonsense - all a two-way bound input does is request that the single source of truth be updated'.

In most cases Ractive isn't able to detect that DOM was changed programatically and it will change it only on next model update

I guess what I'm trying to say is that the timing of that model update should never be left to the user. If input.value is updated programmatically and the model isn't updated, that's a flaw in the program, not the library.

[MartinKolarik]

but it means that input.value may be invalid

It can be invalid only while the input is focused. On blur, model value would be replaced with the new (invalid) value, which would trigger the observer and possible validation. Since you said the validation should occur on blur, it seems correct to me.

that's a flaw in the program, not the library

Yes, but the library shouldn't try to "fix" it, if it's not possible in every case.

[jameshfisher]

The "decorator" approach is good enough for me, so I'm out -- I'll leave the philosophical issues to you guys. :-)

[Rich-Harris]

@MartinKolarik

It can be invalid only while the input is focused

Yeah, that's what I thought, but I tried replacing update with updateModel and the validated data wasn't reflected by input.value following the blur:

<input value='{{foo}}'>

ractive.observe('foo', function (foo) {
  this.set('foo', foo.toLowerCase());
});

input.value = 'FOO';

Focusing then blurring left input.value as 'FOO' even though ractive.get('foo') === 'foo'. As I see it there are three options:

• Ignore the problem - it's not Ractive's job to fix situations like this
• On blur, instead of just updating the model, add a new function that updates the model then updates input.value (updateModelThenView())
• An alternative we haven't previously discussed - update the model on focus, and leave blur behaviour as is

Which do you think is best?

@jameshfisher :-)

[MartinKolarik]

@Rich-Harris

Focusing then blurring left input.value as 'FOO' even though ractive.get('foo') === 'foo'

That's strange. Why wasn't the view updated when set was called?

[jameshfisher]

I have no particular suggestion but a small point which may have been overlooked: in the general case, validation is per-model, not per-field. This means the application must be able to impose a validation predicate over multiple fields. For example, that field A plus field B equals 8. In this example, say a user wishes to change the values from { A=3, B=5 } (a valid combination) to { A=4, B=4 } (another valid combination). Since there is no concept of a "transaction", the user must change field A and then field B, or vice versa. Either way, the view is necessarily invalid in the intermediate state, i.e. the blur event after the first field change. Therefore any solution which resets the field on blur when the model fails validation does not permit multi-field constraints.

This is in fact just a generalization of the craziness, already noted, which occurs when performing validation on every field change event. The craziness in that case arises from the fact that field values must be able to be in invalid states in order to move from one valid value to another (e.g., a rule which says that the length of a string must be an even number). The craziness in the case I described above is exactly analogous: the model must be able to be in invalid states in order to move from one valid model state to another valid model state.

To put it yet another way, the discussion in this thread treats a focused field as a "transaction", where the focus event begins the transaction and the blur event commits it. But transactions must be able to change as much as required by the specific application's business logic, and so these begin/commit transaction events must be defined by the application and not imposed by Ractive.

Hope this makes some sense.

[MartinKolarik]

@jameshfisher Yeah that makes sense, but Ractive's isn't a library for input validation. If you need just a simple validation, you can use Ractive's features (observers) to create some validation rules, if you need something more complex, you are free to use any other library/plugin intended for that purpose.

[Rich-Harris]

@MartinKolarik because of this line, which ensures that any changes initiated by an input (including indirectly, via an observer) don't affect that input. Otherwise every keypress that resulted in invalid input would result in input.value being changed, which is a UX disaster. The update function (defined here) which is fired on blur (and only on blur) deliberately circumvents the usual data-binding mechanism.

@jameshfisher Constraint programming is a fascinating topic that I don't really begin to understand. But like @MartinKolarik says the ractive.observe() hook basically gives you what you need to accomplish stuff like that. Here's a simple demo - this could probably be generalised a bit. It's a digression but if you have any real-world examples of Ractive apps where constraint programming is useful I'd be interested.

[MartinKolarik]

@Rich-Harris now I see. So the input's value is being validated on every keypress, but the result is not being reflected in the view, right? But when blur event is fired, the input is no longer active. I'm not sure where does this.active come from, but is should probably be false at the moment.

[Rich-Harris]

@MartinKolarik it's slightly circuitous, but this.active comes from here via here - updates are handled centrally by the runloop, which unsets this.active once changes have propagated through the system.

Now that I look at the code it seems the extra level of indirection is unnecessary (runloop.js is a module that's accumulated a bit of cruft). Could probably use a spring clean. But this.active should be true as long as updateModel is running, which is why just triggering updateModel on blur isn't a complete solution - we need to update input.value manually after the blur event.

[MartinKolarik]

@Rich-Harris Oh now I see.

An alternative we haven't previously discussed - update the model on focus, and leave blur behaviour as is.

There would still be a problem as the value can change when the input is focused. Though is really an edge case.

On blur, instead of just updating the model, add a new function that updates the model then updates input.value (updateModelThenView())

Guess this one seems best to me as we both expected it to work that way :)

[Rich-Harris]

Done!

[MartinKolarik]

:+1: