field_with_errors div getting added around default <option> tag #7017

Closed
rustygeldmacher opened this Issue Jul 9, 2012 · 3 comments

Projects

None yet

2 participants

Contributor

Came across a strange HTML validation error while updating our app from Rails 3.0.11 to 3.2.6. It seems like if we have a <select> tag for a field with both an :include_blank option and a validation error, the <div class="field_with_errors"> wrapper will be put around both the <select> tag and the first <option>.

Here's a quick way to reproduce.

Assume a fresh Rails 3.2.6 app with the following model generation:

rails g model my_model val:string 

and the following in app/models/my_model.rb:

class MyModel < ActiveRecord::Base
  attr_accessible :val
  validates :val, :presence => true
end

Now in a rails console go:

> mm = MyModel.new
> mm.valid?
> fb = ActionView::Helpers::FormBuilder.new('my_model', mm, helper, {}, nil)
> fb.select(:val, [:a, :b], :include_blank => 'select something...')
 => "<div class=\"field_with_errors\"><select id=\"my_model_val\" name=\"my_model[val]\"><div class=\"field_with_errors\"><option value=\"\">select something...</option></div>\n<option value=\"a\">a</option>\n<option value=\"b\">b</option></select></div>" 

Notice the extra <div class="field_with_errors"> around the first option? This is not valid HTML, and probably should not be included in the output <select> tag.

I've tracked this down to the method add_options in the InstanceTag class (in the file lib/action_view/helpers/form_options_helper.rb) and its use of content_tag in order to generate the option for :include_blank or :prompt. It seems like ActionView::Helpers::ActiveModelHelper wraps content_tag so that it calls error_wrapping around generated tags, thus causing the errant <div> around the first option.

I'd be happy to submit a patch for a fix, but I'd like some guidance on where/how to best fix this. I can think of a few ways to fix this, in order of my personal preference:

  1. Have add_options call content_tag_string rather than content_tag -- content_tag_string is just a thin wrapper around content_tag and is not wrapped so as to call error_wrapping.
  2. Add an extra option to content_tag (called maybe :wrap_errors) that if present and false will cause error_wrapping in ActionView::Helpers::ActiveModelHelper to be skip creating the wrapper div.
  3. Change error_wrapping in ActionView::Helpers::ActiveModelHelper to be aware of and skip <option> tags
  4. Change @@field_error_proc in ActionView::Base to explicitly not wrap <option> tags

Thanks for listening, looking forward to some feedback!

Owner

I think that the options 1. is the better.

Contributor

Sent pull request -- not sure where to continue the discussion, here or there. I added a general test case for selects with errors, then a second case for selects with errors plus empty initial options. Please let me know how it looks, thanks!

@rafaelfranca rafaelfranca was assigned Jul 10, 2012
Owner

Merged both pull requests. Thank you

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