Skip to content
This repository
Browse code

Adds a audio_tag helper for the HTML5 audio tag. Fixed video_path doc…

…s. HTML attributes values should be true or false not attribute's name. [#2864 state:resolved]

Signed-off-by: Yehuda Katz <wycats@yehuda-katzs-macbookpro41.local>
  • Loading branch information...
commit 1e2d7229602f467cfdc0ef606b5ef8a5566a1501 1 parent 61604fe
Emilio Tagua authored Yehuda Katz committed
38  actionpack/lib/action_view/helpers/asset_tag_helper.rb
@@ -462,12 +462,27 @@ def image_path(source)
462 462
       #   video_path("hd")                                            # => /videos/hd
463 463
       #   video_path("hd.avi")                                        # => /videos/hd.avi
464 464
       #   video_path("trailers/hd.avi")                               # => /videos/trailers/hd.avi
465  
-      #   video_path("/trailers/hd.avi")                              # => /videos/hd.avi
  465
+      #   video_path("/trailers/hd.avi")                              # => /trailers/hd.avi
466 466
       #   video_path("http://www.railsapplication.com/vid/hd.avi") # => http://www.railsapplication.com/vid/hd.avi
467 467
       def video_path(source)
468 468
         compute_public_path(source, 'videos')
469 469
       end
470  
-      alias_method :path_to_video, :video_path # aliased to avoid conflicts with an video_path named route
  470
+      alias_method :path_to_video, :video_path # aliased to avoid conflicts with a video_path named route
  471
+
  472
+      # Computes the path to an audio asset in the public audios directory.
  473
+      # Full paths from the document root will be passed through.
  474
+      # Used internally by +audio_tag+ to build the audio path.
  475
+      #
  476
+      # ==== Examples
  477
+      #   audio_path("horse")                                            # => /audios/horse
  478
+      #   audio_path("horse.wav")                                        # => /audios/horse.avi
  479
+      #   audio_path("sounds/horse.wav")                                 # => /audios/sounds/horse.avi
  480
+      #   audio_path("/sounds/horse.wav")                                # => /sounds/horse.avi
  481
+      #   audio_path("http://www.railsapplication.com/sounds/horse.wav") # => http://www.railsapplication.com/sounds/horse.wav
  482
+      def audio_path(source)
  483
+        compute_public_path(source, 'audios')
  484
+      end
  485
+      alias_method :path_to_audio, :audio_path # aliased to avoid conflicts with an audio_path named route
471 486
 
472 487
       # Returns an html image tag for the +source+. The +source+ can be a full
473 488
       # path or a file that exists in your public images directory.
@@ -542,7 +557,7 @@ def image_tag(source, options = {})
542 557
       #  video_tag("trailer.ogg")  # =>
543 558
       #    <video src="/videos/trailer.ogg" />
544 559
       #  video_tag("trailer.ogg", :controls => true, :autobuffer => true)  # =>
545  
-      #    <video autobuffer="autobuffer" controls="controls" src="/videos/trailer.ogg" />
  560
+      #    <video autobuffer="true" controls="true" src="/videos/trailer.ogg" />
546 561
       #  video_tag("trailer.m4v", :size => "16x10", :poster => "screenshot.png")  # =>
547 562
       #    <video src="/videos/trailer.m4v" width="16" height="10" poster="/images/screenshot.png" />
548 563
       #  video_tag("/trailers/hd.avi", :size => "16x16")  # =>
@@ -572,6 +587,23 @@ def video_tag(sources, options = {})
572 587
         end
573 588
       end
574 589
 
  590
+      # Returns an html audio tag for the +source+.
  591
+      # The +source+ can be full path or file that exists in
  592
+      # your public audios directory.
  593
+      #
  594
+      # ==== Examples
  595
+      #  audio_tag("sound")  # =>
  596
+      #    <audio src="/audios/sound" />
  597
+      #  audio_tag("sound.wav")  # =>
  598
+      #    <audio src="/audios/sound.wav" />
  599
+      #  audio_tag("sound.wav", :autoplay => true, :controls => true)  # =>
  600
+      #    <audio autoplay="autoplay" controls="controls" src="/audios/sound.wav" />
  601
