Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When using elAttribute, the value is not saved to the model for checkboxes #138

Open
lastquestion opened this issue May 9, 2013 · 8 comments

Comments

@lastquestion
Copy link

I'm (quite) surprised this wasn't caught by something.. am I doing something wrong here?
Example binding:

{
            visible: {selector: '[name=is_visible]',
                      elAttribute: 'checked'}
};
  <input type="checkbox" name="is_visible" disabled="disabled"></input>

So, when this is changed, we hit

        _isBindingUserEditable: function(elBinding){
             return elBinding.elAttribute === undefined ||
                elBinding.elAttribute === 'text' ||
                elBinding.elAttribute === 'html';
         },

And this is going to return false, so we don't attempt to _copyViewToModel():

                if (this._isBindingUserEditable(elBinding)) {
                    this._copyViewToModel(elBinding, el);
        }

I propose changing _isBindingUserEditable to something like this:

        _isBindingUserEditable: function(elBinding){
            return elBinding.elAttribute === undefined ||
                ['text', 'html', 'checked'].indexOf(elBinding.elAttribute) != -1;
        },
@katowulf
Copy link
Collaborator

katowulf commented May 9, 2013

The "checked" attribute is only used to set the initial state of a checkbox. According the W3C spec, this is not a dynamic value that can be changed on a whim. See the jQuery discussion for a bit more depth on why this is a complex issue.

Does this not work if you just remove the elAttribute? It should default appropriately.

Btw, the "checked" behavior was recently updated by my changes to fix the use of attr and prop.

@lastquestion
Copy link
Author

Ahhhhh, I see. Yes I am on pretty up to date master branch.

I agree that apparently checked is the wrong attribute to use, and instead should be used as a property.

A couple of thoughts:

  1. We need to update the docs
  2. When, exactly, would a user actually use elAttribute at this point, given that changes will never get propagated back? I suppose it could be used for a read-only binding, so I'd like to see this renamed to clarify that... it's going to confuse a lot of people. I already see 3 issues referenced on this very change, so it's clearly a problem

@katowulf
Copy link
Collaborator

katowulf commented May 9, 2013

I haven't personally had a use for elAttribute in a two-way sense. It doesn't really make sense that we're talking two-way unless it's binding to a value or checked property anyway. Is a user inputting data to the style tag or title attribute that we then want propagated to the server? I don't really see this as valuable. Plus, any variation of this edge case could be easily achieved with a converter.

I guess for my part it makes perfect sense that unless you bind to something that is editable and fires change events (an input) then there's not going to be a return value.

As for the naming of elAttribute, it doesn't (again, for me) convey an idea that it does anything other than bind the value to a particular attribute of the DOM element.

I had a quick look for related and open issues and didn't find anything similar to this except for #75, which is about binding to a list of checkboxes. A quick scan of the closed issues reveals only the ones I submitted related to my change, and one about radios not firing a change event. Did I miss some that seem to be caused by "checked" used as elAttribute?

@lastquestion
Copy link
Author

Ah, my bad. I saw two other issues referenced by that pull request but actually they were opened by you.

As for the naming of elAttribute, it doesn't (again, for me) convey an idea that it does anything other than bind the value to a particular attribute of the DOM element.

I think you've explained pretty well how elAttribute works now, and why it works the way it does. Two thoughts:

  1. Did it use to work differently? I merged about.. ~2 months of changes and I feel like I used to "need to use" elAttribute:"checked" for checkboxes to work. If this is so, as people migrate, this is going to come up again and again. I glanced through the docs but nothing jumped out at me. Maybe I just coded it that way because from reading the docs especially with the examples around enabled, I thought that's what I was supposed to do.
  2. I feel that the behavior of elAttribute now is a bit surprising. I guess I get that elAttribute is a "cheap light" way of binding to an attribute so you don't have to spin up a converter. But whereas all the other bindings will work two way by default, OR, you explicitly write a converter that handles bidirectionality (or not), elAttribute "silently" doesn't handle view->model.

Thinking about it more, I can't see a breaking change to remove or rename elAttribute as worth it, so I guess..

TLDR; add documentation, don't change anything is the best that can be done I think.

@lastquestion
Copy link
Author

I be happy to write up some new documentation in a PR...

@katowulf
Copy link
Collaborator

katowulf commented May 9, 2013

I suppose you should wait for @theironcook (Mr. Bart Wood) to chime in with his thoughts about the docs and usage. He is, after all the expert.

As for my part, I've probably done a poor job explaining some of the behaviors. I don't think elAttribute implies anything about whether it's a two way binding and haven't meant to imply that. There just don't seem like there are logically many two-way things that can be bound to.

I wonder if it would make sense to add :checked and :selected (referencing the boolean property) into the elAttributes possibilities (or if these might even work already), and if perhaps "checked" (referencing the checked attribute) should just go ahead and reference :checked internally?

Alas, I like to overthink things; I'm at least one half engineer : )

@theironcook
Copy link
Owner

@katowulf @lastquestion

katowulf has done a very good job at explaining what's going on. @katowulf - would you like to be added as a contributor to the project?

@katowulf
Copy link
Collaborator

@theironcook shoot me a message at katowulf of the gmail realm regarding contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants