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

Already on GitHub? Sign in to your account

check_box :disabled => true doesn't work as expected in Rails 3.0.7 or 3.0.9 #1953

Closed
HHRy opened this Issue Jul 4, 2011 · 30 comments

Comments

Projects
None yet
9 participants

HHRy commented Jul 4, 2011

In the case of:

    <%=  fields_for some_model[some_new_model], foo do |form| %>
      <%= form.check_box :bar, :disabled => true %>
    <% end %>

Correctly generates a disabled checkbox for this field; but the automatically generated hidden form field is not disabled as one would expect.

Contributor

dmathieu commented Jul 4, 2011

Have you tried if it's not already fixed in 3.0.9 ?

HHRy commented Jul 4, 2011

I've just verified that it's present in 3.0.9 as well.

Owner

pixeltrix commented Jul 4, 2011

The hidden field value is flipped depending on whether the checkbox is disabled or not. This is the intended behaviour as there's a test for it.

@pixeltrix pixeltrix closed this Jul 4, 2011

jaybee commented Jul 4, 2011

Hi there. I don't think the problem is being understood here. When a field is marked as :disabled, the value for that field should not be submitted to the server when the form is submitted. By leaving the hidden field in a non-disabled state, the value for that field is submitted, which would seem to break this suggestion:

http://www.w3.org/TR/html401/interact/forms.html#h-17.12.1

Could you clarify why this is intended?

HHRy commented Jul 4, 2011

+1 on the above, this breaks the expectation that disabling a field won't submit any value for it. Disabled elements should never be successful. If this behaviour is required, perhaps readonly is the correct attribute to use.

Owner

pixeltrix commented Jul 4, 2011

@jaybee I understand the problem - the test name explicitly makes it clear. The original commit refers to an original Rails Trac ticket that I can't access so I can't tell exactly what the rationale is, but it's obvious it's intent is to submit the current value when the checkbox is disabled. Until some comes up an explanation for the original commit and why that explanation doesn't apply any longer then nothing should be changed. Even then it shouldn't be changed for 3.0.x or 3.1.x as it's breaking backwards compatibility.

Owner

pixeltrix commented Jul 4, 2011

@HHRy the check_box form helper already breaks expectations somewhat by creating a hidden field. You can use the readonly attribute to prevent changes but that doesn't change the appearance of the form element - it may be you want to disable the element so that it can't be changed but still want the current value submitted back.

jaybee commented Jul 4, 2011

Thanks for responding. I understand the motivation behind the hidden field, and agree with it. It's an important use case where a checkbox is cleared, and you want the "flipped" value to be submitted, not "no value".

What seems to be lost is an understanding of the difference between readonly and disabled when applied to a field. readonly is a presentation-level attribute, where a field is disabled from user input, but its value is submitted to the server. That seems to be what the original commit really means. disabled absolutely, positively should never be submitted to the server. The original commit comment:

Disabled checkboxes don't submit a form value

is fixing a bug that isn't a bug. Disabled checkboxes should not submit a form value, though readonly ones should. The current behaviour intentionally makes it impossible to mark a checkbox field as truly disabled, and it's impossible to override it using an option.

Owner

pixeltrix commented Jul 4, 2011

Okay got the original ticket from archive.org. Looking at the description its intent is to fix the hidden field overriding a disabled checked check box which could've been fixed by applying :disabled to the hidden field so it'll be fine to change on master but I'm against about changing on 3-0-stable and 3-1-stable just in case someone is relying on this behaviour.

jaybee commented Jul 4, 2011

Phew, thanks for that :)

Should this issue be re-opened, or do you want me to file a separate, more explicit issue?

Owner

pixeltrix commented Jul 4, 2011

I'd like a pull request with tests and a changelog entry. :-)

If you don't feel up to it I'll reopen this ticket.

jaybee commented Jul 4, 2011

Hehe: I'm up to it, just not sure when I'll have the time soon ;-)

