Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #10422 (new enhancement)

Opened 5 months ago

Last modified 2 months ago

[PATCH] Add configuration option for icky hidden form fields div

Reported by: toolmantim Assigned to: core
Priority: normal Milestone: 2.1
Component: ActionPack Version: edge
Severity: normal Keywords: hidden input div
Cc:

Description

Currently the hidden fields output by FormTagHelper are surrounded by an icky div with inline styles: 

<div style="margin:0;padding:0">
  <input type="hidden" name="_method" value="put" />
  <input type="hidden" name="authenticity_token" value="atoken" />
</div>

Attached is a patch (with tests) which adds a ActionView::Base.hidden_fields_tag_proc configuration proc so you can customise this, removing the inline styles if you wish, much the same as you can customise ActionView::Base.field_error_proc

I'm not 100% sure as to why the default wouldn't be a plain div w/o styles, the same as ActionView::Base.field_error_proc, but I've left the default as is unless someone would prefer me to change it.

Attachments

action_view_hidden_fields_tag_proc.diff (4.3 kB) - added by toolmantim on 12/08/07 11:13:48.
Patch with tests
action_view_hidden_fields_tag_proc_no_inline_styles.diff (11.0 kB) - added by toolmantim on 12/17/07 00:35:36.
Patch that removes inline styles and keeps config option
action_view_hidden_fields_tag_proc_no_inline_styles_no_config.diff (8.4 kB) - added by toolmantim on 12/17/07 02:06:37.
action_view_hidden_fields_tag_proc_no_inline_styles_no_config_with_clas.diff (10.4 kB) - added by toolmantim on 01/16/08 01:02:33.
Patch that removes inline styles and replaces with a simple "hidden-fields" class

Change History

12/08/07 11:13:48 changed by toolmantim

  • attachment action_view_hidden_fields_tag_proc.diff added.

Patch with tests

12/08/07 11:41:38 changed by manfred

+1. Looks like a good plan, patch applies and tests run.

12/10/07 06:11:16 changed by dylanegan

+1. I hate those divs.

Don't trust manfred though, he likes to injure people.

12/10/07 07:49:13 changed by chuyeow

  • keywords set to verified.

+1, but I can't see a reason why the hidden <input>s need to be in a <div> or any kind of HTML element. Does anyone know? If not, I'd definitely +1 a change that makes the default not surround the <input>s with anything.

12/10/07 08:13:21 changed by manfred

You put is in a special div because css often contains something like: 

input
{
  margin: 5px;
}

12/10/07 08:23:23 changed by chuyeow

Hmm true, but I very much doubt that affects hidden <input> fields (they are just simply not displayed).

12/10/07 12:02:25 changed by toolmantim

The div is for HTML4.01/XHTML1.0 strict compliance. An input can only be contained by one of "p", "h1", "h2", "h3", "h4", "h5", "h6", "div", "pre", "address", "fieldset", "ins" or "del".

12/10/07 13:51:06 changed by chuyeow

Ah, that makes perfect sense then. Thanks (for letting me learn something today)!

12/16/07 23:39:13 changed by david

I'd rather find out whether this is actually still necessarily to have there or not. Is there any moment of display where this will move stuff around? If not, we should just kill it. But I suspect there is, since that's probably how it got there in the first place.

12/17/07 00:34:30 changed by toolmantim

Well we still need the div, but there's no real need for the @margin:0; padding:0@ inline style as that's most browser's default, as well as any app's stylesheet I've dealt with.

Attached is a patch that removes the inline style but keeps the config option.

12/17/07 00:35:36 changed by toolmantim

  • attachment action_view_hidden_fields_tag_proc_no_inline_styles.diff added.

Patch that removes inline styles and keeps config option

12/17/07 00:40:21 changed by david

What would be the benefit of the config option now? Do we have any use cases left for it? I really don't like these in general.

12/17/07 02:02:24 changed by toolmantim

Yeah good point... I certainly don't have a need. Attached is a patch w/o the config option, and if someone else ever needs it they can revisit the ticket I guess.

12/17/07 02:06:37 changed by toolmantim

  • attachment action_view_hidden_fields_tag_proc_no_inline_styles_no_config.diff added.

12/17/07 02:51:00 changed by toolmantim

Would be good to fix up field_error_proc whilst we're here.

01/06/08 11:15:39 changed by toolmantim

If you're still thinking this is a good idea (I do) can we get this committed? Time to get rid of those icky inline styles. The last patch simply removes the inline styles.

(follow-up: ↓ 15 ) 01/16/08 00:33:35 changed by jbarnette

Why are the inline styles being removed? If the user's doing something like... 

form div {
  padding-bottom: 20px; /* space each section */
}

...not only will autogenerated hidden fields provide a surprising new blank space at the top of the form, but there's no easy cross-browser way (assuming IE6) to restyle the unwanted padding away.

01/16/08 01:02:33 changed by toolmantim

  • attachment action_view_hidden_fields_tag_proc_no_inline_styles_no_config_with_clas.diff added.

Patch that removes inline styles and replaces with a simple "hidden-fields" class

(in reply to: ↑ 14 ) 01/16/08 01:09:59 changed by toolmantim

Replying to jbarnette:

Why are the inline styles being removed? If the user's doing something like... {{{ form div { padding-bottom: 20px; /* space each section */ } }}} ...not only will autogenerated hidden fields provide a surprising new blank space at the top of the form, but there's no easy cross-browser way (assuming IE6) to restyle the unwanted padding away.

A simple class would suffice for this case (though a fieldset would be a much better idea for the example above).

I've added an updated patch which includes a "hidden-fields" class on the div.

02/02/08 03:09:29 changed by bitsweat

  • keywords changed from verified to hidden input div.
  • milestone changed from 2.x to 2.1.

Definitely seems simplest to dispense with xhtml strict compliance and just spit out the hidden fields directly. Removing verified keyword as there's no consensus here.

02/02/08 06:45:19 changed by toolmantim

...which makes this the only part of Rails which makes it impossible to output html4.0 strict and xhtml1.0 strict :(

02/02/08 07:47:27 changed by bitsweat

...which seems like odd priorities, IMO :)

02/07/08 14:27:36 changed by arthurgeek

+1 to keep the div (xhtml compliance) but remove inline style and even a class for it.

03/04/08 18:39:02 changed by brett

What about just switching the style on the div to "display:none"?