+      def audio_tag(source, options = {})
  602
+        options.symbolize_keys!
  603
+        options[:src] = path_to_audio(source)
  604
+        tag("audio", options)
  605
+      end
  606
+
575 607
       def self.cache_asset_timestamps
576 608
         @@cache_asset_timestamps
577 609
       end
3  actionpack/lib/action_view/helpers/tag_helper.rb
@@ -8,8 +8,7 @@ module Helpers #:nodoc:
8 8
     module TagHelper
9 9
       include ERB::Util
10 10
 
11  
-      BOOLEAN_ATTRIBUTES = %w(disabled readonly multiple checked autobuffer
12  
-                           autoplay controls loop).to_set
  11
+      BOOLEAN_ATTRIBUTES = %w(disabled readonly multiple checked).to_set
13 12
       BOOLEAN_ATTRIBUTES.merge(BOOLEAN_ATTRIBUTES.map {|attr| attr.to_sym })
14 13
 
15 14
       # Returns an empty HTML tag of type +name+ which by default is XHTML
40  actionpack/test/template/asset_tag_helper_test.rb
@@ -158,8 +158,8 @@ def teardown
158 158
 
159 159
   VideoLinkToTag = {
160 160
     %(video_tag("xml.ogg")) => %(<video src="/videos/xml.ogg" />),
161  
-    %(video_tag("rss.m4v", :autoplay => true, :controls => true)) => %(<video autoplay="autoplay" controls="controls" src="/videos/rss.m4v" />),
162  
-    %(video_tag("rss.m4v", :autobuffer => true)) => %(<video autobuffer="autobuffer" src="/videos/rss.m4v" />),
  161
+    %(video_tag("rss.m4v", :autoplay => true, :controls => true)) => %(<video autoplay="true" controls="true" src="/videos/rss.m4v" />),
  162
+    %(video_tag("rss.m4v", :autobuffer => true)) => %(<video autobuffer="true" src="/videos/rss.m4v" />),
163 163
     %(video_tag("gold.m4v", :size => "160x120")) => %(<video height="120" src="/videos/gold.m4v" width="160" />),
164 164
     %(video_tag("gold.m4v", "size" => "320x240")) => %(<video height="240" src="/videos/gold.m4v" width="320" />),
165 165
     %(video_tag("trailer.ogg", :poster => "screenshot.png")) => %(<video poster="/images/screenshot.png" src="/videos/trailer.ogg" />),
@@ -168,7 +168,27 @@ def teardown
168 168
     %(video_tag("error.avi", "size" => "x")) => %(<video src="/videos/error.avi" />),
169 169
     %(video_tag("http://media.rubyonrails.org/video/rails_blog_2.mov")) => %(<video src="http://media.rubyonrails.org/video/rails_blog_2.mov" />),
170 170
     %(video_tag(["multiple.ogg", "multiple.avi"])) => %(<video><source src="multiple.ogg" /><source src="multiple.avi" /></video>),
171  
-    %(video_tag(["multiple.ogg", "multiple.avi"], :size => "160x120", :controls => true)) => %(<video controls="controls" height="120" width="160"><source src="multiple.ogg" /><source src="multiple.avi" /></video>)
  171
+    %(video_tag(["multiple.ogg", "multiple.avi"], :size => "160x120", :controls => true)) => %(<video controls="true" height="120" width="160"><source src="multiple.ogg" /><source src="multiple.avi" /></video>)
  172
+  }
  173
+
  174
+ AudioPathToTag = {
  175
+    %(audio_path("xml"))          => %(/audios/xml),
  176
+    %(audio_path("xml.wav"))      => %(/audios/xml.wav),
  177
+    %(audio_path("dir/xml.wav"))  => %(/audios/dir/xml.wav),
  178
+    %(audio_path("/dir/xml.wav")) => %(/dir/xml.wav)
  179
+  }
  180
+
  181
+  PathToAudioToTag = {
  182
+    %(path_to_audio("xml"))          => %(/audios/xml),
  183
+    %(path_to_audio("xml.wav"))      => %(/audios/xml.wav),
  184
+    %(path_to_audio("dir/xml.wav"))  => %(/audios/dir/xml.wav),
  185
+    %(path_to_audio("/dir/xml.wav")) => %(/dir/xml.wav)
  186
+  }
  187
