:hidden_fields => false should not render "none" for checkboxes #697

Closed
dnagir opened this Issue Sep 29, 2011 · 15 comments

Projects

None yet

4 participants

@dnagir

When :as => :chck_boxes, :hidden_fields => false, the hidden "_none" field is still rendered.

PROBLEM: fantom, fake entry in the resulting array.

Let me better give an example:

= semantic_form_for @property_search do |f|
  = f.inputs do
    = f.input :bedrooms, :as => :check_boxes, :hidden_fields => false, :collection => {0 => "Studio", 1 => "1 Bedder", 2 => "2 Beds"}
  f.buttons

This renders (stripped to bare):

<input id="property_search_bedrooms_none" name="property_search[bedrooms][]" type="hidden" value="">
<input id="property_search_bedrooms_0" name="property_search[bedrooms][]" type="checkbox" value="0">
<input id="property_search_bedrooms_1" name="property_search[bedrooms][]" type="checkbox" value="1">
<input id="property_search_bedrooms_2" name="property_search[bedrooms][]" type="checkbox" value="2">

The data that gets into into params[:property_search] depending on what's checked:

  • Nothing checked: {"bedrooms"=>[""]}
  • 1 item is checked: {"bedrooms"=>["", "0"]}
  • 2 items are checked: {"bedrooms"=>["", "0", "1"]} etc

So my suggestion is NOT to render any hidden fields when the option is specified to avoid a fake empty element in the array.

@yabawock
Collaborator

If such a fake field isn't rendere it isn't possible to deselect all checkboxes.

HTML Browser don't send the input field value "property_search[bedrooms][]" unless at least 1 checkbox has been ticket. To enable an application to recognize that no checkbox is ticked this "fake" value is needed.

@dnagir

No, the browser does send the fake field even if nothing is selected. That fake field has nothing to do at all with the rest of the checkboxes from HTML perspective (its type is hidden).

But I'm not sure what you mean saying "deselect all checkboxes" and what deselecting has to do with the hidden input.

@yabawock
Collaborator

The hidden field ensures that the browser always sends "something" for the :as => :checkboxes parameter

Like you have shown in your example, when no checkbox is selected you get an "empty" value. This enables the controller to react to the fact that the user hasn't ticked any checkbox. If you leave out the hidden field, don't tick any checkbox and submit the form the browser doesn't transmit anything for that attribute. So you wouldn't be able to update/clear for example a has many association.

@dnagir

The controller can't react correctly when an empty string is a valid value (Is it empty string or nothing selected then?).

Also when nothing is selected, the result should be an empty array or nil. But definitely not an array with one item in it.

What you are saying is kind of inconsistent with the normal understanding of what empty means. Array with one item is not empty. It has one item.

Additionally, that one single fake item needs special treatment all the time. It is not necessary as in most cases you already check for nil and/or empty arrays.

I'm still not convinced that we need that fake item. ESPECIALLY, when explicitly asked not to render the hidden fields.

@yabawock
Collaborator

Let's say it this way: There is no way to transmit an empty array in a HTML form, so you need to resort to such things as zero length strings. Rails performs some "magic" in the background when updating associations and treats the single zero length string as a hint to clear the association.

So it might not be needed in your use case, but it is in most other use cases. I wouldn't consider this a bug.

@dnagir

Yeah, I understand that you can't transfer an empty array via HTTP for Rails.
But that is not the point. :hidden_fields => false should not use any hidden fields. That will cover that small percentage of use-cases you are talking about.
Otherwise what's the point of :hidden_fields option.

@yabawock
Collaborator

Exactly what it states in the documentations:

:hidden_fields can be used to skip Rails' rendering of a hidden field before every checkbox

If you do not set this flag rails will, by default, render a hidden field for each checkbox in the collection so that you can detect the checked/unchecked state for every single checkbox, even if you have a dynamic collection where you do not know which or how many checkboxes will be displayed.

@dnagir

hidden_fields does affect hidden inputs only near every checkbox.
But it should also affect the single "_none" hidden field that doesn't belong to either of the checkboxes.
This way nil will be used when nothing is selected instead of an array with a single item.

Currently the "_none" hidden field is explicitly rendered when :hidden_field => false:
https://github.com/justinfrench/formtastic/blob/master/lib/formtastic/inputs/check_boxes_input.rb#L112

There must be a reason for doing this. But there should be an option of removing that field as well.

@justinfrench

So the proposal is to enhance :hidden_field => false to include not rendering the _none? I'm okay with this in theory (from a DSL/API perspective), but would be great to see some +1s from others to support the change. A pull request would also be great!

@dnagir

The patch is in #702

@yabawock
Collaborator

I'm not really convinced - see the lengthy discussions in #246 and #322 for the original intention of the field.Plus the patch in #702 changes default behaviour and is bound to break existing applications working with associations.

@dnagir

It should not break existing behaviour. All the specs are passing for the checkbox_input.
And it will ONLY apply when you set :hidden_fields to false (you don't usually do it for associations, right?).

@cmaggard

This is whacking me in the face recently as well. I'm using a check_boxes input to pass back IDs, and the phantom empty string gets translated to 0 and raises an exception when the record can't be found. I love Formtastic, but this behavior doesn't happen when using the Rails checkbox input, so I'm torn.

@justinfrench
Owner

@cmaggard you can always customise that input in Formtastic to suit your tastes in the meantime. Basically, the community is divided on how this should be approached, and I don't have time to make sense of it all.

@justinfrench
Owner

Closing. Changing the behaviour we have will cause non-obvious problems for many existing apps, and there's no obviously right or wrong approach. Rails 4 has introduced some hidden inputs in places too (like for a multi-select), so that's more reason to stick with what we have.

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