Add ability to specify attributes for embedded engines

[etdev]

Adds support for the following syntax (as described in Issue #620 by @elstgav):

# embedded css
css [class="my-class" id="my-id"]:
  h1 { color: blue }

which generates:

<style class="my-class" id="my-id" type="text/css">h1 { color: blue }</style>

and

# embedded js
javascript [class="my-class" id="my-id"]:
  console.log('foo');

which generates:

<script class="my-class" id="my-id">console.log('foo');</script>

This is my first PR to this repo so I would highly appreciate any feedback!

[ardecvz]

Hello, Eric!

Some thoughts from a random the Internet guy:

1. I'm not a RegEx's guru but it seems the parser currently catches only square brackets in embedded's attrs such as css [id="my-id"]:. Why not catch other delimiters (and just whitespace) mentioned in tag's attributes? I think it's expected from the embedded to work with attrs like a normal tag. It should be an easy change, especially since you're already using parse_attributes method later in code.
2. Also, do we really need to limit custom attrs to css and js only? As far as I know, embedded engines can't add them so it may be helpful to change attrs in scss, coffee and others. Special for hipsters who can't live without preprocessors in templates ;)
3. It will be super awesome if you add documentation in embedded section of the readme and a corresponding changelog entry. The feature seems already awaited among users and it'll save a few @minad's keystrokes to inform about it.

By the way, thank you very much for your attention to the problem!

[minad]

Hi @etdev,

thx for the pr! I agree with @ardecvz that this should be implemented in a more generic way to support different delimiters and all embedded engines. You could also extend the [:slim, :embedded, ...] s-expression with a field for the attributes (don't use multi here).

Daniel

[etdev]

Thanks a lot @ardecvz and @minad for the advice! I've updated my implementation:

• I added a new field to the Embedded Grammar and am passing the parameters through that instead of using multi.
• It now supports the same delimiters as HTML tags (no longer requires brackets around the attributes).
• I updated the README and CHANGES files (also the Japanese README).
• It now works for any embedded engine that eventually compiles down to TagEngine (JavaScript, CoffeeScript, CSS, SASS, SCSS, LESS, Styl, Opal).
• I added a few new tests (let me know if you would like more).

I didn't attempt to implement this for the remaining embedded engines (markdown, creole, rdoc, etc.) since there doesn't seem to be a one-to-one mapping to a specific tag in those cases, so I wouldn't know where to put the attributes in the result.

Please let me know if you have any more comments/questions!

[etdev]

Hey, any feedback on this? @minad

[minad]

This looks good in general. However I would like to have a solution which also passes options to the underlying tilt engines. Any ideas about that?

[ardecvz]

It seems the PR is stalled and I just wanted to remind you about it.

@minad Personally, I've already found the proposed solution simple and elegant. I don't understand about what options you're talking, could you elaborate more, please?

As far as I understand, there're are two separate entities - Tilt's engine options (such as :pretty option for sass) and plain html attrs (class, id and so on). This PR is about the latter.

There's no reason and it's plainly impossible to pass them to Tilt engines - we just pass templates and capture the output.

As for the former, we already may pass options to tilt engines by something like this configuration string from README - Slim::Embedded.options[:markdown] = {auto_ids: false}. Do you want to pass them in the proposed syntax too? A right offhand example where it could be useful to configure embedded blocks independently is coffescript standard and literal modes but I'm not sure if it's worth an additional burden...

And two additional points to the PR:

1. Preparing for near future Content Security Policy Level 2, how do you see the way users would set inline hash-source CSP? There's no way to access embedded filter's content from template's context so there should be a special handling of this html attribute.
2. It seems it's impossible to override already set in register block attrs.
   Falling with Temple::FilterError: Multiple type attributes specified spec:

def test_override_already_set_in_register_attribute
  source = %q{
css type=false:
}
  assert_html "<style></style>", source
end

In my opinion, it's better to handle multiple embedded's attributes differently than tag's ones as it's completely legitimate to override them at least after register.

[minad]

I don't like how this parsing is done. I would prefer if the parse_attributes method is somehow generalized to work also for the embedded attributes.

[etdev]

I've removed the nil check here, but this requires that all the engines' on_slim_embedded methods take an attrs param.

One alternative is to do something like this:

if @engines[name].is_a?(TagEngine)
  @engines[name].on_slim_embedded(name, body, attrs)
else
  @engines[name].on_slim_embedded(name, body)
end

[etdev]

@minad Thanks for your comments! I've updated the implementation to remove the nil check and use the parse_attributes method directly.

[ardecvz]

1) Preparing for near future Content Security Policy Level 2, how do you see the way users would set inline hash-source CSP? There's no way to access embedded filter's content from template's context so there should be a special handling of this html attribute.
Sorry, Eric, I've wasted your time with the stupid remark confusing CSP hashes with SRI which is pointless with inline content. Forget it.
However, my second point is actual as I'm still wanting to remove type attribute on style in order to pair with the already default omitted in Slim type on script. It seems logical. Thanks!

