input checkbox checked attribute rendered incorrectly or cached incorrectly

MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications

https://mithril.js.org

MIT License

13.98k stars 926 forks source link

input checkbox checked attribute rendered incorrectly or cached incorrectly #691

Closed sixtram closed 8 years ago

sixtram commented 9 years ago

In the following example, a checkbox's checked attribute is binded to a boolean (isLightOn) and an onclick handler is attached to toggle this bit. We display this - isLightOn - value separately as well to make sure we bind the correct value to the checked attribute.

This works fine, clicking the checkbox on and off toggles the bit correctly.

Then we intoduce a bit to switch off the light and disable toggling. This correctly sets isLightOn to false and the checkbox checked attribute nicely follows this setting.

Now clicking the checkbox correctly handles to not switch the light flag on, at the same time the checkbox checked attribute is rendered incorrectly because the checkbox is marked as 'checked'.

The only way to correct the rendering in this case is to assign a new key to the same checkbox and then the redraw will be correct. Uncomment this in the toggleLightOnOff() to test.

<head>
    <title></title>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/mithril/0.2.0/mithril.js"></script>
</head>
<body>

<div id="content"></div>

<script>
var app = {
    model: function() {
        var self = this;
        self.key = 0;
        self.isTogglingEnabled = true;
        self.isLightOn = false;

        self.toggleLightOnOff = function() {
            if (self.isTogglingEnabled === true) {
                self.isLightOn = !self.isLightOn;
            }
            //self.key = self.key + 1; // enabling this line will solve the rendering issue!!!!!
        }

        self.disableToggling = function() {
            self.isTogglingEnabled = false;
            self.isLightOn = false;
        }
    },
    view: function(c) {
        return [
            m("div", [
                m("div", [
                    m("div", "1. toggle the checkbox and see how isLightOn changes."),
                    m("input", { key: c.model.key, type: "checkbox", onclick: c.model.toggleLightOnOff, checked: c.model.isLightOn }),
                    m("div", "isLightOn = " + c.model.isLightOn ),
                    m("hr"),
                    m("div", "2. disable toggling and see that checkbox checked state and the isLightOn is no longer in sync"),
                    m("div", "isTogglingEnabled = " + c.model.isTogglingEnabled),
                    m("input[type=button]", { value: "Disable Toggling", onclick: c.model.disableToggling }),
                ])
            ])
        ];
    },
    controller: function() {
        var self = this;
        window.model = self.model = new app.model();
    }
};

m.module(document.getElementById('content'), app);

</script>
</body>
</html>

pelonpelon commented 9 years ago

This is not really a Mithril issue. It's basic HTML and Javascript.

Try this http://jsbin.com/qayiyo/1/edit?js,output

Checked is a funny property. In HTML you don't set it to true or false: <input type=checkbox checked> It exists or it doesn't exist. Mithril puts HTML attributes inline -- checked=false is still "checked" from the HTML point of view.

In JS it's boulean: document.getElementsByTagName('input')[0].checked=false But in your example, if you set it in your toggleLightOnOff function, the HTML is redrawn later and "checked" would be added the input tag anyway.

I think you want to "disable" the checkbox. See the jsbin example.

sixtram commented 9 years ago

That was my first idea as well, but notice that if you enable the "self.key = self.key + 1;" line then the application behaves as expected. It happens because mithril recreates the whole DOM for that element.

In other words you can initially render a checkbox correctly both with checked=true and checked=false flags, however it seems that if you flip the bit during the event handler the redraw fails.

ArthurClemens commented 9 years ago

You should not use the checked attribute when the value is false. Instead build an options object:

var inputOpts = {
    key: c.model.key,
    type: "checkbox",
    onclick: c.model.toggleLightOnOff
};
if (c.model.isLightOn) {
    inputOpts.checked = true;
}
...
m("input", inputOpts)

http://jsbin.com/keqabe/edit?js,output

There is a feature request to remove nullified attributes: #679

sixtram commented 9 years ago

thanks 'pelonpelon' and 'ArthurClemens' for your responses, but the issue is not related to whether we use checked true or false. that part of mithril is correct.

the issue is, when on a checkbox click's handler you reverse the same bit then and only then it's not rendered correctly. so the initial render works fine both cases when isLighOn is true or false.

That's why I say that if we change the key property of the checkbox on every render and therefore we force a hard redraw then the app behaves correctly (self.key = self.key + 1 line)

so I still think it's a bug in mithril.

sixtram commented 9 years ago

My conclusion is that it's a bug or side effect how the virtual DOM compares the changes and updates the real DOM. The diffing algo is not covering all cases, and I just ran into one.

Here's what's happening:

var inputOpts = {
    type: "checkbox",
    onclick: c.model.nop(); // dummy function
};
if (c.model.isLightOn) {
    inputOpts.checked = true;
}

render a checkbox with a checked = true attribute
add a click handler to it, and within the click handler do nothing, so don't change the islighton flag
an automatic redraw is happening
because nothing changed in the model (we didn't change the binded checked variable), therefore the view() creates the same virtual DOM as in the first pass (in step 1)
because nothing changed in the virtual DOM mithril doesn't replace the checkbox
becuase the checkbox was not replaced the original browser's native onclick handler turned off the checked state even if in the virtual dom we set it to enabled!!!
that's why if we change the key prop in each render as mentioned above then mithril always rebuild the checkbox (remove old element and add new) therefore it renders correctly

pelonpelon commented 9 years ago

You are absolutely correct. The diff algorithm doesn't see the change and the virtual dom never overwrites the real DOM that you change when you check the box. Another option to ensure that the view is redrawn:

 self.toggleLightOnOff = function() {
            if (self.isTogglingEnabled === true) {
                self.isLightOn = !self.isLightOn;
            }else{
              m.redraw.strategy("all")
            }
            //self.key = self.key + 1; // enabling this line will solve the rendering issue!!!!!
        }

BTW: The checked=false issue came up months ago in the forum and I completely forgot the outcome, which is that checked=false in inline HTML does work (although the consensus is that it shouldn't).

pdfernhout commented 9 years ago

@sixtram I wonder if what you describe could be simmilar to issue #701 that I opened yesterday regarding textarea? Apparently, if you do not handle changes to a textarea by (somehow) updating the value in the textarea options (by responding to "oninput" or "onchange" events), Mithril seems to not think the textarea has changed despite user input. So, Mithril will then not update the textarea's HTML element if user code later tries to set the textarea to the same value that Mithril has cached (a value which is out of date relative to user-initiated changes).

With "input" elements, the "value" is retrieved by Mithril in setAttributes and compared to the cached value. In that issue, I echoed a suggestion by "ldimi" that Mithril check the textarea's value in the same way as for inputs. So, is looks like maybe Mithril should also examine the "checked" attribute of checkboxes as well in setAttributes?

Here is a rough first try to improve this (I have not tested this) which replace a couple lines near the end of setAttributes:

else if (attrName === "value" && (tag === "input" || tag === "textarea") && node.value != dataAttr) {
    node.value = dataAttr;
}
else if (attrName === "checked" && (tag === "input") && node.checked != dataAttr) {
    node.checked = dataAttr;
}

Perhaps the test there on the tag should be tightened up with a check on type="checkbox"?

Might there be other attributes where that matters as well? I can wonder what attributes other vdom systems check every time?

pdfernhout commented 9 years ago

@ArthurClemens I often set object keys to "undefined" instead of "null" in order to get JavaScript to ignore them in certain situations, which can be done by appending "|| undefined" to the end of some test. Since I'm using TypeScript with Mithril, a construction like the following, while perfectly valid JavaScript, would still typically produce a type error (without casting inputOpts to "any") as TypeScript does not like modifying defined objects:

if (c.model.isLightOn) {
    inputOpts.checked = true;
}

Here is an example from the code I am working on right now which sets the attributes of "readOnly" and "disabled" to undefined when they are not true:

var readOnly = fieldSpecification.displayReadOnly || (fieldSpecification.valueImmutable && value) || undefined;
var disabled = (readOnly && displayType === "select") || undefined;

var standardValueOptions = {
    value: value,
    id: getIdForText(fieldID),
    onchange: change,
    readOnly: readOnly,
    disabled: disabled
};

...
} else if (displayType === "text") {
    makeLabel();
    parts = [
        m("input[class=narrafirma-textbox]", standardValueOptions),
        m("br")
    ];
...

I think I've been getting way with setting "checked" to false with Mithril attributes, but I should doublecheck that.

Looking at that code reminds me that "selects" have a value too. I would suspect Mithril's vdom would also lose track of their state long with textareas and checkboxes?

So, regarding my previous comment, the improved test should probably be (maybe with a better test for "checked" that considers input type):

//#348 dataAttr may not be a string, so use loose comparison (double equal) instead of strict (triple equal)
else if (attrName === "value" && (tag === "input" || tag === "textarea" || tag === "select") && node.value != dataAttr) {
    node.value = dataAttr;
}
else if (attrName === "checked" && (tag === "input") && node.checked != dataAttr) {
    node.checked = dataAttr;
}

pdfernhout commented 9 years ago

I decided to check JQuery's implementation of "val" to see what needs to be set as a "value", and it turns out I forgot about radiobuttons also needing to be "checked". https://github.com/jquery/jquery/blob/a644101ed04d0beacea864ce805e0c4f86ba1cd1/src/attributes/val.js

So, radiobuttons should be added to that test as well as they likely also probably have this issue, same as checkboxes, textareas and likely selects. But since radiobuttons are a form of "input", the general test for "checked" for any input type in the code above would work for them too. If the test was made more specific (like jQuery does), the test should include both the checkbox input type and the radiobutton input type.

pdfernhout commented 9 years ago

Actually, since checked is a boolean, probably the related assignment part of that test should read:

node.checked = !!dataAttr;

pdfernhout commented 9 years ago

Maybe it is obvious to others, but I'm just realizing that if you modified other HTML element attributes like disabled or readonly via a browser's development tools, or via direct HTML manipulation in code (like with jQuery or such), that this same sort of issue could also appear. In such cases, Mithril's cache would be out-of-date and Mithril would not realize it. For example, if a developer disabled a component via a browser's developer tools to see what it would look like, I'd expect Mithril would never re-enable the component despite a true "enabled" flag passed in as an attribute to the "m" function. The component would likely stay disabled until that component was disabled and then re-enabled by code using Mithril, which would resync the cache. (I have not tested that though.)

However, those developer-caused cases presumably could be ignored in practice. We could assume anyone using jQuery and Mithril together would have to understand this and code for it. Still, I mention it because, for a developer, messing around with an HTML element's attributes during testing via browser developer tools might then lead to this sort of unexpected behavior, where Mithril does not update things you might think it should because Mithril's vdom cache is out-of-date. Perhaps this issue is already documented somewhere?

pdfernhout commented 9 years ago

The attributes that are important to check in setAttributes are presumably just attributes that an end-user would change in the normal course of interacting with elements on a web page, which are things like value and checked. I would hope the above may have them all covered, but it would still be good to find a definitive list of those. I've noticed a few more possible cases, mentioned below.

I'm wondering now if text selection might also be an example of this, and that "selectionStart" and "selectionEnd" should also be checked and assigned? The Mithril 0.2.0 code does not include either of those two strings, so I'd guess that is another example of this behavior? And "selectionDirection" may also be one? https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/setSelectionRange

Scrollbar position is likely another such case? The "scrollTop" attribute is probably another example of this. https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTop

The string "focus" is not in the Mithril 0.2.0 code either. Could that be another issue? Probably not, as focus seems mostly managed programmatically via things like focus(), blur(), and activeElement, and the documentation say to use "config", so this is probably not an issue.

The input:file element does not have a settable value. So, state could not be restored for a file input. But that could not manifest as this issue, which relates to ignoring setting state sometimes, not always.

I looked through some other HTML elements and did not notice any other missed cases. But maybe other people might think of some.

pdfernhout commented 9 years ago

It looks like the keygen element might potentially have this issue too? https://developer.mozilla.org/en-US/docs/Web/API/HTMLKeygenElement

I found that one looking here: https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Forms

pdfernhout commented 9 years ago

A "multiple" select's option element's "selected" property probably has this issue. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/option

pdfernhout commented 9 years ago

Here is an attempt at improving the test at the end of setAttributes for all the cases listed above (except keygen which I don't fully understand, and with the caveat I have not compiled or tested any of this):

//#348 dataAttr may not be a string, so use loose comparison (double equal) instead of strict (triple equal)
else if (attrName === "value" && (tag === "input" || tag === "textarea" || tag === "select") && node.value != dataAttr) {
    node.value = dataAttr;
}
else if (attrName === "checked" && (tag === "input") && node.checked != dataAttr) {
    // TODO: Maybe this case should be more selective to checkbox and radio?
    node.checked = !!dataAttr;
}
else if (attrName === "selected" && (tag === "option") && node.selected != dataAttr) {
    node.selected = !!dataAttr;
}
else if (attrName === "selectionStart" && (tag === "input" || tag === "textarea") && node.selectionStart != dataAttr) {
    node.selectionStart = dataAttr;
}
else if (attrName === "selectionEnd" && (tag === "input" || tag === "textarea") && node.selectionEnd != dataAttr) {
    node.selectionEnd = dataAttr;
}
else if (attrName === "selectionDirection" && (tag === "input" || tag === "textarea") && node.selectionDirection != dataAttr) {
    node.selectionDirection = dataAttr;
}
else if (attrName === "scrollTop" && node.scrollTop != dataAttr) {
    // TODO: Maybe this case should be more selective?
    node.scrollTop = dataAttr;
}
else if (tag === "keygen") {
    throw new Error("keygen support unfininshed");
}

pelonpelon commented 9 years ago

I wonder if you couldn't consolidate:

else if (attrName === "checked" || attrName === "selected") node[attrName] = !!dataAttr; 
else if (node[attrName] != dataAttr) node[attrName] = dataAttr;

Here's your textarea example from #701 with an added checkbox using an edited mithril.js. http://jsfiddle.net/pelonpelon/ev9y4dn4/2/ The checkbox is pre-checked and has no on-event so it won't reverse itself upon clicking it. But it does right itself on redraw.

The changes to mithril.js are at lines 426-430 in this modified file: http://jsbin.com/gaciwi/370/edit?js The only other changes to the file are console.logs to watch redraws.

I've made the irresponsible assumption that keygen works like a typical attribute.

pdfernhout commented 9 years ago

@pelonpelon That change clearly makes the test work, thanks. However, the change makes Mithril go from a very limited testing and setting of just "value" for just inputs (without my changes) to a much larger amount of testing and setting on many attributes. I wonder if possible concerns about that change might include performance, writing to readonly properties producing errors, and possible side-effects of accessing properties? Still, it might be good enough in practice depending on the rest of the system; I don't know. If it is not good enough, I outline another option below -- to create a table of functions that would do the testing and setting.

== More details

I'm not sure about whether you would want to consult another library's code for legal reasons, but in issue #69, there is a link to a version of React's HTMLDOMPropertyConfig.js which lists at the top the Apache license. I did look at that file myself. The files lists a lot of HTML properties, and whether they require accessing/setting via attribute, property, as boolean, as numeric, as positive numeric, and whether they have side effects. It seems to me that Mithril may need something similar.

Prior to looking at that code I had been working on an approach like the following to be more efficient than a series of ifs but with still assuming tags needed to be checked. I've included that below. This code would not work as is though because it does not handle cases where properties apply to all tags. If we could ignore the check on the specific tag, then this code could be simplified further, and essentially this table would probably begin to cover all the cases in React.

// Global table
var userModifiableAttributes = {
    value: {input: true, textarea: true, select: true},
    checked: {input: true},
    selected: {option: true},
    selectionStart: {input: true, textarea: true},
    selectionEnd: {input: true, textarea: true},
    selectionDirection: {input: true, textarea: true},
    scrollTop: "*" // ????
};

...

else {
    // TODO: Booleans get no special handling; no support for "*" attributes like scrollTop
    var userModifiableAttribute = userModifiableAttributes[attrName];
    if (userModifiableAttribute && userModifiableAttribute[tag]) {
        node[attrName] = dataAttr;
    }
}

React uses a set of bits to reflect everything about an attribute. Given that I doubt there are many combinations of attribute type (a guess?), as a simpler alternative instead perhaps we could use a more function-based approach with the table above. So, something like this, changing the table name to be more general (but the table name probably could be better):

var attributesTable = {
    value: setStringAttribute,
    checked: setBooleanAttribute,
    selected: setBooleanAttribute,
    selectionStart: setNumericalAttribute,
    selectionEnd: setNumericalAttribute,
    selectionDirection: setLimitedStringAttribute.bind(null, ["forward","backward","none"]),
    scrollTop: setNumericalAttribute
};

Where these functions were called like so:

var attributeFunction = attributesTable[attrName];
if (attributeFunction) attributeFunction(node, attrName, dataAttr, tag);

And they could be implemented like so, as an example for booleans:

function setBooleanAttribute(node, attrName, dataAttr, tag) {
    var dataValue = !!dataAttr;
    if (node[attrName] !== dataValue) node[attrName] = dataValue;
}

Properties with limited choices like selectionDirection could have extra checking and error logging. In the example above, I configured a "setLimitedStringAttribute" using bind, but there could be more general constructor functions used if needed.

I'm not sure if the tag needs to be included as an argument. However, I'd expect they might be needed to be checked in a few special cases?

We'd have to profile the code to see if it was a lot slower with the extra function call, but probably this function-based approach could be the core of a reasonably fast correct maintainable implementation. Since this is in the "inner loop" of the diff/build algorithm, performance matters here to some extent. But "get it right, then get it fast".

A major reasons I picked Mithril is not liking React/Facebook's overly broad patent clause (although it was not the only reason, as I like Mithril's elegance). So, rather than look at the React code, a better source for info on all these properties might be this Mozilla page and similar (since that page does not cover everything): https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement

Although, I can still wonder how Matt-Esch's (MIT-licensed) virtual-dom handles this issue? I looked briefly at that code but could not find an equivalent to a properties file. https://github.com/Matt-Esch/virtual-dom

If we had such testing/setting functions, we could use them in the body of setAttributes perhaps so acceptable values were checked or warned about (at least in some debug mode) or converted as needed and so on. Again, there might be performance implications though.

All that said, if your change was good enough, then it is simpler to understand and faster, and so generally better. But I remain concerned about side-effects and such. Still, every library has its limits and assumptions, and users need to learn them. So, given the current Mithril 0.2.0 works fairly well in many cases, perhaps a simpler approach to all this is good enough and uses will just need to learn to work within some limitations for other benefits? It also might be possible to add this extra dispatching as a configurable option perhaps if it were to slow things down significantly.

pdfernhout commented 9 years ago

I've created another test using "scrollTop" this time, based on the textarea test. As expected, the test does show the incorrect caching issue with Mithril 0.2.0. If you scroll a textarea which had its scrollTop initially set to 0 and then and try to reset the scrollTop to 0, the scrollbar position (incorrectly) remains unchanged. The scrollbar position can still be changed to other values though which work as expected (as they are different from the cached value). Here is the fiddle: http://jsfiddle.net/pdfernhout/ggjgdd04/1/

When I changed that test to use the modified version of Mithril from @pelonpelon, the test works correctly: http://jsfiddle.net/pdfernhout/ggjgdd04/2/

To understand limitations of pelonpelon's possible fix better, it would be good to have a test case using a property with side effects or one that is read-only, where the Mithril 0.2.0 code works better somehow than the "fix". But as I think about making such a test, you could not expect setting a readonly property to work at all. So, maybe side-effects of access are all that might be an issue? And even then, you probably should know about that if you are changing the property? And maybe then you should use "config" for those properties? Also, maybe the library should just assume users have converted their data as needed, or that the DOM code will do it for them or just generate an error, like passing a non-numeric value to scrollTop? Even the "!!" conversion I added for booleans might not be needed if the DOM is going to do that internally (I'm not sure what will happen otherwise).

While it's hard for me to accept, there can often be a big win for "worse is better" in terms of a library internals' understandability and the library's adoption. User priorities can determine what edge cases are worth fixing. Is this situation possibly one of those cases where the difficulty of all the edge cases can be pushed back onto the user, compared to the more complex approach I outlined with table of functions? It seems to be mostly working already, given how long it took this edge case to show up in practice (where you set a property but don't track it).

