fieldWithErrors shouldn't use a div

The

element is a block element, not an inline element. Nesting a

Great. Stupid lighthouse formatting.

The p element is a block element, not an inline element. Nesting a p inside a div is perfectly fine.

Sorry, but I don't really see a point in changing this.

Yes, perhaps my terminology is at fault. However, it is an issue. Firefox doesn't like DIVs inside Ps, it moves the p outside the div. Just try it in a plain file. It also fails validation.

Arthur, nesting a P inside a DIV is fine, agreed. But what's happening here is a DIV is being nestled inside a P, which isn't allowed.

The code looks like this:

I'll try again (lighthouse formatting): <p><div class="fieldWithErrors"><input /></div></p>

I just ran into this today, for the Nth time. I've always disliked adding the extra div. I'd rather see it just add the class to the proper elements, or structure the elements so that the div or span or whatnot is always there, instead of coming and going. Guess it's time to look at how Merb does this...

Alex, I looked this up in the HTML 4 and HTML 5 specs. You're right. The P element is only allowed to contain inline elements, no block elements. As DIV is a block element, things start to go bad.

So, +1 from me for your patch. :)

In the long term, this should be fixed by setting the class attribute on invalid input elements directly, instead on wrapping them into SPAN tags.

Adam, brilliant - that's definitely the way forward. I'll check out the patch.

+1 for adam's approach (that's how it should be imho)

+1 for Adam's patch. :)

Awesome. I always wanted this :D

+1

+1 for Adam's patch

+1 This or adding the a class right on the elements

+1 for Adam's patch. Have always been tweaking field_errors_proc on most projects to achieve the same thing.

Just to be Devil's Advocate - here's a reason why you'd want the extra markup rather than adding the class name to the element:

IE6 doesn't support double classes properly - so lets say you were trying to style an error marked checkbox differently from a textinput, for example, you wouldn't be able to do it if the checkbox had a class of fieldWithErrors, but you would if there was was a span with a class of fieldWithErrors containing that checkbox.

Maybe this is why extra markup was implemented in the first place?

Good point Alex! I agree that IE6 sucks, but unfortunately this is still a good reason to still use SPAN, imho.

So +1 for the first patch.

What I'd do in such a case might be to attach class names on the form elements' parent node, specifying the type of form input it encapsulates. For example:

<p class="input">
  <label>...</label>
  <input type="text" />
</p>
<p class="checkbox">
  <input type="checkbox" />
  <label>...</label>
</p>

And style the error marked checkbox using:

p.checkbox input.fieldWithErrors {
  ...
}

IMO, adding additional markup might interrupt with the styling of a page. As such, I'd still push for Adam's patch instead of replacing divs with spans.

+1 - very annoying problem

Oh, so we would have also to change scaffold.css in the patch, right?

José: No, I don't think you do. The class .fieldWithErrors is not element specific and should work find on the input, rather than (as currently) the div.

+1 for Adam's patch

+1 to this and #1760 in scaffold templates, change paragraph tags to fieldset tags

+1 for Adam's patch.

+1 for Adam's patch

+1 for Adam's patch

This is a great addition. I have looked at adding the class on the actual element in the past, but using a span around works for me just fine. +1 for sure. We have struggled with this faulty markup in every project I have been on.

There's another reason for the extra element around the the field -- some form elements can't take a big fat red border cleanly -- try that with a Select, especially in Safari -- having the wrapper makes it easy to style the error with padding and a background color so it pops out of the page.

i think the simplest approach would be to do something like this

http://drawohara.com/post/951920...

which is to say

1) just switch from div to span 2) allow overriding the error explanation classes (optional)

by moving div -> span the benefits of wrapping with another element remain and the html output is mostly backwards compatible.

my 2 cts.

+1

I'm looking at the tests in Adam's patch to get the general idea... and I'm not sure I'm in agreement. There's certainly room for improvement over the default output, but I don't think this patch addresses the real problem:

Currently errors get flagged by a fieldWithErrors wrapper div that encloses only the input element, and it comes and goes based on the status of the field... The patch nils out the field_error_proc, so we can avoid the transitory div, and moves the error class directly on the input element itself: but I think that's moving in the wrong direction.

