Merge pull request #4679 from lest/patch-1

AssetTagHelper improvements

2 files changed, 37 insertions, 27 deletions

diff --git a/actionpack/lib/action_view/helpers/asset_tag_helper.rb b/actionpack/lib/action_view/helpers/asset_tag_helper.rb
index 251b8f8005..280e51549e 100644
--- a/actionpack/lib/action_view/helpers/asset_tag_helper.rb
+++ b/actionpack/lib/action_view/helpers/asset_tag_helper.rb
@@ -355,8 +355,8 @@ module ActionView
       #    <img src="/images/mouse.png" onmouseover="this.src='/images/mouse_over.png'" onmouseout="this.src='/images/mouse.png'" alt="Mouse" />
       #  image_tag("mouse.png", :mouseover => image_path("mouse_over.png")) # =>
       #    <img src="/images/mouse.png" onmouseover="this.src='/images/mouse_over.png'" onmouseout="this.src='/images/mouse.png'" alt="Mouse" />
-      def image_tag(source, options = {})
-        options.symbolize_keys!
+      def image_tag(source, options={})
+        options = options.dup.symbolize_keys!
 
         src = options[:src] = path_to_image(source)
 
@@ -416,22 +416,12 @@ module ActionView
       #  video_tag(["trailer.ogg", "trailer.flv"] :size => "160x120") # =>
       #    <video height="120" width="160"><source src="/videos/trailer.ogg" /><source src="/videos/trailer.flv" /></video>
       def video_tag(*sources)
-        options = sources.extract_options!.symbolize_keys!
-        sources.flatten!
+        multiple_sources_tag('video', sources) do |options|
+          options[:poster] = path_to_image(options[:poster]) if options[:poster]
 
-        options[:poster] = path_to_image(options[:poster]) if options[:poster]
-
-        if size = options.delete(:size)
-          options[:width], options[:height] = size.split("x") if size =~ %r{^\d+x\d+$}
-        end
-
-        if sources.size > 1
-          content_tag("video", options) do
-            safe_join sources.map { |source| tag("source", :src => path_to_video(source)) }
+          if size = options.delete(:size)
+            options[:width], options[:height] = size.split("x") if size =~ %r{^\d+x\d+$}
           end
-        else
-          options[:src] = path_to_video(sources.first)
-          tag("video", options)
         end
       end
 
@@ -449,17 +439,7 @@ module ActionView
       #  audio_tag("sound.wav", "sound.mid")  # =>
       #    <audio><source src="/audios/sound.wav" /><source src="/audios/sound.mid" /></audio>
       def audio_tag(*sources)
-        options = sources.extract_options!.symbolize_keys!
-        sources.flatten!
-
-        if sources.size > 1
-          content_tag("audio", options) do
-            safe_join sources.collect { |source| tag("source", :src => path_to_audio(source)) }
-          end
-        else
-          options[:src] = path_to_audio(sources.first)
-          tag("audio", options)
-        end
+        multiple_sources_tag('audio', sources)
       end
 
       private
@@ -467,6 +447,22 @@ module ActionView
         def asset_paths
           @asset_paths ||= AssetTagHelper::AssetPaths.new(config, controller)
         end
+
+        def multiple_sources_tag(type, sources)
+          options = sources.extract_options!.dup.symbolize_keys!
+          sources.flatten!
+
+          yield options if block_given?
+
+          if sources.size > 1
+            content_tag(type, options) do
+              safe_join sources.map { |source| tag("source", :src => send("path_to_#{type}", source)) }
+            end
+          else
+            options[:src] = send("path_to_#{type}", sources.first)
+            tag(type, options)
+          end
+        end
     end
   end
 end
diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb
index 37153ee960..3422a5efbd 100644
--- a/actionpack/test/template/asset_tag_helper_test.rb
+++ b/actionpack/test/template/asset_tag_helper_test.rb
@@ -445,6 +445,12 @@ class AssetTagHelperTest < ActionView::TestCase
     ImageLinkToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
   end
 
+  def test_image_tag_does_not_modify_options
+    options = {:size => '16x10'}
+    image_tag('icon', options)
+    assert_equal({:size => '16x10'}, options)
+  end
+
   def test_favicon_link_tag
     FaviconLinkToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
   end
@@ -488,6 +494,14 @@ class AssetTagHelperTest < ActionView::TestCase
     AudioLinkToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
   end
 
+  def test_video_audio_tag_does_not_modify_options
+    options = {:autoplay => true}
+    video_tag('video', options)
+    assert_equal({:autoplay => true}, options)
+    audio_tag('audio', options)
+    assert_equal({:autoplay => true}, options)
+  end
+
   def test_timebased_asset_id
     expected_time = File.mtime(File.expand_path(File.dirname(__FILE__) + "/../fixtures/public/images/rails.png")).to_i.to_s
     assert_equal %(<img alt="Rails" src="/images/rails.png?#{expected_time}" />), image_tag("rails.png")