[holidayworking]

Is there any plan for merge this PR?
I want to add to specify attributes for embedded engines.

[holidayworking]

I read the comment of @ardecvz (#653 (comment)).

However, my second point is actual as I'm still wanting to remove type attribute on style in order to pair with the already default omitted in Slim type on script. It seems logical. Thanks!

In order for this PR to be merged, is it necessary to make the following modifications ?

diff --git a/lib/slim/embedded.rb b/lib/slim/embedded.rb
index 80e5528..05b0c6b 100644
--- a/lib/slim/embedded.rb
+++ b/lib/slim/embedded.rb
@@ -210,17 +210,10 @@ module Slim
 
       def on_slim_embedded(engine, body, attrs)
 
-        unless options[:attributes].empty?
-          options[:attributes].map do|k, v|
-            attrs << [:html, :attr, k, [:static, v]]
-          end
-        end
-
         if options[:engine]
           opts = {}.update(options)
           opts.delete(:engine)
           opts.delete(:tag)
-          opts.delete(:attributes)
           @engine ||= options[:engine].new(opts)
           body = @engine.on_slim_embedded(engine, body, attrs)
         end
@@ -235,7 +228,7 @@ module Slim
     class JavaScriptEngine < TagEngine
       disable_option_validator!
 
-      set_options tag: :script, attributes: {}
+      set_options tag: :script
 
       def on_slim_embedded(engine, body, attrs)
         super(engine, [:html, :js, body], attrs)
@@ -262,10 +255,10 @@ module Slim
     # These engines are executed at compile time
     register :coffee,     JavaScriptEngine, engine: TiltEngine
     register :opal,       JavaScriptEngine, engine: TiltEngine
-    register :less,       TagEngine, tag: :style,  attributes: { type: 'text/css' },         engine: TiltEngine
-    register :styl,       TagEngine, tag: :style,  attributes: { type: 'text/css' },         engine: TiltEngine
-    register :sass,       TagEngine, :pretty, tag: :style, attributes: { type: 'text/css' }, engine: SassEngine
-    register :scss,       TagEngine, :pretty, tag: :style, attributes: { type: 'text/css' }, engine: SassEngine
+    register :less,       TagEngine, tag: :style,          engine: TiltEngine
+    register :styl,       TagEngine, tag: :style,          engine: TiltEngine
+    register :sass,       TagEngine, :pretty, tag: :style, engine: SassEngine
+    register :scss,       TagEngine, :pretty, tag: :style, engine: SassEngine
 
     # These engines are precompiled, code is embedded
     register :erb,        ERBEngine
@@ -274,7 +267,7 @@ module Slim
 
     # Embedded javascript/css
     register :javascript, JavaScriptEngine
-    register :css,        TagEngine, tag: :style,  attributes: { type: 'text/css' }
+    register :css,        TagEngine, tag: :style
 
     # Embedded ruby code
     register :ruby,       RubyEngine
diff --git a/test/core/test_embedded_engines.rb b/test/core/test_embedded_engines.rb
index 449d1b3..afca2d3 100644
--- a/test/core/test_embedded_engines.rb
+++ b/test/core/test_embedded_engines.rb
@@ -116,7 +116,7 @@ wiki:
 css:
   h1 { color: blue }
 }
-  assert_html "<style type=\"text/css\">h1 { color: blue }</style>", source
+  assert_html "<style>h1 { color: blue }</style>", source
   end
 
   def test_render_with_css_empty_attributes
@@ -124,7 +124,7 @@ css:
 css []:
   h1 { color: blue }
 }
-  assert_html "<style type=\"text/css\">h1 { color: blue }</style>", source
+  assert_html "<style>h1 { color: blue }</style>", source
   end
 
   def test_render_with_css_attribute
@@ -132,7 +132,7 @@ css []:
 css scoped = "true":
   h1 { color: blue }
 }
-  assert_html "<style scoped=\"true\" type=\"text/css\">h1 { color: blue }</style>", source
+  assert_html "<style scoped=\"true\">h1 { color: blue }</style>", source
   end
 
   def test_render_with_css_multiple_attributes
@@ -140,7 +140,7 @@ css scoped = "true":
 css class="myClass" scoped = "true" :
   h1 { color: blue }
 }
-  assert_html "<style class=\"myClass\" scoped=\"true\" type=\"text/css\">h1 { color: blue }</style>", source
+  assert_html "<style class=\"myClass\" scoped=\"true\">h1 { color: blue }</style>", source
   end
 
   def test_render_with_javascript
@@ -254,7 +254,7 @@ scss:
   $color: #f00;
   body { color: $color; }
 }
-    assert_html "<style type=\"text/css\">body{color:red}</style>", source
+    assert_html "<style>body{color:red}</style>", source
   end
 
   def test_render_with_scss_attribute
@@ -263,7 +263,7 @@ scss [class="myClass"]:
   $color: #f00;
   body { color: $color; }
 }
-    assert_html "<style class=\"myClass\" type=\"text/css\">body{color:red}</style>", source
+    assert_html "<style class=\"myClass\">body{color:red}</style>", source
   end
 
   def test_render_with_sass
@@ -273,7 +273,7 @@ sass:
   body
     color: $color
 }
-    assert_html "<style type=\"text/css\">body{color:red}</style>", source
+    assert_html "<style>body{color:red}</style>", source
   end
 
   def test_render_with_sass_attribute
@@ -283,7 +283,7 @@ sass [class="myClass"]:
   body
     color: $color
 }
-    assert_html "<style class=\"myClass\" type=\"text/css\">body{color:red}</style>", source
+    assert_html "<style class=\"myClass\">body{color:red}</style>", source
   end
 
 
diff --git a/test/core/test_erb_converter.rb b/test/core/test_erb_converter.rb
index 4c9d93c..303f2ca 100644
--- a/test/core/test_erb_converter.rb
+++ b/test/core/test_erb_converter.rb
@@ -51,7 +51,7 @@ multiline comment-->
 </script><!--[if lt IE 9]>
 <script src="old-ie1.js">
 </script><script src="old-ie2.js">
-</script><![endif]--><style type="text/css">body{background-color:red}
+</script><![endif]--><style>body{background-color:red}
 
 </style>
 </head><body>
diff --git a/test/core/test_pretty.rb b/test/core/test_pretty.rb
index c167d8e..92ee7f4 100644
--- a/test/core/test_pretty.rb
+++ b/test/core/test_pretty.rb
@@ -56,7 +56,7 @@ html
     <script src="old-ie1.js"></script>
     <script src="old-ie2.js"></script>
     <![endif]-->
-    <style type="text/css">
+    <style>
       body {
         background-color: red;
       }

[jcraigk]

Is this PR still being considered? Would love to see it get merged if possible.

[marcelopazzo]

Hey @minad, I'd love to see this PR merged. Anything we can do to help with?
Are the changes proposed by @holidayworking #653 (comment) what you need?

[minad]

Hey, yes, this PR is still considered. Back then it didn't seem ready too me, but we can soon revisit it. @stonean What do you think?

[stonean]

@minad Definitely a need-to-do. Just closed issue #810 referencing this as the main issue to follow. Will follow-up soon with more.

[stonean]

All tests past after resolving some simple merge conflicts.

https://travis-ci.org/slim-template/slim/builds/400183616

Merging into master

[stonean]

Merged into master. Thank you @etdev

All tests passed: https://travis-ci.org/slim-template/slim/builds/400191013

I want to go through more pull requests and issues before releasing a new gem version, but it's available for testing. Thanks again.