I'll see how I get on and revisit if it's taking me too long to get to.

@ghost ghost assigned pixeltrix Jul 4, 2011

@pixeltrix pixeltrix reopened this Jul 4, 2011

Owner

pixeltrix commented Jul 4, 2011

There's no rush - we still haven't released 3.1, so 3.2 will be a while yet. I've reopened the ticket and assigned it to me so it doesn't get lost in the noise.

jaybee commented Jul 4, 2011

All good. I've got a note to pick away at it. Should be an easy enough fix.

Contributor

dmathieu commented Jul 4, 2011

I think this should come with a change in jquery-ujs and prototype-ujs.
When a checkbox is disabled after loading the page, the hidden field should disappear.
Same thing when the checkbox is reenabled.

jaybee commented Jul 4, 2011

Not sure if it should "disappear": simply disabling it is good enough (and "correct", IMHO). If we remove it, we then have to put it back, and I don't think JS should be modifying the DOM like that for something that should still work without JS.

Owner

pixeltrix commented Jul 4, 2011

@dmathieu are you thinking that if it's re-enabled by javascript and the checkbox is left disabled then unchecking the checkbox won't work.

Contributor

dmathieu commented Jul 4, 2011

I mean that if javascript enables a checkbox which was loaded by rails as disabled (so with a disabled hidden field), the hidden field should be enabled too.
Otherwise, you end up with different behaviors when javascript enables/disables the checkbox.

jaybee commented Jul 4, 2011

Oh, yes, definitely this. In the broad strokes, whatever "happens" to
the checkbox should be mirrored on the hidden field.

Member

JangoSteve commented Jul 14, 2011

If this ends up being something that needs to be addressed in jquery-ujs, please open a ticket (or submit a pull request) over there when the time comes and I can take a look.

Contributor

joeellis commented Dec 23, 2011

Was running into this same issue, just wondering if it'll make it in the new 3.2 release

Member

JangoSteve commented Dec 23, 2011

I'm curious as well. If so, I'll need to update jquery-ujs to remove the disabled property of the hidden attribute when the form is submitted for check boxes that are enabled.

Contributor

tadast commented Jan 2, 2012

Let me know if there are any outstanding issues with the pull request, I'd be happy to change it if there is a need. @pixeltrix ?

@josevalim josevalim closed this in 37b4ba2 Jan 5, 2012

carlosantoniodasilva added a commit to carlosantoniodasilva/rails that referenced this issue Jan 27, 2012

I just run into an issue caused by the fix for this issue. It's very annoying that the disabled checkbox isn't sent from the form. Yes, I know the the most correct behavior is to not send disabled controls but in this case it's very inconvenient. Some people here suggested to use readonly, but, surprise surprise, readonly on on a checkbox doesn't work.

Owner

pixeltrix commented May 22, 2013

@jacob-carlborg care to describe what the issue is?

The value of the checkbox is not sent to Rails if it's disabled. I think it's inconvenient.

Owner

pixeltrix commented May 22, 2013

@jacob-carlborg that's the point of disabled - I mean how did it affect your application. For example was a column left with a NULL value?

No, we had a check to see if the checkbox was enabled. If it was, we associated another model with the model being updated. It failed to run some code that it should run.

You can give the same argument for checkboxes that are not disabled but unchecked.

"When a form is submitted, only "on" checkbox controls can become successful."

http://www.w3.org/TR/html4/interact/forms.html#checkbox

But we have hidden fields as a workaround for that, because it's more convenient.

Owner

pixeltrix commented May 22, 2013

I'm unable to see how this change will have affected your application. The hidden field is only present when using form_for and check_box - the check_box_tag helper doesn't add it. If the input isn't submitted then it will retain the existing value surely? Unless you're checking the attributes hash directly rather than the model.

Either way, the new behaviour is correct in my opinion and I don't see any way of easing the pain.

Yes, I did check the attributes hash directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment