allow RJS enum values to be passed to visual_effect() and other scriptaculous helpers #4080 [Rick Olson]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@3772 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

5 files changed, 80 insertions, 68 deletions

diff --git a/actionpack/lib/action_view/helpers/prototype_helper.rb b/actionpack/lib/action_view/helpers/prototype_helper.rb
index db36f3de5f..9a58f41821 100644
--- a/actionpack/lib/action_view/helpers/prototype_helper.rb
+++ b/actionpack/lib/action_view/helpers/prototype_helper.rb
@@ -409,7 +409,8 @@ module ActionView
       class JavaScriptGenerator
         def initialize(context, &block) #:nodoc:
           @context, @lines = context, []
-          include_helpers_from_context
+          # removed because those methods were overriding valid generator methods
+          # include_helpers_from_context 
           @context.instance_exec(self, &block)
         end
   
@@ -567,7 +568,7 @@ module ActionView
         
         # Starts a script.aculo.us visual effect. See 
         # ActionView::Helpers::ScriptaculousHelper for more information.
-        def visual_effect(name, id, options = {})
+        def visual_effect(name, id = nil, options = {})
           record @context.send(:visual_effect, name, id, options)
         end
         
@@ -791,7 +792,7 @@ module ActionView
         append_enumerable_function!("zip(#{arguments.collect { |a| a.to_json } * ', '}")
         if block
           function_chain[-1] += ", function(array) {"
-          yield @generator, ActiveSupport::JSON::Variable.new('array')
+          yield ActiveSupport::JSON::Variable.new('array')
           add_return_statement!
           @generator << '});'
         else
@@ -811,7 +812,8 @@ module ActionView
         def enumerable_method(enumerable, variable, yield_params, &block)
           add_variable_assignment!(variable) if variable
           append_enumerable_function!(enumerable)
-          yield *([@generator] + yield_params.collect { |p| JavaScriptVariableProxy.new(@generator, p) })
+          # only yield as many params as were passed in the block
+          yield *yield_params.collect { |p| JavaScriptVariableProxy.new(@generator, p) }[0..block.arity-1]
           add_return_statement! if variable
           @generator << '});'
         end
diff --git a/actionpack/lib/action_view/helpers/scriptaculous_helper.rb b/actionpack/lib/action_view/helpers/scriptaculous_helper.rb
index bd88b3ec0a..263be5d032 100644
--- a/actionpack/lib/action_view/helpers/scriptaculous_helper.rb
+++ b/actionpack/lib/action_view/helpers/scriptaculous_helper.rb
@@ -42,7 +42,7 @@ module ActionView
       # You can change the behaviour with various options, see
       # http://script.aculo.us for more documentation.
       def visual_effect(name, element_id = false, js_options = {})
-        element = element_id ? "'#{element_id}'" : "element"
+        element = element_id ? element_id.to_json : "element"
         
         js_options[:queue] = if js_options[:queue].is_a?(Hash)
           '{' + js_options[:queue].map {|k, v| k == :limit ? "#{k}:#{v}" : "#{k}:'#{v}'" }.join(',') + '}'
@@ -76,7 +76,7 @@ module ActionView
       end
       
       def sortable_element_js(element_id, options = {}) #:nodoc:
-        options[:with]     ||= "Sortable.serialize('#{element_id}')"
+        options[:with]     ||= "Sortable.serialize(#{element_id.to_json})"
         options[:onUpdate] ||= "function(){" + remote_function(options) + "}"
         options.delete_if { |key, value| PrototypeHelper::AJAX_OPTIONS.include?(key) }
   
@@ -87,7 +87,7 @@ module ActionView
         options[:containment] = array_or_string_for_javascript(options[:containment]) if options[:containment]
         options[:only] = array_or_string_for_javascript(options[:only]) if options[:only]
   
