Unexpected Behavior When Using The Checked Binder With Radio Inputs
Opened this issue · 15 comments
Issue Discussion
I've been using Tinybind for a small personal project and came across a documentation bug for the checked binder. The binder documentation states the following.
Checked
Checks the input when the value evaluates to true and unchecks the input when the value evaluates to false. This also sets the bound object's value to true/false when the user checks/unchecks the input (two-way).
Use this instead of value when binding to checkboxes or radio buttons.
Checkbox inputs using the checked binder work as expected, but radio inputs do not. Below is a description of the actual behavior and a minimal demo I put together.
Based on the documentation, I put together a data structure to bind to some radio inputs.
var DATA = {
"radios": [
{
// Binds to the radio ID attribute.
"id": "d1r1",
// Binds to the radio value attribute.
"value": "radio-1",
// Used for the label associated with the radio.
"label": "Radio 1",
// Binds to the radio checked DOM value.
"checked": true,
// Helper variable to display the actual DOM value associated
// with the radio.
"_value": undefined,
// Helper variable to display the actual DOM checked state
// associated with the radio.
"_checked": undefined
},
{
"id": "d1r2",
"value": "radio-2",
"label": "Radio 2",
"checked": false,
"_value": undefined,
"_checked": undefined
},
{
"id": "d1r3",
"value": "radio-3",
"label": "Radio 3",
"checked": false,
"_value": undefined,
"_checked": undefined
}
]
};
Here is the template code. The updateLabels function purely updates the label so I have omitted it here.
<div rv-each-rdata="radios">
<input
type="radio"
name="demo-1"
rv-id="rdata.id"
rv-value="rdata.value"
rv-checked="rdata.checked"
rv-on-click="functions.updateLabels"
>
<label
rv-for="rdata.id"
>{ rdata.label } - DOM Value: { rdata._value } - DOM Checked: { rdata._checked } - Model Checked: { rdata.checked }</label>
</div>
The first issue that arises is a failure to check radio input when the associated model datum is set to the boolean true. In the demo below, Radio 1 should be checked because the associated model datum is true, but you can see it is not. Even setting the model datum explicitly after the bind call does not update the DOM state for the radio inputs.
The second issue is an incorrect updating of the model datum when the DOM state is changed. When a user clicks any of the radio inputs, the model datum associated with that radio input is not set to true as stated in the documentation. It is instead set to the DOM value associated with the radio input. If no value is set, the value 'on' is used.
These bugs manifest with various configurations including inside a <fieldset> and even in the absence of the name and value attributes.
I initially attempted to generate a patch that honors the intent of the checked binder based on the documentation, but another issue arises. Radio inputs do not emit any DOM events when the are unchecked. This means there is no hook for the library to update the underlying model datum for a radio input that becomes unchecked.
I don't fully understand the rational for the HTML standard to exclude radios from emitting events when unchecked, but that is separate conversation.
What does work is treating radio inputs the same way you would treat a select input. Another way to say this is treating the checked binding as a string result across a group of radio input instead of a boolean result on a single radio input.
New data structure.
var DATA = {
// The single value that all radio inputs in the group
// should bind to. Set to the string 'undefined' for
// demo purposes.
"selected-value": "undefined",
"radios": [
{
"id": "d3r1",
"value": "radio-1",
"label": "Radio 1",
"_value": undefined,
"_checked": undefined
},
{
"id": "d3r2",
"value": "radio-2",
"label": "Radio 2",
"_value": undefined,
"_checked": undefined
},
{
"id": "d3r3",
"value": "radio-3",
"label": "Radio 3",
"_value": undefined,
"_checked": undefined
}
]
};
New template code.
<label>Selected Value: {selected-value}</label>
<div rv-each-rdata="radios">
<input
type="radio"
name="demo-3"
rv-id="rdata.id"
rv-value="rdata.value"
rv-checked="selected-value"
rv-on-click="functions.updateLabels"
>
<label
rv-for="rdata.id"
>{ rdata.label } - DOM Value: { rdata._value } - DOM Checked: { rdata._checked }</label>
</div>
If this is the expected functionality, then the documentation needs to be updated to discuss how to properly use the checked binder with radio inputs.
Demo Code
<!doctype html>
<html lang="en">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Tinybind Demo</title>
<meta name="description" content="Demo for tinybind.">
<meta name="author" content="Joey C. DiGiorgio">
</head>
<body>
<script src="tinybind.js"></script>
<script src="tinybindPatched.js"></script>
<script>
////////////////////////////////
// Variables
var g_pageStructureLoaded = false;
var g_pageContentLoaded = false;
////////////////////////////////
// Functions
function idToTitle(id)
{
var wordList = id.split("_");
var capWordList = wordList.map(
function(word)
{
return word[0].toUpperCase() + word.substring(1);
}
);
var title = capWordList.join(" ");
return title;
}
function addExampleDiv(id)
{
var heading = idToTitle(id);
var style = `
<style>
.example
{
text-align: center;
border: 1px solid black;
margin-bottom: 1rem;
padding-bottom: 1rem;
padding-left: 1rem;
padding-right: 1rem;
};
</style>
`;
var html = `
<div id="${id}" class="example">
<h2>${heading}</h2>
</div>
`;
var div = document.createElement("div");
div.innerHTML = `${style}${html}`;
document.body.append(div);
return div.lastElementChild;
}
function bugDemo()
{
var exampleDiv = addExampleDiv("Tinybind Radio Input Bug Demo");
var updateLabels = function(e, view)
{
var radioDomList = exampleDiv.querySelectorAll(`input[name=demo-1]`);
var radioDataList = view["$parent"]["radios"]
for(var i = 0; i < radioDomList.length; i++)
{
var radioDom = radioDomList[i];
var radioData = radioDataList[i];
radioData._value = radioDom.value;
radioData._checked = radioDom.checked;
}
};
var DATA = {
"radios": [
{
"id": "d1r1",
"value": "radio-1",
"label": "Radio 1",
"checked": true,
"_value": undefined,
"_checked": undefined,
},
{
"id": "d1r2",
"value": "radio-2",
"label": "Radio 2",
"checked": false,
"_value": undefined,
"_checked": undefined,
},
{
"id": "d1r3",
"value": "radio-3",
"label": "Radio 3",
"checked": false,
"_value": undefined,
"_checked": undefined,
},
],
"functions": {
"updateLabels": updateLabels,
},
};
var HTML = `
<div rv-each-rdata="radios">
<input
type="radio"
name="demo-1"
rv-id="rdata.id"
rv-value="rdata.value"
rv-checked="rdata.checked"
rv-on-click="functions.updateLabels"
>
<label
rv-for="rdata.id"
>{ rdata.label } - DOM Value: { rdata._value } - DOM Checked: { rdata._checked } - Model Checked: { rdata.checked }</label>
</div>
`;
exampleDiv.innerHTML += HTML;
tinybind.bind(exampleDiv, DATA);
updateLabels(undefined, {"$parent": DATA});
window.bugDemo = DATA;
}
function patchDemo()
{
var exampleDiv = addExampleDiv("Tinybind Radio Input Patch Demo");
var updateLabels = function(e, view)
{
var radioDomList = exampleDiv.querySelectorAll(`input[name=demo-2]`);
var radioDataList = view["$parent"]["radios"]
for(var i = 0; i < radioDomList.length; i++)
{
var radioDom = radioDomList[i];
var radioData = radioDataList[i];
radioData._value = radioDom.value;
radioData._checked = radioDom.checked;
}
};
var DATA = {
"radios": [
{
"id": "d2r1",
"value": "radio-1",
"label": "Radio 1",
"checked": true,
"_value": undefined,
"_checked": undefined,
},
{
"id": "d2r2",
"value": "radio-2",
"label": "Radio 2",
"checked": false,
"_value": undefined,
"_checked": undefined,
},
{
"id": "d2r3",
"value": "radio-3",
"label": "Radio 3",
"checked": false,
"_value": undefined,
"_checked": undefined,
},
],
"functions": {
"updateLabels": updateLabels,
},
};
var HTML = `
<div rv-each-rdata="radios">
<input
type="radio"
name="demo-2"
rv-id="rdata.id"
rv-value="rdata.value"
rv-checked="rdata.checked"
rv-on-click="functions.updateLabels"
>
<label
rv-for="rdata.id"
>{ rdata.label } - DOM Value: { rdata._value } - DOM Checked: { rdata._checked } - Model Checked: { rdata.checked }</label>
</div>
`;
exampleDiv.innerHTML += HTML;
tinybindPatched.bind(exampleDiv, DATA);
updateLabels(undefined, {"$parent": DATA});
window.patchDemo = DATA;
}
function workingDemo()
{
var exampleDiv = addExampleDiv("Tinybind Radio Working Demo");
var updateLabels = function(e, view)
{
var radioDomList = exampleDiv.querySelectorAll(`input[name=demo-3]`);
var radioDataList = view["$parent"]["radios"]
for(var i = 0; i < radioDomList.length; i++)
{
var radioDom = radioDomList[i];
var radioData = radioDataList[i];
radioData._value = radioDom.value;
radioData._checked = radioDom.checked;
}
};
var DATA = {
"selected-value": "<undefined>",
"radios": [
{
"id": "d3r1",
"value": "radio-1",
"label": "Radio 1",
"_value": undefined,
"_checked": undefined,
},
{
"id": "d3r2",
"value": "radio-2",
"label": "Radio 2",
"_value": undefined,
"_checked": undefined,
},
{
"id": "d3r3",
"value": "radio-3",
"label": "Radio 3",
"_value": undefined,
"_checked": undefined,
},
],
"functions": {
"updateLabels": updateLabels,
},
};
var HTML = `
<label>Selected Value: {selected-value}</label>
<div rv-each-rdata="radios">
<input
type="radio"
name="demo-3"
rv-id="rdata.id"
rv-value="rdata.value"
rv-checked="selected-value"
rv-on-click="functions.updateLabels"
>
<label
rv-for="rdata.id"
>{ rdata.label } - DOM Value: { rdata._value } - DOM Checked: { rdata._checked }</label>
</div>
`;
exampleDiv.innerHTML += HTML;
tinybind.bind(exampleDiv, DATA);
updateLabels(undefined, {"$parent": DATA});
window.workingDemo = DATA;
}
function main()
{
// Wait for both the page structure and content to be loaded.
if(!g_pageStructureLoaded || !g_pageContentLoaded)
{
return;
}
bugDemo();
patchDemo();
workingDemo();
}
function onPageContentLoaded()
{
g_pageContentLoaded = true;
main();
}
function onPageStructureLoaded()
{
g_pageStructureLoaded = true;
main();
}
////////////////////////////////
// Script Entry Point
document.addEventListener(
"DOMContentLoaded",
onPageStructureLoaded
);
window.addEventListener(
"load",
onPageContentLoaded
);
</script>
</body>
</html>
Patch Code
diff --git a/src/binders.js b/src/binders.js
index 79d14b1..396cc7f 100644
--- a/src/binders.js
+++ b/src/binders.js
@@ -214,11 +214,7 @@ const binders = {
},
routine: function(el, value) {
- if (el.type === 'radio') {
- el.checked = getString(el.value) === getString(value)
- } else {
- el.checked = !!value
- }
+ el.checked = value
}
},
diff --git a/src/bindings.js b/src/bindings.js
index afe9a9c..db35f54 100644
--- a/src/bindings.js
+++ b/src/bindings.js
@@ -2,7 +2,7 @@ import {parseType} from './parsers'
import Observer from './observer'
function getInputValue(el) {
- if (el.type === 'checkbox') {
+ if (el.type === 'checkbox' || el.type === 'radio') {
return el.checked
} else if (el.type === 'select-multiple') {
const results = []
I don't mind drafting the changes to the documentation, but wanted to verify what the expected functionality is first before submitting a pull request for code or documentation changes.
Hi, i dont use tinybind anymore due to such limitations
I can grant access to repository if you want
@blikblum definitely interested in keeping this project alive as a "micro framework" . there is some usefulness still to be had here and will try to help starting with @MrJman006 patch if he is willing to help.
Im interested in keeping it alive also. Im up for getting access to the repo. In terms of documentation on the website how do you manage that?
Im interested in keeping it alive also. Im up for getting access to the repo. In terms of documentation on the website how do you manage that?
It seems he has a gh-pages branch where it appears to have the site and the documentation.
I do not see a major overhaul at the moment once taking over the project and with it being a micro framework we can continue with the current site/ documentation used with the necessary edits and updates.
My only suggestion might be toi move it to its own repo to separate from the core project. We can maybe use the repo Wiki for more detailed documenting( like custom components and adapters) as the code would be near by. Still up for debate though
Issue Discussion
I've been using Tinybind for a small personal project and came across a documentation bug for the
checkedbinder. The binder documentation states the following.Checked
Checks the input when the value evaluates to true and unchecks the input when the value evaluates to false. This also sets the bound object's value to true/false when the user checks/unchecks the input (two-way).
Use this instead of value when binding to checkboxes or radio buttons.Checkbox inputs using the
checkedbinder work as expected, but radio inputs do not. Below is a description of the actual behavior and a minimal demo I put together.Based on the documentation, I put together a data structure to bind to some radio inputs.
var DATA = { "radios": [ { // Binds to the radio ID attribute. "id": "d1r1", // Binds to the radio value attribute. "value": "radio-1", // Used for the label associated with the radio. "label": "Radio 1", // Binds to the radio checked DOM value. "checked": true, // Helper variable to display the actual DOM value associated // with the radio. "_value": undefined, // Helper variable to display the actual DOM checked state // associated with the radio. "_checked": undefined }, { "id": "d1r2", "value": "radio-2", "label": "Radio 2", "checked": false, "_value": undefined, "_checked": undefined }, { "id": "d1r3", "value": "radio-3", "label": "Radio 3", "checked": false, "_value": undefined, "_checked": undefined } ] };
One thing that I'm curious about was
Why use
rv-checkedand notrv-valuefor radios?
While check boxes and radio buttons act similar their business logic is quite different.
Check Boxes are more of an optionally "enabled" "disabled" logic while radio buttons are more of a required "choose either or" value based logic. They should be treated different even though they have a similar functionality with Radio buttons using onclick event listeners instead with a "select option" apprach as you suggested, as radio buttons are always plural and not singular.
Perhaps we can mandate that the template syntax be such that the markup provides a grouping wit the nested radio buttons and a parent model as you suggested.
I dont't see why we need to make the recommended patch though has his logic has merit we simply need to "add specifity".
I'm yet to run tests for this issue but will look at it as I may need this functionality especially, along with this issue
I added both as collaborators to this repo.
The docs are in gh-pages branch, _harp folder
While check boxes and radio buttons act similar their business logic is quite different. Check Boxes are more of an optionally "enabled" "disabled" logic while radio buttons are more of a required "choose either or" value based logic. They should be treated different even though they have a similar functionality...
I completely agree. Checkbox inputs and radio inputs both have a checked state, but their usage is for boolean selection and set selection respectively.
Why use rv-checked and not rv-value for radios?
The value binder can't be used because we would be breaking its semantics. There are several HTML elements that use the value attribute. When using the value binder with input elements specifically (excluding radio inputs) the semantic is a two way data binding between a model datum and the value attribute. Changing this semantic for radio inputs not only breaks Tinybind users expectations when using the value binder, but also prohibits the use of the each-* binder as there would no longer be any way the user to set the value attribute for each-* generated elements.
Perhaps we can mandate that the template syntax be such that the markup provides a grouping wit the nested radio buttons and a parent model as you suggested.
So I'm torn on this point. I agree that we need some way to get different groups of radio inputs for the purposes of keeping the model data in sync, but I really dislike forcing the user to structure their page a specific way to make a Tinybind feature work. Especially when alternative page formats are valid HTML. Interestingly the HTML spec for radio inputs states that all radio inputs that are not explicitly grouped are to be treated as in the same group based on several structure rules. We might be able to use those structure rules to keep a group list at bind time and use that list to update the model data when changes occur. The problem is catching all of the situations where a radio input can have its checkedness status change some of we may not have any way of determining.
I dont't see why we need to make the recommended patch though has his logic has merit we simply need to "add specifity".
So some kind of patch is needed to fix the checked binder for radio inputs because the semantics of the checked binder for radio inputs are not satisfied currently. The patch in my original post, is only a partial fix. The rest of the fix requires changes to address the grouping issue discussed above since radio inputs are dependent on each other unlike all other input elements.
I also think having a new binder to handle the selected functionality that I demo in my last GIF is important to have as we both agree that is a primary use case for radio buttons. I might move that to a separate issue to keep things organized.
My only suggestion might be toi move it to its own repo to separate from the core project.
Now that I see the documentation is on a branch of the project I think I understand that the website documentation is being pulled directly from that branch using GitHub pages. I guess we could technically put it in a separate repo if there is a compelling reason to do that, but I'm fine leaving it here as a branch also.
The docs are build with http://harpjs.com/docs/
So I ran some tests and made this patch.
The premise is that we need to treat rv-value for input[type=radio|checkbox] as a regular attribute, as the rv-checked will do the data binding for us. Adding to this, while input[type=checkbox] needed only to be checked/unchecked input[type=radio] needs to select a variable's option value ( much like <select> ) thus we need to set rv-checked=variable|property to be used instead of rv-checked=true|false
I added some new specs to test out the patch but not quite sure my tests were sound so if you guys want to give it go...have at it!
I also setup a demo wit the patch to show how we should be using radio buttons and checkboxes with both rv-checked and rv-value and will update the docs according once you guys review the modifications!
So I ran some tests and made this patch.
The premise is that we need to treat
rv-valuefor input[type=radio|checkbox] as a regular attribute, as therv-checkedwill do the data binding for us. Adding to this, while input[type=checkbox] needed only to be checked/unchecked input[type=radio] needs to select a variable's option value ( much like<select>) thus we need to setrv-checked=variable|propertyto be used instead ofrv-checked=true|falseI added some new specs to test out the patch but not quite sure my tests were sound so if you guys want to give it go...have at it!
I also setup a demo wit the patch to show how we should be using radio buttons and checkboxes with both
rv-checkedandrv-valueand will update the docs according once you guys review the modifications!
I'm confused what the patch you added does? The checked binder already was setting a non-binary value for input[type=radio] and the value binder worked for both input[type=checkbox] and input[type=radio]. Maybe we need to chat together so we don't run over each other because I have been working on a patch as well to fix the checked binder to handle boolean values as the documentation states and to add a new binder called select that implements the current checked functionality for radio inputs.
@blikblum one question though as you are more familiar with the project
what does this do and could we have used this to toggle event listening and publish() based on element type?
It is a state variable telling Tinybind which binders publish view changes back to the model. See here (src/view.js lines 165-172.
@MrJman006 based on what I understood about your original issue, using the rv-checked on radio buttons did not update the dom element accordingly. Your patch did fix that but I encountered another issue with it using it on checked boxes at it updated the text content to "true" instead of the value of the check box. There was another issue as well using both rv-checked and rv-value on the same element for checkboxes/ radio buttons as well with the patch. So i had to refactor the code a bit to fix both issues so that
- Both elements triggered update of the DOM element to visually show it being "checked"
- Radio buttons updated a model value with its model property value if checked
- Check buttons updated ITS model property value to true or false
- Radio button functioned properly when using
rv-valueandrv-checked
What is needed to be done is to update the documentaion on showing the proper way to use the elements with the respective binders as necessary. I think this is the main part as it may not be properly conveyed with a distinction between the two elements.