From the last link here: https://en.wikipedia.org/wiki/Worse_is_better http://c2.com/cgi/wiki?WorseIsBetter http://www.jwz.org/doc/worse-is-better.html "The MIT [Lisp] guy did not see any code that handled this case and asked the New Jersey [Unix/C] guy how the problem was handled. The New Jersey guy said that the Unix folks were aware of the problem, but the solution was for the system routine to always finish, but sometimes an error code would be returned that signaled that the system routine had failed to complete its action. A correct user program, then, had to check the error code to determine whether to simply try the system routine again. The MIT guy did not like this solution because it was not the right thing. The New Jersey guy said that the Unix solution was right because the design philosophy of Unix was simplicity and that the right thing was too complex. Besides, programmers could easily insert this extra test and loop. The MIT guy pointed out that the implementation was simple but the interface to the functionality was complex. The New Jersey guy said that the right tradeoff has been selected in Unix-namely, implementation simplicity was more important than interface simplicity. The MIT guy then muttered that sometimes it takes a tough man to make a tender chicken, but the New Jersey guy didn't understand (I'm not sure I do either). "

Now that we are "aware" of this issue, perhaps the simple fix is good-enough and the documentation could include warnings about any edge cases that must be handled by the user (like with "config")? And noting that these edge cases only arise when you set a user-changeable attribute (like scrollTop) without tracking changes on it?

I just modified the scrollTop test to track changes using Mithril 0.2.0 (without the fix) and then works as expected: http://jsfiddle.net/pdfernhout/ggjgdd04/3/

However, in that case, a big downside is that you get a redraw for every scroll event. I added a redraw counter to show that.

An alternative in that case could be just setting the scrollTop outside of Mithril, after retrieving the existing element by its ID. So again, maybe dealing with the issue could be pushed back on the user?

The main thing about this issue may be more that it was surprising and worrying, rather than that it could not be dealt with somehow. In my original case (issue #701) it was just an issue I noticed while working towards code that would indeed track all changes to a value in a textarea or otherwise disable the textarea. For this overall issue, practically speaking, if you are going to put up a checkbox that is not disabled, you probably are also going to want to respond to the user checking it. If you don't track the changes, what is the point of putting up the live component? Unless, perhaps you plan on retrieving the value directly at some latter point, but then why bypass the Mithril approach and add in extra DOM access later (ala typical jQuery patterns) which makes your app more confusing and brittle (inclusing needing to invent unique IDs for such components)?

Maybe I'm coming around to thinking this issue may not need to be "fixed" at all (especially if it involves significant complexity or performance issues)? Although if so, the issue perhaps should be better to documented -- assuming it is not already documented somewhere?

Here is a first cut at just documenting this issue in a couple sentences: "If you set an attribute the end user can change by interacting with the application (like scrollTop, checked, or value), and if you also intend to reset the initial value later, then you need to track changes to that attribute via an on-* method to keep the Mithril cache in sync with the actual DOM Element's changing attribute. Otherwise, updates to the attribute from the application later may not always be propagated to the DOM Element due to Mithril's caching."

Still, I can hope pelonpelon's fix is good enough because is will reduce the amount of surprise for developers learning Mithril.

pelonpelon commented 9 years ago

@pdfernhout I've written the following test which input passes before and after the proposed changes and textarea passes only after the changes. If you have the time, I would like your opinion on the efficacy of the test before I write tests for all the attributes you've outlined above.

test(function() {
// always recreate elements if DOM node attributes differ from data node attrs
    var root = mock.document.createElement("div")
    var component = {
      controller: function(){
        this.textValue = ""
      },
      view: function(ctrl){
        return m("textarea", {
          value: ctrl.textValue,
          onclick: function() { ctrl.textValue = "" }
        })
      }
    }
    m.mount(root, component)
    mock.requestAnimationFrame.$resolve()

    root.childNodes[0].value = "new textValue"

    root.childNodes[0].onclick( {} )

    m.redraw()
    mock.requestAnimationFrame.$resolve()

    return root.childNodes[0].value === ""
})

pdfernhout commented 9 years ago

@pelonpelon I don't know enough right now about the Mithril tests to have much of an opinion on what you supplied, sorry. That will be a new learning curve when I have time.

As an update, I've been running with the last fix I outlined (with all the if/elses), and I have noticed that this part of the fix seems to break "select" elements in my app:

 else if (attrName === "selected" && (tag === "option") && node.selected != dataAttr) {
    node.selected = !!dataAttr;
}

In the app, I set both the select's value and I set "selected" to true on the option for a select. With that "fix" in place, I'm seeing (in FireFox Developer 40.0a2) that selects appear correctly the first item they are drawn with the correct selected options (usually, this may be inconsistent though as sometimes the value seems not set). But after a redraw, all the selects lose their selections. Trying to change the selection does not accomplish anything (it goes back to the unselected state). When I remove the fix, things start working OK again. I'm not sure if it is the "fix" as regards select options or how my code interacts with it, the code having been designed without the fix. Since selects support (in JavaScript) using both select value and a option with "selected", maybe there is some weird interaction? When I try an older version of Safari (5.1) I see pretty much the same issue (sometimes the selects don't even display with the correct value the first time, and they ignore any changes to them). So, at least for selects, this "fix" may not work as expected. I just tried your simpler version, and it has the same issue.

