Skip to content
This repository

Make the HTML generated for collection fields valid #347

Closed
wants to merge 1 commit into from

3 participants

Massimiliano Filacchioni Carlos Antonio da Silva Rafael Mendonça França
Massimiliano Filacchioni

By removing the for attribute from radio and check box collection main labels using a regular expression. The attribute is not removed if overwritten using :label_html even if this is perhaps useless.

A quick and dirty solution! I don't know SimpleForm enough to be sure it is completely safe, but it works for my forms.

Massimiliano Filacchioni Make the HTML generated for collection fields valid
By removing the for attribute from radio and check box collection main labels
bfda6df
Carlos Antonio da Silva
Collaborator

Ok, the issue really seems to exist, but I think we could work out a more cleaner way to do that =). I'll try to check the problem, meanwhile if you want to give it a try, you can start looking at the builder in view_extensions dir, where I think the problem lies / should be solved. Thanks.

Massimiliano Filacchioni

My approach is dirty, but it is the only solution I can think of that does not require changing the behavior of rails label helper (only for an edge case) nor writing a specific one that would replicate 90% of it!

From my point of view the real (and dangerous) issue is the use of a regular expression. If you can live with that perhaps is better to use it in the label component than in a helper.

What do you think?

Carlos Antonio da Silva
Collaborator

Well, I don't think it's necessary to recreate a label component to do that.

What I have in mind is to intercept the html attributes and give the already sanitized :for option before creating the label, instead of changing the result label string. That's why I was thinking about using this only in the specific places (ie radio/check boxes helpers).

Massimiliano Filacchioni

From what I can remember the rails label helper always generates a for attribute, and we should avoid this for the main label of radio and check box collections. At the moment I am in a hurry and I am not able to double chek, so I can be wrong.

Carlos Antonio da Silva
Collaborator
Rafael Mendonça França
Collaborator

Closed by #372.

@mfila thanks for the test cases

Rafael Mendonça França rafaelfranca closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Oct 11, 2011
Massimiliano Filacchioni Make the HTML generated for collection fields valid
By removing the for attribute from radio and check box collection main labels
bfda6df
This page is out of date. Refresh to see the latest.
6  lib/simple_form/components/labels.rb
@@ -22,7 +22,11 @@ def translate_required_mark
22 22
       end
23 23
 
24 24
       def label
25  
-        @builder.label(label_target, label_text, label_html_options)
  25
+        res = @builder.label(label_target, label_text, label_html_options)
  26
+        if !label_html_options.key?(:for) && input_type =~ /^radio|check_boxes$/
  27
+          res = res.sub(/\sfor="[^"]*"/, '').html_safe
  28
+        end
  29
+        res
26 30
       end
27 31
 
28 32
       def label_text
27  test/components/label_test.rb
@@ -247,6 +247,33 @@ def @controller.action_name; nil; end
247 247
     assert_select 'label[for=project_name]', /Name/
248 248
   end
249 249
 
  250
+  test 'label should include for attribute for select collection' do
  251
+    with_label_for @user, :sex, :select, :collection => [:male, :female]
  252
+    assert_select 'label[for=user_sex]'
  253
+  end
  254
+
  255
+  test 'label should not include for attribute for radio collection' do
  256
+    with_label_for @user, :sex, :radio, :collection => [:male, :female]
  257
+    assert_select 'label'
  258
+    assert_no_select 'label[for=user_sex]'
  259
+  end
  260
+
  261
+  test 'label should include for attribute for radio collection when overwritten' do
  262
+    with_label_for @user, :sex, :radio, :collection => [:male, :female], :label_html => { :for => 'sex' }
  263
+    assert_select 'label[for=sex]'
  264
+  end
  265
+
  266
+  test 'label should not include for attribute for check box collection' do
  267
+    with_label_for @user, :hobbies, :check_boxes, :collection => [:sports, :movies, :shopping]
  268
+    assert_select 'label'
  269
+    assert_no_select 'label[for=user_hobbies]'
  270
+  end
  271
+
  272
+  test 'label should include for attribute for check box collection when overwritten' do
  273
+    with_label_for @user, :hobbies, :check_boxes, :collection => [:sports, :movies, :shopping], :label_html => { :for => 'hobbies' }
  274
+    assert_select 'label[for=hobbies]'
  275
+  end
  276
+
250 277
   test 'label should use i18n properly when object is not present' do
251 278
     store_translations(:en, :simple_form => { :labels => {
252 279
       :project => { :name => 'Nome' }
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.