-        %(Sortable.create('#{element_id}', #{options_for_javascript(options)});)
+        %(Sortable.create(#{element_id.to_json}, #{options_for_javascript(options)});)
       end
 
       # Makes the element with the DOM ID specified by +element_id+ draggable.
@@ -102,7 +102,7 @@ module ActionView
       end
       
       def draggable_element_js(element_id, options = {}) #:nodoc:
-        %(new Draggable('#{element_id}', #{options_for_javascript(options)});)
+        %(new Draggable(#{element_id.to_json}, #{options_for_javascript(options)});)
       end
 
       # Makes the element with the DOM ID specified by +element_id+ receive
@@ -128,7 +128,7 @@ module ActionView
         options[:accept] = array_or_string_for_javascript(options[:accept]) if options[:accept]    
         options[:hoverclass] = "'#{options[:hoverclass]}'" if options[:hoverclass]
         
-        %(Droppables.add('#{element_id}', #{options_for_javascript(options)});)
+        %(Droppables.add(#{element_id.to_json}, #{options_for_javascript(options)});)
       end
     end
   end
diff --git a/actionpack/test/controller/new_render_test.rb b/actionpack/test/controller/new_render_test.rb
index 24a92d6f9e..1ae742d5ac 100644
--- a/actionpack/test/controller/new_render_test.rb
+++ b/actionpack/test/controller/new_render_test.rb
@@ -358,6 +358,18 @@ class NewRenderTest < Test::Unit::TestCase
     assert_equal "<html>\n  <p>Hello David</p>\n<p>This is grand!</p>\n</html>\n", @response.body
   end
 
+  def test_enum_rjs_test
+    get :enum_rjs_test
+    assert_equal <<-EOS.strip, @response.body
+$$('.product').each(function(value, index) {
+new Effect.Highlight(element,{});
+new Effect.Highlight(value,{});
+Sortable.create(value, {onUpdate:function(){new Ajax.Request('/test/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(value)})}});
+new Draggable(value, {});
+});
+EOS
+  end
+
   def test_render_xml_with_default
     get :greeting
     assert_equal "<p>This is grand!</p>\n", @response.body
@@ -365,17 +377,17 @@ class NewRenderTest < Test::Unit::TestCase
 
   def test_render_rjs_with_default
     get :delete_with_js
-    assert_equal %!["person"].each(Element.remove);\nnew Effect.Highlight('project-4',{});!, @response.body
+    assert_equal %!["person"].each(Element.remove);\nnew Effect.Highlight(\"project-4\",{});!, @response.body
   end
 
   def test_render_rjs_template_explicitly
     get :render_js_with_explicit_template
-    assert_equal %!["person"].each(Element.remove);\nnew Effect.Highlight('project-4',{});!, @response.body
+    assert_equal %!["person"].each(Element.remove);\nnew Effect.Highlight(\"project-4\",{});!, @response.body
   end
 
   def test_rendering_rjs_action_explicitly
     get :render_js_with_explicit_action_template
-    assert_equal %!["person"].each(Element.remove);\nnew Effect.Highlight('project-4',{});!, @response.body
+    assert_equal %!["person"].each(Element.remove);\nnew Effect.Highlight(\"project-4\",{});!, @response.body
   end
 
   def test_layout_rendering
diff --git a/actionpack/test/template/prototype_helper_test.rb b/actionpack/test/template/prototype_helper_test.rb
index 8ae7a98108..d0d30b3d08 100644
--- a/actionpack/test/template/prototype_helper_test.rb
+++ b/actionpack/test/template/prototype_helper_test.rb
@@ -260,52 +260,50 @@ Element.update("baz", "<p>This is a test</p>");
   end
   
   def test_visual_effect
-    assert_equal %(new Effect.Puff('blah',{});), 
+    assert_equal %(new Effect.Puff("blah",{});), 
       @generator.visual_effect(:puff,'blah')
   end 
   
   def test_visual_effect_toggle
-    assert_equal %(Effect.toggle('blah','appear',{});), 
+    assert_equal %(Effect.toggle("blah",'appear',{});), 
       @generator.visual_effect(:toggle_appear,'blah')
   end
   
   def test_sortable
-    assert_equal %(Sortable.create('blah', {onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize('blah')})}});), 
+    assert_equal %(Sortable.create("blah", {onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize("blah")})}});), 
       @generator.sortable('blah', :url => { :action => "order" })
   end
   
   def test_draggable
-    assert_equal %(new Draggable('blah', {});), 
+    assert_equal %(new Draggable("blah", {});), 
       @generator.draggable('blah')
   end
   
   def test_drop_receiving
-    assert_equal %(Droppables.add('blah', {onDrop:function(element){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}});), 
+    assert_equal %(Droppables.add("blah", {onDrop:function(element){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}});), 
       @generator.drop_receiving('blah', :url => { :action => "order" })
   end
 
   def test_collection_proxy_with_each
-    @generator.select('p.welcome b').each do |page, value|
+    @generator.select('p.welcome b').each do |value|
       value.remove_class_name 'selected'
     end
-    @generator.select('p.welcome b').each do |page, value, index|
-      page.call 'alert', index
-      page.call 'alert', value, 'selected'
+    @generator.select('p.welcome b').each do |value, index|
+      @generator.visual_effect :highlight, value
     end
-      assert_equal <<-EOS.strip, @generator.to_s
+    assert_equal <<-EOS.strip, @generator.to_s
 $$('p.welcome b').each(function(value, index) {
 value.removeClassName("selected");
 });
 $$('p.welcome b').each(function(value, index) {
-alert(index);
-alert(value, "selected");
+new Effect.Highlight(value,{});
 });
       EOS
   end
 
   def test_collection_proxy_on_enumerables_with_return_and_index
-    iterator            = Proc.new { |page, value|        page << '(value.className == "welcome")' }
-    iterator_with_index = Proc.new { |page, value, index| page.call 'alert', index ; page << '(value.className == "welcome")' }
+    iterator            = Proc.new { |value|        @generator << '(value.className == "welcome")' }
+    iterator_with_index = Proc.new { |value, index| @generator.call 'alert', index ; @generator << '(value.className == "welcome")' }
     ActionView::Helpers::JavaScriptCollectionProxy::ENUMERABLE_METHODS_WITH_RETURN.each do |enum|
       @generator.select('p').enumerate(enum, 'a', &iterator)
       @generator.select('p').enumerate(enum, 'b', &iterator_with_index)
@@ -324,12 +322,12 @@ return (value.className == "welcome");
   end
 
   def test_collection_proxy_with_grep
-    @generator.select('p').grep 'a', /^a/ do |page, value|
-      page << '(value.className == "welcome")'
+    @generator.select('p').grep 'a', /^a/ do |value|
+      @generator << '(value.className == "welcome")'
     end
-    @generator.select('p').grep 'b', /b$/ do |page, value, index|
-      page.call 'alert', value
-      page << '(value.className == "welcome")'
+    @generator.select('p').grep 'b', /b$/ do |value, index|
+      @generator.call 'alert', value
+      @generator << '(value.className == "welcome")'
     end
 
     assert_equal <<-EOS.strip, @generator.to_s
@@ -344,12 +342,12 @@ return (value.className == "welcome");
   end
 
   def test_collection_proxy_with_inject
-    @generator.select('p').inject 'a', [] do |page, memo, value|
-      page << '(value.className == "welcome")'
+    @generator.select('p').inject 'a', [] do |memo, value|
+      @generator << '(value.className == "welcome")'
     end
-    @generator.select('p').inject 'b', nil do |page, memo, value, index|
-      page.call 'alert', memo
-      page << '(value.className == "welcome")'
+    @generator.select('p').inject 'b', nil do |memo, value, index|
+      @generator.call 'alert', memo
+      @generator << '(value.className == "welcome")'
     end
 
     assert_equal <<-EOS.strip, @generator.to_s
@@ -370,8 +368,8 @@ return (value.className == "welcome");
 
   def test_collection_proxy_with_zip
     ActionView::Helpers::JavaScriptCollectionProxy.new(@generator, '[1, 2, 3]').zip('a', [4, 5, 6], [7, 8, 9])
-    ActionView::Helpers::JavaScriptCollectionProxy.new(@generator, '[1, 2, 3]').zip('b', [4, 5, 6], [7, 8, 9]) do |page, array|
-      page.call 'array.reverse'
+    ActionView::Helpers::JavaScriptCollectionProxy.new(@generator, '[1, 2, 3]').zip('b', [4, 5, 6], [7, 8, 9]) do |array|
+      @generator.call 'array.reverse'
     end
 
     assert_equal <<-EOS.strip, @generator.to_s
diff --git a/actionpack/test/template/scriptaculous_helper_test.rb b/actionpack/test/template/scriptaculous_helper_test.rb
index 0a76f7d652..8ae45af50c 100644
--- a/actionpack/test/template/scriptaculous_helper_test.rb
+++ b/actionpack/test/template/scriptaculous_helper_test.rb
@@ -22,56 +22,56 @@ class ScriptaculousHelperTest < Test::Unit::TestCase
   end
   
   def test_effect
-    assert_equal "new Effect.Highlight('posts',{});", visual_effect(:highlight, "posts")
-    assert_equal "new Effect.Highlight('posts',{});", visual_effect("highlight", :posts)
-    assert_equal "new Effect.Highlight('posts',{});", visual_effect(:highlight, :posts)    
-    assert_equal "new Effect.Fade('fademe',{duration:4.0});", visual_effect(:fade, "fademe", :duration => 4.0)
+    assert_equal "new Effect.Highlight(\"posts\",{});", visual_effect(:highlight, "posts")
+    assert_equal "new Effect.Highlight(\"posts\",{});", visual_effect("highlight", :posts)
+    assert_equal "new Effect.Highlight(\"posts\",{});", visual_effect(:highlight, :posts)    
+    assert_equal "new Effect.Fade(\"fademe\",{duration:4.0});", visual_effect(:fade, "fademe", :duration => 4.0)
     assert_equal "new Effect.Shake(element,{});", visual_effect(:shake)
-    assert_equal "new Effect.DropOut('dropme',{queue:'end'});", visual_effect(:drop_out, 'dropme', :queue => :end)
-    assert_equal "new Effect.DropOut('dropme',{queue:{limit:2,scope:'test',position:'end'}});", 
+    assert_equal "new Effect.DropOut(\"dropme\",{queue:'end'});", visual_effect(:drop_out, 'dropme', :queue => :end)
+    assert_equal "new Effect.DropOut(\"dropme\",{queue:{limit:2,scope:'test',position:'end'}});", 
       visual_effect(:drop_out, 'dropme', :queue => {:position => "end", :scope => "test", :limit => 2})
-    assert_equal "new Effect.DropOut('dropme',{queue:{limit:2,scope:'list'}});", 
+    assert_equal "new Effect.DropOut(\"dropme\",{queue:{limit:2,scope:'list'}});", 
       visual_effect(:drop_out, 'dropme', :queue => {:scope => :list, :limit => 2})
-    assert_equal "new Effect.DropOut('dropme',{queue:{limit:2,scope:'test',position:'end'}});", 
+    assert_equal "new Effect.DropOut(\"dropme\",{queue:{limit:2,scope:'test',position:'end'}});", 
       visual_effect(:drop_out, 'dropme', :queue => {:position => :end, :scope => :test, :limit => 2})
   end
   
   def test_toggle_effects
-    assert_equal "Effect.toggle('posts','appear',{});", visual_effect(:toggle_appear, "posts")
-    assert_equal "Effect.toggle('posts','slide',{});", visual_effect(:toggle_slide, "posts")
-    assert_equal "Effect.toggle('posts','blind',{});", visual_effect(:toggle_blind, "posts")
-    assert_equal "Effect.toggle('posts','appear',{});", visual_effect("toggle_appear", "posts")
-    assert_equal "Effect.toggle('posts','slide',{});", visual_effect("toggle_slide", "posts")
-    assert_equal "Effect.toggle('posts','blind',{});", visual_effect("toggle_blind", "posts")
+    assert_equal "Effect.toggle(\"posts\",'appear',{});", visual_effect(:toggle_appear,  "posts")
+    assert_equal "Effect.toggle(\"posts\",'slide',{});",  visual_effect(:toggle_slide,   "posts")
+    assert_equal "Effect.toggle(\"posts\",'blind',{});",  visual_effect(:toggle_blind,   "posts")
+    assert_equal "Effect.toggle(\"posts\",'appear',{});", visual_effect("toggle_appear", "posts")
+    assert_equal "Effect.toggle(\"posts\",'slide',{});",  visual_effect("toggle_slide",  "posts")
+    assert_equal "Effect.toggle(\"posts\",'blind',{});",  visual_effect("toggle_blind",  "posts")
   end
   
 
   def test_sortable_element
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create('mylist', {onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize('mylist')})}})\n//]]>\n</script>), 
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create(\"mylist\", {onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(\"mylist\")})}})\n//]]>\n</script>), 
       sortable_element("mylist", :url => { :action => "order" })
-    assert_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create('mylist', {constraint:'horizontal', onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize('mylist')})}, tag:'div'})\n//]]>\n</script>), 
+    assert_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create(\"mylist\", {constraint:'horizontal', onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(\"mylist\")})}, tag:'div'})\n//]]>\n</script>), 
       sortable_element("mylist", :tag => "div", :constraint => "horizontal", :url => { :action => "order" })
-    assert_dom_equal %|<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create('mylist', {constraint:'horizontal', containment:['list1','list2'], onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize('mylist')})}})\n//]]>\n</script>|, 
+    assert_dom_equal %|<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create(\"mylist\", {constraint:'horizontal', containment:['list1','list2'], onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(\"mylist\")})}})\n//]]>\n</script>|, 
       sortable_element("mylist", :containment => ['list1','list2'], :constraint => "horizontal", :url => { :action => "order" })
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create('mylist', {constraint:'horizontal', containment:'list1', onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize('mylist')})}})\n//]]>\n</script>), 
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nSortable.create(\"mylist\", {constraint:'horizontal', containment:'list1', onUpdate:function(){new Ajax.Request('http://www.example.com/order', {asynchronous:true, evalScripts:true, parameters:Sortable.serialize(\"mylist\")})}})\n//]]>\n</script>), 
       sortable_element("mylist", :containment => 'list1', :constraint => "horizontal", :url => { :action => "order" })
   end
   
   def test_draggable_element
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nnew Draggable('product_13', {})\n//]]>\n</script>),
-      draggable_element('product_13')
-    assert_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nnew Draggable('product_13', {revert:true})\n//]]>\n</script>),
-      draggable_element('product_13', :revert => true)
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nnew Draggable(\"product_13\", {})\n//]]>\n</script>),
+      draggable_element("product_13")
+    assert_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nnew Draggable(\"product_13\", {revert:true})\n//]]>\n</script>),
+      draggable_element("product_13", :revert => true)
   end
   
   def test_drop_receiving_element
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add('droptarget1', {onDrop:function(element){new Ajax.Request('http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
-      drop_receiving_element('droptarget1')
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add('droptarget1', {accept:'products', onDrop:function(element){new Ajax.Request('http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
-      drop_receiving_element('droptarget1', :accept => 'products')
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add('droptarget1', {accept:'products', onDrop:function(element){new Ajax.Updater('infobox', 'http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
-      drop_receiving_element('droptarget1', :accept => 'products', :update => 'infobox')
-    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add('droptarget1', {accept:['tshirts','mugs'], onDrop:function(element){new Ajax.Updater('infobox', 'http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
-      drop_receiving_element('droptarget1', :accept => ['tshirts','mugs'], :update => 'infobox')
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add(\"droptarget1\", {onDrop:function(element){new Ajax.Request('http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
+      drop_receiving_element("droptarget1")
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add(\"droptarget1\", {accept:'products', onDrop:function(element){new Ajax.Request('http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
+      drop_receiving_element("droptarget1", :accept => 'products')
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add(\"droptarget1\", {accept:'products', onDrop:function(element){new Ajax.Updater('infobox', 'http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
+      drop_receiving_element("droptarget1", :accept => 'products', :update => 'infobox')
+    assert_dom_equal %(<script type=\"text/javascript\">\n//<![CDATA[\nDroppables.add(\"droptarget1\", {accept:['tshirts','mugs'], onDrop:function(element){new Ajax.Updater('infobox', 'http://www.example.com/', {asynchronous:true, evalScripts:true, parameters:'id=' + encodeURIComponent(element.id)})}})\n//]]>\n</script>),
+      drop_receiving_element("droptarget1", :accept => ['tshirts','mugs'], :update => 'infobox')
   end
 end
\ No newline at end of file