From 804860d9df7b97c7823d94ceac706e09514d2c84 Mon Sep 17 00:00:00 2001 From: lukas Date: Sun, 26 Jul 2009 23:55:43 +0200 Subject: [PATCH 1/3] Changed behaviour of how TagHelper#tag_options handles boolean options, so it returns correct HTML options. I.e. if :disabled => false is passed, returns nil and if :disabled => true returns " disabled=\"disabled\"", this works for any boolean option. PS: Also added tests for the escape option. --- actionpack/lib/action_view/helpers/tag_helper.rb | 30 +++++++++++++-------- actionpack/test/template/tag_helper_test.rb | 20 +++++++++++--- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/actionpack/lib/action_view/helpers/tag_helper.rb b/actionpack/lib/action_view/helpers/tag_helper.rb index 66d7592..f63329a 100644 --- a/actionpack/lib/action_view/helpers/tag_helper.rb +++ b/actionpack/lib/action_view/helpers/tag_helper.rb @@ -8,9 +8,6 @@ module ActionView module TagHelper include ERB::Util - BOOLEAN_ATTRIBUTES = %w(disabled readonly multiple checked).to_set - BOOLEAN_ATTRIBUTES.merge(BOOLEAN_ATTRIBUTES.map {|attr| attr.to_sym }) - # Returns an empty HTML tag of type +name+ which by default is XHTML # compliant. Set +open+ to true to create an open tag compatible # with HTML 4.0 and below. Add HTML attributes by passing an attributes @@ -128,19 +125,28 @@ module ActionView "<#{name}#{tag_options}>#{content}" end + # Converts hash to HTML tag options string, special handling has been + # added for boolean and nil options. The results string attributes are + # sorted, so they appear in alphabetical order. + # + # ==== Examples + # tag_options(:value => "office & space") + # # => " value=\"office & space\"" + # + # tag_options(:disabled => true) + # # => " disabled=\"disabled"" + # + # tag_options(:disabled => false, :value => "hello world") + # # => " value=\"hello world\"" def tag_options(options, escape = true) unless options.blank? attrs = [] - if escape - options.each_pair do |key, value| - if BOOLEAN_ATTRIBUTES.include?(key) - attrs << %(#{key}="#{key}") if value - else - attrs << %(#{key}="#{escape_once(value)}") if !value.nil? - end + options.each_pair do |key, value| + if value == true + attrs << %(#{key}="#{key}") + elsif !value.nil? and value != false + attrs << %(#{key}="#{escape ? escape_once(value) : value}") end - else - attrs = options.map { |key, value| %(#{key}="#{value}") } end " #{attrs.sort * ' '}" unless attrs.empty? end diff --git a/actionpack/test/template/tag_helper_test.rb b/actionpack/test/template/tag_helper_test.rb index ef88cae..5703a59 100644 --- a/actionpack/test/template/tag_helper_test.rb +++ b/actionpack/test/template/tag_helper_test.rb @@ -19,8 +19,8 @@ class TagHelperTest < ActionView::TestCase assert_equal "

", tag("p", :ignored => nil) end - def test_tag_options_accepts_false_option - assert_equal "

", tag("p", :value => false) + def test_tag_options_rejects_false_option + assert_equal "

", tag("p", :value => false) end def test_tag_options_accepts_blank_option @@ -28,10 +28,20 @@ class TagHelperTest < ActionView::TestCase end def test_tag_options_converts_boolean_option - assert_equal '

', - tag("p", :disabled => true, :multiple => true, :readonly => true) + assert_equal '

', + tag("p", :autostart => true, :disabled => true, :multiple => true, :readonly => true) end - + + def test_tag_options_escapes_value + assert_equal '

', + tag("p", :disabled => true, :value => 'office & space') + end + + def test_tag_options_allows_to_disabled_escaping + assert_equal '

', + tag("p", { :disabled => true, :value => 'office & space' }, false, false) + end + def test_content_tag assert_equal "Create", content_tag("a", "Create", "href" => "create") assert_equal content_tag("a", "Create", "href" => "create"), -- 1.6.3.2 From e8608dda7895ec6673b7e5aa0732283d8f78e244 Mon Sep 17 00:00:00 2001 From: lukas Date: Mon, 27 Jul 2009 00:55:35 +0200 Subject: [PATCH 2/3] when using `value == true` it threw exceptions, because Mime::Type#== requires .to_sym available on TrueClass... --- actionpack/lib/action_view/helpers/tag_helper.rb | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/actionpack/lib/action_view/helpers/tag_helper.rb b/actionpack/lib/action_view/helpers/tag_helper.rb index f63329a..92a3fe6 100644 --- a/actionpack/lib/action_view/helpers/tag_helper.rb +++ b/actionpack/lib/action_view/helpers/tag_helper.rb @@ -142,7 +142,7 @@ module ActionView unless options.blank? attrs = [] options.each_pair do |key, value| - if value == true + if value.eql?(true) attrs << %(#{key}="#{key}") elsif !value.nil? and value != false attrs << %(#{key}="#{escape ? escape_once(value) : value}") -- 1.6.3.2 From 0fe29b1bd9feda0374fda781457578b1470e48da Mon Sep 17 00:00:00 2001 From: lukas Date: Mon, 27 Jul 2009 00:56:02 +0200 Subject: [PATCH 3/3] updated tests to reflect the new use of #tag_options --- actionpack/test/template/asset_tag_helper_test.rb | 8 ++++---- actionpack/test/template/form_helper_test.rb | 2 +- actionpack/test/template/form_tag_helper_test.rb | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb index 921bfeb..0896394 100644 --- a/actionpack/test/template/asset_tag_helper_test.rb +++ b/actionpack/test/template/asset_tag_helper_test.rb @@ -158,8 +158,8 @@ class AssetTagHelperTest < ActionView::TestCase VideoLinkToTag = { %(video_tag("xml.ogg")) => %(