input checkbox checked attribute rendered incorrectly or cached incorrectly #691
Comments
|
This is not really a Mithril issue. It's basic HTML and Javascript. Try this Checked is a funny property. In JS it's boulean: I think you want to "disable" the checkbox. See the jsbin example. |
|
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. |
|
You should not use the http://jsbin.com/keqabe/edit?js,output There is a feature request to remove nullified attributes: #679 |
|
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. |
|
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:
|
|
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 |
|
@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: 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? |
|
@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: 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: 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): |
|
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". 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. |
|
Actually, since checked is a boolean, probably the related assignment part of that test should read: |
|
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? |
|
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? Scrollbar position is likely another such case? The "scrollTop" attribute is probably another example of this. 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. |
|
It looks like the keygen element might potentially have this issue too? I found that one looking here: |
|
A "multiple" select's option element's "selected" property probably has this issue. |
|
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): |
|
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. The changes to mithril.js are at lines 426-430 in this modified file: I've made the irresponsible assumption that keygen works like a typical attribute. |
|
@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. 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): Where these functions were called like so: And they could be implemented like so, as an example for booleans: 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): 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. 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. |
|
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: When I changed that test to use the modified version of Mithril from @pelonpelon, the test works correctly: 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: 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: 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: Still, I can hope pelonpelon's fix is good enough because is will reduce the amount of surprise for developers learning Mithril. |
|
@pdfernhout 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 === ""
}) |
|
@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: 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). |
|
@pdfernhout But, whichever approach one favors, it seems that the exception made for the |
|
Noob here. What if this synching was an option within the view? Greg
|
|
@casajarm |
|
@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: 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. |
|
@pdfernhout 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? |
|
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. |
|
There is more than one issue previously submitted suggesting the same thing. I'm sorry I can't remember which ones. |
|
@sixtram you could achieve this with 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 |
|
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. 😄 |
dead-claudia
added
duplicate
Type: Breaking Change
|
@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 |
|
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. domvm/domvm#18 helped convince me that it was the correct behavior to adhere to. |
|
@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. 😢 Have you already implemented the approach you're describing in https://github.com/leeoniya/domvm? |
|
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: basically if you render an initially "checked" input (with no event handlers), then user clicks it to uncheck it. then |
|
@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"? |
|
There are a couple changes that went into the final patch. But yes, any props (explicit via [1] https://github.com/leeoniya/domvm/blob/master/src/util.js#L73 |
|
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. 😦 |
|
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 Compare |
dead-claudia
added
contribution welcome
and removed
Type: Breaking Change
|
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. |
|
IIRC the snippet in db17958 used to exist to deal w/ this issue |
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.
The text was updated successfully, but these errors were encountered: