This project is archived and is in readonly mode.

#1626 ✓wontfix
Alex MacCaw

fieldWithErrors shouldn't use a div

Reported by Alex MacCaw | December 24th, 2008 @ 09:39 AM | in 3.x

ActiveRecordHelper shouldn't generate form items in a div (with a class of fieldWithErrors) since a lot of the time these form elements are in a 'p' element (even the Rails scaffolding generates it so) and, since you can't have a block element in an inline one, the page breaks in many browsers (it's standards uncompliant).

In other words, the pages that the Rails scaffold generates at the moment are broken html when they're displaying ActiveRecord errors.

This fixes just changes it to a span element.

Comments and changes to this ticket

  • Arthur Schreiber

    Arthur Schreiber December 24th, 2008 @ 04:09 PM

    The

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

    inside a

    is perfectly fine.

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

  • Arthur Schreiber

    Arthur Schreiber December 24th, 2008 @ 04:11 PM

    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.

  • Alex MacCaw

    Alex MacCaw December 24th, 2008 @ 04:17 PM

    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.

  • Alex MacCaw

    Alex MacCaw December 24th, 2008 @ 05:49 PM

    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:

  • Alex MacCaw

    Alex MacCaw December 24th, 2008 @ 05:50 PM

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

  • Josh Susser

    Josh Susser December 25th, 2008 @ 04:17 AM

    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...

  • Arthur Schreiber

    Arthur Schreiber December 27th, 2008 @ 06:21 PM

    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 Milligan

    Adam Milligan December 27th, 2008 @ 09:56 PM

    • Tag changed from “actionview, helper” to “actionview, helper, patch, tested”

    I agree with Josh and Arthur that the correct behavior would be setting the class attribute on the invalid input element. I also believe that there's no time like the present, so here's a patch that does exactly that.

    I left the #field_errors_proc class accessor on ActionView::Base, but set it to nil by default. I added a separate #field_error_options class accessor that defaults to { :class => 'fieldWithErrors }. This options hash gets merged (carefully) into the HTML options for the invalid input tag.

  • Alex MacCaw

    Alex MacCaw December 27th, 2008 @ 10:47 PM

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

  • Michael Siebert

    Michael Siebert December 28th, 2008 @ 08:44 AM

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

  • Arthur Schreiber

    Arthur Schreiber December 28th, 2008 @ 10:16 AM

    +1 for Adam's patch. :)

  • Carlos Júnior (xjunior)

    Carlos Júnior (xjunior) December 29th, 2008 @ 02:53 AM

    Awesome. I always wanted this :D

  • Carlos Júnior (xjunior)

    Carlos Júnior (xjunior) December 29th, 2008 @ 02:53 AM

    +1

  • Erin Staniland

    Erin Staniland December 30th, 2008 @ 11:53 AM

    +1 for Adam's patch

  • Will

    Will January 1st, 2009 @ 11:33 AM

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

  • Jason Cheow

    Jason Cheow January 1st, 2009 @ 11:33 PM

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

  • Alex MacCaw

    Alex MacCaw January 7th, 2009 @ 03:44 PM

    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?

  • José Valim

    José Valim January 7th, 2009 @ 04:04 PM

    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.

  • Jason Cheow

    Jason Cheow January 8th, 2009 @ 04:17 AM

    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.

  • Alex MacCaw

    Alex MacCaw January 8th, 2009 @ 09:21 AM

    • Tag changed from “actionview, helper, patch, tested” to “actionview, helper, patch, tested, verified”

    Jason, Good point +1 for Adam's patch

    We must have enough +1's by now

  • Wiktor Schmidt

    Wiktor Schmidt January 12th, 2009 @ 10:17 PM

    +1 - very annoying problem

  • José Valim

    José Valim January 18th, 2009 @ 08:36 AM

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

  • Alex MacCaw

    Alex MacCaw January 18th, 2009 @ 10:22 AM

    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.

  • Chris Barnett

    Chris Barnett January 19th, 2009 @ 12:28 PM

    +1 for Adam's patch

  • Mislav

    Mislav January 19th, 2009 @ 02:19 PM

    +1 to this and #1760

  • joost baaij

    joost baaij January 27th, 2009 @ 03:54 PM

    +1 for Adam's patch.

  • DHH

    DHH February 5th, 2009 @ 08:11 PM

    • Assigned user set to “DHH”
  • Tor Erik

    Tor Erik February 5th, 2009 @ 10:00 PM

    +1 for Adam's patch

  • Jonas Schneider

    Jonas Schneider February 9th, 2009 @ 09:32 PM

    +1 for Adam's patch

  • tonycoco

    tonycoco March 24th, 2009 @ 08:09 PM

    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.

  • Walter

    Walter March 24th, 2009 @ 08:25 PM

    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.

  • ara.t.howard

    ara.t.howard April 11th, 2009 @ 07:38 PM

    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.

  • Jim Neath

    Jim Neath April 17th, 2009 @ 04:19 PM

    +1

  • Andrew Vit

    Andrew Vit April 17th, 2009 @ 07:08 PM

    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.

  • Jose Otavio R. Ferreira

    Jose Otavio R. Ferreira June 12th, 2009 @ 05:45 PM

    +1 on Adam's patch.

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

  • Gustavo Delfino

    Gustavo Delfino June 18th, 2009 @ 07:45 PM

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

  • jduff

    jduff June 19th, 2009 @ 08:20 PM

    @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

  • Jess Martin

    Jess Martin August 18th, 2009 @ 07:31 PM

    +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.

  • ronin-75041 (at lighthouseapp)

    ronin-75041 (at lighthouseapp) November 4th, 2009 @ 01:36 PM

    +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.

  • James Le Cuirot

    James Le Cuirot November 11th, 2009 @ 03:46 PM

    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.

  • Harry Brundage

    Harry Brundage December 13th, 2009 @ 07:51 AM

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

  • Martin Schuerrer

    Martin Schuerrer December 15th, 2009 @ 10:09 AM

    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.

  • Martin Schuerrer

    Martin Schuerrer December 15th, 2009 @ 10:11 AM

    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

  • Henrique

    Henrique December 26th, 2009 @ 10:59 PM

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

  • Sharagoz

    Sharagoz April 20th, 2010 @ 08:46 AM

    +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>" }
  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from “2.x” to “3.x”
  • Rohit Arondekar

    Rohit Arondekar June 15th, 2010 @ 11:28 AM

    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?

  • Andrew White

    Andrew White June 15th, 2010 @ 02:00 PM

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

  • Andrew White

    Andrew White June 15th, 2010 @ 02:03 PM

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

  • José Valim

    José Valim June 23rd, 2010 @ 04:24 PM

    • State changed from “new” to “wontfix”

    The main issue was that we had a div inside a

    element. Since scaffold now defaults to

    instead of

    this is no longer an issue. And I don't think we should move this to dynamic_form. dynamic_form just holds the very old form method and error_messages_for.

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer November 8th, 2010 @ 08:25 AM

    • Tag cleared.
    • Importance changed from “” to “Low”

    Automatic cleanup of spam.

  • Victor Costan

    Victor Costan April 10th, 2011 @ 10:35 PM

    • Assigned user changed from “DHH” to “José Valim”

    The issue still impacts people who we want to wrap forms in <p>s. As long as the markup wrapped by the .field_with_errors class consists of inline elements, it would still be better to wrap it with <span>s. Good defaults FTW?

  • Dmitry Polushkin

    Dmitry Polushkin April 10th, 2011 @ 11:42 PM

    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).

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Referenced by

Pages