+
  188
+  AudioLinkToTag = {
  189
+    %(audio_tag("xml.wav")) => %(<audio src="/audios/xml.wav" />),
  190
+    %(audio_tag("rss.wav", :autoplay => true, :controls => true)) => %(<audio autoplay="true" controls="true" src="/audios/rss.wav" />),
  191
+    %(audio_tag("http://media.rubyonrails.org/audio/rails_blog_2.mov")) => %(<audio src="http://media.rubyonrails.org/audio/rails_blog_2.mov" />),
172 192
   }
173 193
 
174 194
   def test_auto_discovery_link_tag
@@ -311,6 +331,18 @@ def test_video_tag
311 331
     VideoLinkToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
312 332
   end
313 333
 
  334
+  def test_audio_path
  335
+    AudioPathToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
  336
+  end
  337
+
  338
+  def test_path_to_audio_alias_for_audio_path
  339
+    PathToAudioToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
  340
+  end
  341
+
  342
+  def test_audio_tag
  343
+    AudioLinkToTag.each { |method, tag| assert_dom_equal(tag, eval(method)) }
  344
+  end
  345
+
314 346
   def test_timebased_asset_id
315 347
     expected_time = File.stat(File.expand_path(File.dirname(__FILE__) + "/../fixtures/public/images/rails.png")).mtime.to_i.to_s
316 348
     assert_equal %(<img alt="Rails" src="/images/rails.png?#{expected_time}" />), image_tag("rails.png")
@@ -354,7 +386,7 @@ def test_caching_image_path_with_caching_and_proc_asset_host_using_request
354 386
         "#{request.protocol}assets#{source.length}.example.com"
355 387
       end
356 388
     end
357  
-    
  389
+
358 390
     ActionController::Base.perform_caching = true
359 391
 
360 392
 

5 notes on commit 1e2d722

Tieg Zaharia
tiegz commented on 1e2d722

Just caught this change to the video_tag() changes that moved its attributes out of BOOLEAN_ATTRIBUTES. Those attributes, according to the HTML5 spec draft, are boolean attributes (ie "The autobuffer attribute is a boolean attribute...").

According to the spec (http://dev.w3.org/html5/spec/Overview.html#boolean-attributes):

A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

I thought the presence of BOOLEAN_ATTRIBUTES was to follow this standard, so I just wanted to point this out. Not sure if this applies anywhere else in ActionView.

Tieg Zaharia
tiegz commented on 1e2d722

Although XHTML2 will be discontinued in favor of HTML5, it should be noted that XHTML2 also seems to follow this standard (ie http://www.w3.org/TR/xhtml2/mod-object.html#adef_object_declare).

Marc Love

+1 for tiegz & bugmenot

I left a comment on the ticket as well:
https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2864-patch-add-audio_tag-to-actionview

What's the best way to correct the problem? revert this commit and replace it with a new patch OR a new ticket and patch that will repair what this commit broke?

Lukas Westermann
lwe commented on 1e2d722

How about changing ActionView::Helpers::TagHelper#tag_options to handle boolean arguments differently? So that tag_options(:disabled => true) would render to disabled="disabled" and tag_options(:disabled => false) to "", just as in the HTML docs. However, I'm not sure what the implications would be for existing applications, that depend on the fact that currently tag_options(:foo => true) returns foo="true".

Roman Le Négrate

lwe: +1.

Marc Love

Fixed for HTML5 compliant output in future ticket.

Marc Love

Fixed to produce only standards-compliant XHTML/HTML5 for boolean attributes. autoplay="autoplay" and controls="controls" instead of values of "true". Fix will be submitted in future commit.

Marc Love

Fixed to produce only standards-compliant XHTML/HTML5 for boolean attributes. autobuffer="autobuffer" instead of values of autobuffer="true". Fix will be submitted in future commit.

Marc Love

Fixed to produce only standards-compliant XHTML/HTML5 for boolean attributes. autoplay="autoplay" and controls="controls" instead of values of "true". Fix will be submitted in future commit.

Please sign in to comment.
Something went wrong with that request. Please try again.