(Also, I wonder how this patch handles composite inputs like date_select, would that mean 3 separate fieldWithErrors, thus 3 separate red borders on the select fields?)

I think everyone agrees there's a clear notion of a "field" which groups elements together: the input and the label, just to start! (One might also want inline error messages, hints, etc. on a field but that's custom). The point is these elements should be semantically part of the invalid field together.

I would rather propose a standard div element that wraps the pieces that make up a field, and conditionally apply the invalid class to that, just a little higher up in the DOM. From there it's easy to select invalid inputs or labels, or whatever other element of the field, using a descendant selector.

Here's what I would prefer to see as default behaviour:

<!-- valid field -->
<div class="field">
<label for="post[author_name]">Author Name</label>
<input name="post[author_name]" id="post_author_name" size="30" type="text" value="" />
</div>

<!-- invalid field: the only diff is the outer wrapper -->
<div class="field with_errors">
<label for="post[author_name]">Author Name</label>
<input name="post[author_name]" id="post_author_name" size="30" type="text" value="" />
</div>

This would obviously require another helper, but I think most of us already enclose our fields in a div, p, or li tag by convention anyway so a little ERB would just take the place of the HTML wrapper, something like this:

<% f.field_tag(:author_name, :as => :div) do %>
<%= f.label :author_name %>
<%= f.text_field :author_name %>
<% end %>

There could also be an option for having the error class on the input element like in Adam's patch without needing to use the field_tag, but I think that usage without a label should be more the exception than the rule.

+1 on Adam's patch.

For the time being, I'm just redefining ActionView::Base.field_error_proc on an initializer.

What if we apply a 'display: inline' to the div inside the p? Wouldn't that make the markup valid?

@Gustavo what would be the point of that when you could just change it to a span, which is already an inline element?

+1 for Adam's patch

+1 for Adam's patch. I've had to use a Proc in an initializer to sub out the div for a span on several projects in the last few days.

+1, I'm voting for Adam's patch. It might not be IE6-compatible but to be honest this should not mean that there can't be a solid implementation in Rails (which Adam's patch certainly supplies).

Also, I think this has been open for long enough to finally make it into a release. Very annoying to still have this around with everybody rolling their own workaround.

I'm not normally one to rudely bump a ticket but really, what's the hold up here? There was a ticket for the exact same issue in Trac dating back to 2005 and yet it still hasn't been applied despite strong support and a lack of any convincing counter-argument.

+1 for Adam, lets fix this please! What's the hold up?

I think neither approach solves the real problem. (Which still exists in Rails 3 as of now)
Being able to customize field_error_proc on an request by request basis.

I currently use the following code in an initializer that gets around that:

# Problem:
# We want to customize field_error_proc for each action.
# We also want field_error_proc to remain threadsafe.
# Solution:
# We can access the controller inside of the proc. We check is the controller has an
# instance variable @field_error_proc. If so we invoke it. If not we invoke the default proc.
def override_field_error_proc
  pre = ActionView::Base.field_error_proc
  ActionView::Base.field_error_proc = proc { |input, instance|
    proc = instance.instance_variable_get('@template_object').controller.instance_variable_get('@field_error_proc')
    if proc.nil?
      pre.call(input, instance)
    else
      proc.call(input, instance)
    end
  }
end

override_field_error_proc

Edited by Rohit Arondekar for formating.

Sorry for the formatting error, maybe someone could fix that. It seems I can't edit my comment.

Here's the code I wanted to display: http://pastie.org/743863

+1 on this. In all my projects I have the fix_fields_with_errors.rb file in initializers to change div for span.

+1

In every project I do the below to change div to span:

config.action_view.field_error_proc = Proc.new{ |html_tag, instance| "<span class=&quot;fieldWithErrors&quot;>#{html_tag}</span>" }

The methods in question I believe have been extracted into a plugin http://github.com/rails/dynamic_form Can somebody check if the issue still exists and update the ticket?

The error_proc and error wrapping code is still part of core so the issue still exists.

Having said that, it'd probably better if they were moved to the dynamic_form plugin.

error_messages_for no more in the rails core, it's separate plugin. So you may try to contact with maintainer, add pull request from your fork or something like that. Or better to use simple_form or formtastic (in formtastic there are good semantic_errors method).