Until that select issue is resolved or better understood, I think I'm going to back out the entire change and just ensure any user-modifiable value I set via Mithril is being tracked via on-* methods. It's OK with me if Mithril has limitations in how it can be used successfully (everything does). I just want to understand those limitations so I can work within them (or around them) and that I know any surprises are not the tip of the iceberg of some larger issue. After looking at this issue, it seems contained within these bounds (setting a user-modifiable value but not tracking it). In practice, those boundaries are not that hard to work within -- even if they can be surprising when developing an app step-by-step (like when just displaying a value using a checkbox or such and not yet tracking changes).

pelonpelon commented 9 years ago

@pdfernhout I have been re-thinking our entire premise and go back and forth as to whether Mithril should be responsible for keeping real DOM attributes in sync with vDom attributes. The argument in favor is that, _at the developer level_, it is consistent with other aspects of the diffing algorithm that a change in state should be reflected when views are rendered. I suspect most developers would assume that behavior. The argument against is that, within the scope of core Mithril logic, only the vDOM in its cached state and its "new" state is of concern to mithril.js, and syncing real DOM changes with the vDOM should be the responsibility of the developer.

But, whichever approach one favors, it seems that the exception made for the input element in mithril.js code is an inconsistency. I believe it ought to be all or nothing.

casajarm commented 9 years ago

Noob here. What if this synching was an option within the view?

Greg

On Jul 6, 2015, at 8:25 AM, pelonpelon notifications@github.com wrote:

@pdfernhout I have been re-thinking our entire premise and go back and forth as to whether Mithril should be responsible for keeping real DOM attributes in sync with vDom attributes. The argument in favor is that, at the developer level, it is consistent with other aspects of the diffing algorithm that a change in state should be reflected when views are rendered. I suspect most developers would assume that behavior. The argument against is that, within the scope of core Mithril logic, only the vDOM in its cached state and its "new" state is of concern to mithril.js, and syncing real DOM changes with the vDOM should be the responsibility of the developer.

But, whichever approach one favors, it seems that the exception made for the input element in mithril.js code is an inconsistency. I believe it ought to be all or nothing.

— Reply to this email directly or view it on GitHub.

pelonpelon commented 9 years ago

@casajarm Currently, there is no way to tell Mithril "I want this element/node to be rendered no matter what", without setting m.redraw.strategy to "all" -- which is not always a bad option, but not optimal.

pdfernhout commented 9 years ago

@pelonpelon On "all or nothing", I was thinking the same thing myself this morning.

If this is an issue developers will just have to understand, it may actually be more confusing in some ways that value for input in handled automatically and the rest is not. I first bumped into this with a textarea, and then I check to find that a text input worked differently than a textarea, and that inconsistency just felt like a bug, and so I reported it (issue #701). If text input had worked the same as textarea, I might have been more likely to see that behavior as a "feature" I did not understand and gone searching through the documentation to understand it. So, I agree it might be best, assuming that was the choice, for there to be no handling for any user changes to real DOM elements, which would mean the current clause to fetch an input's value near the end of setAttributes should be removed. Developers would bump into the issue quickly, but then they would learn about it, put in place onchange etc. calls if they wanted it to work as expected, and move on. And it could be well documented as a "feature" that might affect incremental development (where on-* methods were added later).

That said, if it was easy to make the update from user changes reflect back to the vdom as expected, and there was little performance impact, then I am all for it -- mainly because it is one less surprise for a developer when doing incremental development. But the side-effects I saw with the select (if that was the root cause of the issue, and not some other weird interaction with my app) suggest that doing that backwards reflection 100% consistently may be some effort (at the very least, in testing). Doing it 100% may well require some large special case system for each property and/or tag, such as React has (mentioned above). And then the the issue is, is it worth the time and complexity compared to focusing on some other aspect of Mithril that might be a higher priority (like perhaps the unmounting issue and/or selective redrawing or whatever for example as mentioned in issue #694, although for that I have a workaround too which is just mounting everything at the start -- but there the docs seem to disagree with the behavior).

Of course, removing the input value setting code would change current behavior and so break backward compatibility. But would that really affect any working apps in practice? I guess maybe that is the big issue -- is there any realistic case where you would want to set the value on an attribute but not track it? I guess the only one I can really think of is something like setting the scrollTop or selection position -- but in those cases, it could be done by using DOM methods directly. And also, when you think about it (as we are now), it does not really make conceptual sense to think you can set the vdom to a scrollTop of 0 on every redraw but then have the scrollers really work otherwise OK (given redraws could in theory happen at any time). So, that would need to be another part of the documentation. If you want to set those sorts of values, you need to do it outside of setting vdom attributes by changing the DOM yourself.

BTW, React has the notion of "controlled" component, which overlaps with this issue: https://facebook.github.io/react/docs/forms.html "An "input" with value set is a controlled component. In a controlled "input", the value of the rendered element will always reflect the value prop. ... Any user input will have no effect on the rendered element because React has declared the value to be Hello!. If you wanted to update the value in response to user input, you could use the onChange event: ... An "input" that does not supply a value (or sets it to null) is an uncontrolled component. In an uncontrolled "input", the value of the rendered element will reflect the user's input. ...". [I replaced the angle brackets around input with quotes because they were not showing up in the preview.]

I'm not saying React handles this identically -- there is a mention of "defaultValue" and similar for defaultChecked. I'm just saying React had documentation that relates to the issue of whether you expect an input to have a onchanged message or similar, going to great length to invent terminology and explain it.

pelonpelon commented 9 years ago

@pdfernhout I was reading about controlled components over the weekend. I've built apps with React but never had the stomach to delve into the source. Controlled components from my bird's-eye view seems like just the kind of complexity and bloat that I think Mithril means to avoid. Mithril is not a handholding framework and I think many of us early adopters like it specifically for that reason. Now that there is more popular interest, many devs coming from React, Angular, Backbone etc, are looking for formulas and plug-and-play. If you read the Mithril Gitter chat, you'll notice that a large amount of confusion on the part of new users is a lack of understanding of javascript. I think most of us can be tripped up by plain old javascript but these newbies have come to expect the frameworks (or jQuery) to protect them from any ugliness.

I am still in the all-or-nothing camp, yet I am unsure which is the road best taken. I'm leaning towards expected behavior over internal consistency. This is a very long discussion, I hope Leo chimes in at some point. He may have come down on this one way or another ages ago. @lhorie are you listening?

sixtram commented 9 years ago

one solution to these kind of problems would be a non-breaking api change, where the view() function can return a special attribute in the vdom element to force redraw or do a cache invalidation of that element. it's basically same as redraw strategy but implemented as an attribote of the vdom element, and available in the view() function.

return m("input", { forceDOMUpdate : true }); // would be cool if supported :)

pelonpelon commented 9 years ago

There is more than one issue previously submitted suggesting the same thing. I'm sorry I can't remember which ones.

barneycarroll commented 9 years ago

@sixtram you could achieve this with key and an incrementer function:

var unique = ( function closure( key ){
    return function increment(){
        key++

        return key
    }
}( 0 ) )

// later…

m( 'input', {
    key : unique()
} )

This would ensure each draw consider the element as new and create it from scratch.

@pelonpelon I agree the React approach you mention is way too heavy handed for this kind of thing. Cito — another virtual DOM library inspired by Mithril — has an interesting approach to this. Whereas it's normal DOM diff/patch algorithm is extremely lightweight, it makes an exception for form inputs and always reads from the live DOM to perform diffs: https://github.com/joelrich/citojs/blob/master/README.md#input-elements

pelonpelon commented 9 years ago

@barneycarroll I think @pdfernhout and I agree that there is more than one uncomfortable solution, incremental keys might be the best, yet least obvious to someone not steeped in Mithril internals. In the end, my greatest concern is the inconsistent handling of input within mithril.js. If keys is the canonical solution, input should behave like all other user input.

I read about Cito when @lhorie mentioned it in the Gitter and I was especially drawn to the handling of selective rendering and inputs. Very elegant with exciting potential. I'm curious to see where Leo's green field experiments are headed.

lhorie commented 9 years ago

FYI, key incrementing is an anti-pattern. I don't recommend doing it. I'd rather make a change in core to read from the live DOM.

barneycarroll commented 9 years ago

key incrementing is an anti-pattern

You can drop 'incrementing' from that ;) – but srsly the key approach is a bad idea, it'd play havok on continuous user input elements, for instance

pelonpelon commented 9 years ago

@lhorie :+1: Yes, please. I think the magic resulting from the diff paradigm (never touch the DOM) has made some people giddy. Me included. config is a main topic of conversation in Gitter because we want to force everything into a closed MVC-esque system and realize we can't. The user is a messy outside force. Reading from the live DOM is intuitive and will allow for predictable responses to unpredictable input.

pdfernhout commented 9 years ago

How should the Mithril vdom interact with end-user-modifiable properties? Here are five options in an attempt to summarize the design space. A commonality of all these options is that any user-modifiable DOM element property that is not set in the vdom will be left however the user set it -- unless the component is replaced, like with a redraw strategy of "all" or a key change.

DOM reflects vdom 100% only on redraw: Mithril should set the property's value initially and then reset the property on every redraw if needed. So, if application code sets a texarea's value, whatever the user types into a textarea will be replaced on every redraw. If setting the scrollTop property, whatever scrolling the user did should be reset on each redraw. A checkbox with checked set will be reset to whatever the vdom has as the checked state. To do this, Mithril would need to inspect actual DOM element properties on every redraw for any vdom property that was set by the application developer and could be changed by the end user (such as value, scrollTop, checked, selectionStart, etc.). Mithril works this way to a very small extent, as currently, Mithril 0.2.0 does this sort of redraw update for input value (but only for that). Application code is expected to have onchange methods (or other on-* methods) to update the vdom for any user-modifiable properties which the application code changes so that the GUI will appear to change over time. Essentially, this onchange etc. code makes a tight loop that changes the GUI step-by-step for each tiny change to any user-modifiable property that is being set just as each redraw is triggered automatically (like tracking a change each small change to scrollTop and updating it in each redraw). Some code I included in previous comments was essentially an attempt to get this option to work consistently for all user-modifiable properties for what I initially thought was a buggy/incomplete setAttributes method, but that does not necessarily mean I feel now that this is the best option. Also, the code I put together seemed to cause select elements to malfunction, as the "selected" property and "value" property seem to interact (at least in my application code). This option may be (arguably) what most developers might expect Mithril's diff behavior should be (assuming the select issue or any others can be resolved) -- especially in the case where the is no onchange/on-* method to reflect a user-modifiable property's change back to the vdom. The reason this approach may match (initial) developer expectations is that a developer can see that whatever property was set in the vdom in a view method is always propagated to the real DOM elements on every redraw. Despite any increasing library complexity from supporting this option, this approach may make for conceptual simplicity and understandability of the redraw/diffing API (even if it might make the actual library code for redraw/diffing harder to understand or implement or test).
DOM reflects vdom 100% always (with special onchange handling): Mithril should set the property's value initially and then actively counter any user effort to change the property. So, if the user types one character into a textarea with its value set, the text area would be reset to the vdom's cached value. If the user attempts to scroll when scrollTop is set, Mithril will put the old scroll position back immediately. If the user clicks on a checkbox to change it, Mithril will put the vdom value back immediately. Without advocating for this option, and ignoring the work involved to do this, in some ways this is the most consistent option to me relative to what is means to set a vdom property (i.e. that's the way the application developer want the property to be). However, it also was still not what I initially expected, given the emphasis everywhere else in the documentation on Mithril having a "redraw" process where you expect the DOM to be changed as needed. This approach might be incompatible with using an onchange handler as opposed to an oninput handler in a textarea or text input, because the vdom might never let character changes accumulate in text (since it is actively resisting them). There might need to be special handling when an onchange callback (or other on-* callback) was set to allow that specific property to be varied by the user. This extra special case handling for on-* methods might make such an API harder to understand conceptually -- since Mithril would reliably be changing DOM properties (back to vdom values) except when you had an onchange (etc.) handler. It seems React has special "defaultValue" and "defaultChecked" vdom attributes for "controlled" components that may help address this potential confusion? Mithril might need something similar if pursuing this option.
DOM element is disabled/readOnly when user-modifiable property is set (with special onchange handling): As a variant of the above (mostly as a strawman), Mithril should disable and/or make readOnly DOM elements like inputs, selects, textareas, and scrolling divs without on-* methods that track the change. The intent would be to prevent the user form changing properties on the DOM element. I don't think this is feasible with standard HTML elements because the developer might track only some properties of an element. So how do you get a working scroller but disabled text? Or, what if the user set the scrollTop of a textarea but tracked the value? So, it might be impossible to make disabling/readOnly work consistently and completely just with the DOM (although it could in theory be possible, if no-doubt out of scope for Mithril, with a library like Zebra that draws every widget on an HTML canvas http://www.zebkit.com/ and which comes with its own potential issues like accessibility and screen readers). Such a change would also would broaden the complexity of the Mithril interpretation to be changing property values (like readOnly) not directly specified by the application developer (if, say, just value is set). That said, in practice, when a component is only for display and you don't want the user to change it, disabling the input component or making it readOnly after setting the value is probably a common design pattern at the application level. So, this all is not as far fetched as it sounds, even if, say, the choices with HTML remain limited (like if you disable a text input, you can no longer select the text typically which is unfortunate). This also may not be a great user interface choice as well, compared to displaying unchangeable data as plain (non-form / non-input) HTML, which otherwise can still be selected and scrolled.
DOM updated only when vdom is changed: Mithril should just set the property's value initially based on what is in the vdom, and then Mithril should not change the corresponding DOM element's property's value unless the vdom value (the "cached" value) is explicitly changed by application code from what it was before. So, all user changes will always be left as is until the vdom's internal property value is changed by code, and then (and only then, apart from replacing the component entirely with redraw "all" or a key change) the user change will be overwritten if it differs from the new value. If the vdom attribute is "changed" to be the same value it already has in the cache (like by setting an attribute's value in a view method just to the same value as last time view was called), Mithril should do nothing to change the DOM element's property, even if the DOM element's property differs from the vdom's cached value. In practice, finished application GUIs tend to have an onchange (or whatever on-* handler) handler that tracks user changes for properties of interest that the application itself changes. So, even without the Mithril library's diff algorithm reading user-modifiable properties itself, interfaces in practice will tend to work as expected once that on-* handler is in place because the vdom's cached value for an application-set property will reflect the user's change as a side-effect of the onchanged/on-* method changing the application model's underlying value to reflect the GUI's state. Applications being build incrementally may experience some unexpected behavior (like documented in this issue or issue #701). This approach is almost what Mithril 0.2.0 currently does (except for special handling of an input's "value" property as mentioned in the first option). While this behavior is consistent (ignoring the special case for input value in the current 0.2.0 implementation), the behavior (arguably) does not seem to be what developers initially expect, and so it requires extra documentation. The primary violation of an developers expectations with this option is that a view method may seem to set a DOM element's end-user-modifiable "value" property (or "checked" or "scrollTop" or whatever) but the DOM element does not usually change due to Mithril's vdom caching. Worse from a consistency standpoint, a DOM property sometimes is changed -- but only if the applications underlying model value changed for other reasons and so the vdom then acts on the change. However, every library has a learning curve, and expectations can change, so the violation of expectations may not be enough, by itself, to argue against this option given other benefits (like the library code remaining simpler and more understandable from a maintenance perspective). A developer also have to understand that for some specific changes to user-modifiable DOM properties like scrollTop or selectionStart (changes that are intended to change GUI state transiently like to set the scroll bar to the top or to clear a selection), those changes need to be done by direct DOM manipulation after getting a specific component by id or otherwise by using "config".
DOM updated only when vdom is changed except for input value which always reflects vdom after a redraw: Mithril should do what it does now (for 0.2.0), as a combination of the first option (just for input value) and the last option (for everything else). It's inconsistent, but users can learn about this inconsistency if they run into it and then move forward to write beautiful and elegant applications anyway. The advantage of the current approach is that the most common case of an input value is handled as developers might expect when developing incrementally (option 1) without the performance or potential risky side-effects (like when setting selected) or extra work and testing of handling all cases for end-user-modifiable DOM element properties. It might be interesting to know what commit introduced the backwards reflection of an input's "value" and if that commit was associated with any specific reported issue.

Is option 2 what React means by a "controlled" component (at least as far as the "value" property and maybe some others)? What does React do in the general case like for scrollTop or selectionStart? How do Mercury/vdom or other "diff paradigm" libraries approach this issue?

Did I miss any obvious cases? Are we now discussing choosing between option 1 and option 4? If so, that seems like a choice between emphasizing whether the Mithril library's API is (arguably) easier for application developers to understand based on initial expectations (as indicated by these issues being raised) -- or whether the Mithril library's code is easier to maintain (from less side effects and special cases), is easier to trace through, is easier to test, is smaller, and is perhaps is a bit faster.

That is a tough choice. And that helps explain how we ended up with the current Mithril approach of option 5 as a rough compromise. Could creative coding perhaps help us have both benefits (API meeting naive expectations and implementation elegance) without as much of a tradeoff -- assuming option 1 was most desirable from an API standpoint? If not, option 4 (choosing "worse is better" by pushing some effort back on the library user) still has a lot going for it.

angloc commented 8 years ago

Arguably this is more an HTML issue than a Mithril one.

This is what W3C say:

"A number of attributes in HTML5 are boolean attributes . The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value."

A number of libraries take the view that this is misconceived, and we would be better off pretending that the attributes are always present with a boolean value. While personally I agree W3C got it wrong, I feel it would be against the spirit of Mithril to conceal this. I like Mithril because for me having at least 3 different syntax/semantic combinations to deal with (HTML, CSS, Javascript, and that's just at the client end) is bad enough without opinionated libraries on top, and Mithril is a very thing layer in this respect..

So imo there is no requirement that Mithril should intervene behind the scenes to "correct" the HTML semantics. It is up to us to only include the boolean attributes in our views when they are needed.

The Mithril m functions builds the element attributes from a dictionary. In the case of an input check-box, it should only add the checked attribute if that is the intended state of the view model.

This JSFiddle shows this approach https://jsfiddle.net/angloc/m7hL2pnp/

The checked attribute is only added to the view if needed. Mithril doesn't need to understand anything about it. The view author needs to know and apply HTML semantics, that's all.

Some helper function for constructing the attribute dictionary functionally would be a good idea, but isn't essential.

angloc commented 8 years ago

Please note my comment addresses the original question only!

dead-claudia commented 8 years ago

By the way, I'm closing this as a dupe of #679, which deals with boolean attributes more generally (where this deals with inputs). Please continue all discussion there. Thank you. :smile:

masaeedu commented 8 years ago

@isiahmeadows It doesn't look like the discussion there is relevant. It doesn't matter whether you use true/false or null/not null. This issue is about changes to DOM elements which do not occur via Mithril's model (e.g. via user input). If the model hasn't changed, Mithril will simply ignore any changes that have happened to the element, which is not a reasonable default.

E.g. if you have a checkbox's checked attribute bound to some boolean value, and the user clicks the checkbox but (for whatever reason) no change in state occurs, Mithril will not reset the checkbox to an unchecked state. You can work around this by creating a new key for every render, but others have suggested the default behavior for the "input" family of elements be that properties like checked/value etc. are always read from the DOM and trump the cache if changed.

leeoniya commented 8 years ago

@masaeedu

I'll chime in here and mention that I have recently reached the same conclusion after addressing this exact situation in domvm. Any dynamic attributes/properties that are defined in the vtree templates must be re-synced back to the DOM regardless of unsynced user interaction because the contract is, "the DOM will always be consistent with the vtree definition after any redraw". Since initial rendering of these attributes affects the properties as well, it follows that all future redraws will also keep them in sync. It took some time for this to become clear to me. https://github.com/leeoniya/domvm/pull/18 helped convince me that it was the correct behavior to adhere to.

masaeedu commented 8 years ago

@leeoniya I can't really contribute much to a discussion of what should be done; I was just pointing out this issue was closed as a result of a misunderstanding. I'm suffering from the same problem with https://github.com/Matt-Esch/virtual-dom at the moment, which is how I found this issue. :cry:

Have you already implemented the approach you're describing in https://github.com/leeoniya/domvm?

dead-claudia commented 8 years ago

@leeoniya @masaeedu

Could someone give me a JSFiddle/CodePen/etc. to see a more detailed description of the issue at hand? The impression I got from this is that it's something resulting from a fundamental flaw in several vdom libraries in probably over-simplified element handling.

leeoniya commented 8 years ago

@masaeedu

yes. it means that you're responsible for binding onchange handlers and ensuring your "checked" prop (for example) is synced back into the model before any followup redraws regenerate the template and patch the DOM, otherwise it will become unchecked! domvm does not have automatic redraw on bound handlers, so you can sync without invoking redraw to avoid a perf hit or infinite loop.

@isiahmeadows

if you look at the test that's in that PR, it's a concise demonstration of the issue:

https://github.com/leeoniya/domvm/pull/18/files#diff-38dcab2adf8a7e3431829d8daca050fcR745

basically if you render an initially "checked" input (with no event handlers), then user clicks it to uncheck it. then redraw is invoked manually and nothing would happen in the dom. so the vtree is essentially out of sync with the dom after redraw. after the patch, the "checked" state would get reverted to match the vtree.

masaeedu commented 8 years ago

@leeoniya That makes sense. Is this special-cased for certain types of elements and attributes or have you abandoned the cache entirely in favor of "redraw when prudent"?

leeoniya commented 8 years ago

@masaeedu

There are a couple changes that went into the final patch. But yes, any props (explicit via {".someProp": 123}.. or implicit via isDynProp(attr) [1]) are always re-set on the DOM elements on every redraw. There is no perf hit to speak of cause prop setting is incredibly cheap and actually quite rare.

[1] https://github.com/leeoniya/domvm/blob/master/src/util.js#L73

dead-claudia commented 8 years ago

Yeah...it's the attribute caching that's the problem. I took one of the older fiddles from this issue and make a pen out of it, using a textarea instead. It's really one and the same in that it's the attribute caching that's getting in the way. And talk about a lot of changes needed...I'm pretty familiar with Mithril's code base, but I'm not quite familiar enough with the rendering and patching aspect of it to be comfortable with making that kind of refactor.

Or in other words, it would require an almost-rewrite of the renderer to fix. :frowning:

barneycarroll commented 8 years ago

I just came up against this bug again — I'm shocked it's been open all this time.

Why is this labelled 'enhancement' and 'breaking change proposal'? It's a painstaking report of bug in the virtual DOM implementation — there is no proposal to break anything.

barneycarroll commented 8 years ago

Interesting comparison: here's a working Mithril checkbox that doesn't exhibit this bug (in the top right hand corner); and here's a later version of the same codebase, where the bug is apparent (open the settings modal in the top right to reveal the checkbox).

EDIT: fixed link

dead-claudia commented 8 years ago

@barneycarroll Compare {checked: "false"} (HTML spec's version, JS value stringified) vs {checked: false}. Also, I marked it with those labels three months ago, and the issue has since changed somewhat in scope, but nobody updated the labels yet.

dead-claudia commented 8 years ago

It's still a proposal, since it's requesting a change to things, but it's not very breaking, since the breakage it could cause is trivial to fix. Also, I forgot to remove the duplicate label when I re-opened it.

lhorie commented 8 years ago

IIRC the snippet in db17958f0b96204ee8d97c882323d93c32564be2 used to exist to deal w/ this issue