Patch (review request) - #1760 - replace scaffold templates <p> with <fieldset> where appropriate

[Charlie Park]

Hopefully this would be a pretty quick patch to review. There's no
real programming involved ... just a slight tweak to the frontend
template generator code.

As I mention in the ticket at Lighthouse (http://
rails.lighthouseapp.com/projects/8994/tickets/1760), the current
scaffold generator templates include paragraph tags <p> in the forms.
A better HTML tag would be the <fieldset> tag.

As the W3C notes (http://www.w3.org/TR/html401/interact/
forms.html#h-17.10), "The FIELDSET element allows authors to group
thematically related controls and labels. Grouping controls makes it
easier for users to understand their purpose while simultaneously
facilitating tabbing navigation for visual user agents and speech
navigation for speech-oriented user agents. The proper use of this
element makes documents more accessible." In the scaffolded templates,
that's what we have with the label/input field: thematically-related
controls.

I'd love any feedback on it, but it seems to me that this is an easy
call. Thoughts?

Thanks!

[Damian Janowski]

On Thu, Jan 15, 2009 at 12:28 AM, Charlie Park <cha...@pearbudget.com> wrote:
> As I mention in the ticket at Lighthouse (http://
> rails.lighthouseapp.com/projects/8994/tickets/1760), the current
> scaffold generator templates include paragraph tags <p> in the forms.
> A better HTML tag would be the <fieldset> tag.
>
> As the W3C notes (http://www.w3.org/TR/html401/interact/
> forms.html#h-17.10), "The FIELDSET element allows authors to group
> thematically related controls and labels. Grouping controls makes it
> easier for users to understand their purpose while simultaneously
> facilitating tabbing navigation for visual user agents and speech
> navigation for speech-oriented user agents. The proper use of this
> element makes documents more accessible." In the scaffolded templates,
> that's what we have with the label/input field: thematically-related
> controls.
>
> I'd love any feedback on it, but it seems to me that this is an easy
> call. Thoughts?

No, label/inputs do not constitute a group of controls. A field set
would make sense if you grouped fields like address, zip code and
country inside a field set with a legend like "Your location".

I'm -1 as this is misleading.

[Charlie Park]

> No, label/inputs do not constitute a group of controls. A field set
> would make sense if you grouped fields like address, zip code and
> country inside a field set with a legend like "Your location".
>
> I'm -1 as this is misleading.

Is this the preferred place for discussion? Or at the Lighthouse
ticket? Posting this to both.

Clearly, the <p> tag is the wrong tag. If an input and a label require
a wrapping container at all, what options do you have? <p> is for
*paragraphs of text*. That's clearly not what the template is
generating here. The other options would seem to be <div> and
<fieldset>. I can see your point about fieldsets being for grouping
more controls together, rather than an input and a related label, but
shouldn't we at least convert that <p> to a <div>?

[Mislav Marohnić]

On Thu, Jan 15, 2009 at 23:26, Charlie Park <cha...@pearbudget.com> wrote:

> No, label/inputs do not constitute a group of controls. A field set
> would make sense if you grouped fields like address, zip code and
> country inside a field set with a legend like "Your location".
>
> I'm -1 as this is misleading.

Clearly, the <p> tag is the wrong tag. If an input and a label require
a wrapping container at all, what options do you have? <p> is for
*paragraphs of text*.

FIELDSET is also wrong here, but +1 for bringing this up. I've hated the P elements there, and they teach wrong HTML usage to under-experienced developers.

What I suggest is "div.field". It should definitely have a classname because IE 6 doesn't have a child CSS selector ("form > div"), so without a classname styling forms would be difficult.

[Damian Janowski]

I agree with that - there should always be a div.field. This is
related to another discussion:
http://groups.google.com/group/rubyonrails-core/browse_thread/thread/a03d3e9d4014683c/3c844528ec21221d

[Charlie Park]

I think <div class="field"> works well, and, as Damian pointed out,
would make the .fieldWithErrors div valid as well.

Also, I e-mailed a few people who work with web standards
professionally, to get their input on it. Here's my conversation with
Jonathan Snook ...

Me: You have a form. It might have a couple of inputs (or it might
just have a single input ... doesn't really matter), and each input
has a corresponding label. You need a container / wrapper for the
input and label, such that they'd be grouped together. How would you
structure that?

Jonathan: I've always stood by the best element designed to divide
elements from one division into another. It's paid in dividends. Ahem,
I think I've played that as long as I could. :) The DIV! ... I know
some people love their list items (but I don't buy that it's a "list"
of form elements any more than I buy that an article is a "list" of
paragraphs.) Some people seem hell bent on subverting definition lists
but we're not defining anything. And some people just think a table is
hunky dory. Well, maybe a table is okay if you have header cells. To
me, DIVs do just fine.

I then sent him a follow-up, noting that consensus seems to be leaning
towards div.field.

Jonathan: I agree on the class="field". It's how I normally do it. I
do it to differentiate from <div class="actions"> where I include
submit buttons (and other form actions).


So ... yeah. Anyone else have any thoughts?

[Charlie Park]

Oh. Also, I wanted to add in Adam Milligan's comment from the
Lighthouse ticket:

"HTML provides the definition list for exactly this scenario. Using dl/
dt/dd seems more appropriate than either p or fieldset."

I guess I can see his reasoning there, but I think the div.field is a
lot cleaner and more intuitive. That is, if the desire is to wrap a
single input and label in some container, a div makes more sense to me
than a dl/dt/dd.

[Michael Twentyman]

Totally in agreement on div.field. I find myself replacing it exactly
this way.

[Mislav Marohnić]

On Sat, Jan 17, 2009 at 12:48, Charlie Park <cha...@pearbudget.com> wrote:

Jonathan: I agree on the class="field". It's how I normally do it. I
do it to differentiate from <div class="actions"> where I include
submit buttons (and other form actions).

Totally agreed with Snook.

div.field for fields and div.actions for submit buttons, "cancel" links and Ajax controls.

I wouldn't go with definition lists here because lots of people, especially usability experts, disagree on their usage here. For Rails core generated markup I'd always stick with DIVs and SPANs with easily understandable classnames.

[Charlie Park]

I'd be happy to re-do my patch (1760), replacing the fieldsets with
appropriately-classed divs, and then soliciting more feedback. Would
that make sense? Should we get more input before I do that? Does it
make sense to wait until things settle down with ticket 1626 first?
I'm new to the world of actually contributing back to Rails, so I'm
not sure what the standard protocol is for the next steps.

[Mislav Marohnić]

On Mon, Jan 19, 2009 at 12:57, Charlie Park <cha...@pearbudget.com> wrote:

I'd be happy to re-do my patch (1760), replacing the fieldsets with
appropriately-classed divs, and then soliciting more feedback. Would
that make sense? Should we get more input before I do that?

The consensus has obviously been reached, so I suggest you update the patch.

 

Does it make sense to wait until things settle down with ticket 1626 first?

No. Your patch changes generated code and #1626 changes the form builder - those are two separate issues. Even if we replace P with DIVs (your patch) I still support wrapping fields with errors in SPANs.