Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Actually parse @keyframes directives.

This adds a KeyframeBlockNode that represents the blocks within a
@keyframe directive. Separating this out allows us to add detection
for invalid @keyframes contents and for invalid selectors that look
like expressions.

Closes #946
  • Loading branch information...
commit f4b07be31558b755371fa426829c9f3188f6a19a 1 parent 19a5ebb
@nex3 nex3 authored
View
7 doc-src/SASS_CHANGELOG.md
@@ -340,6 +340,10 @@ of all directives, but will preserve any CSS rules.
* Allow modulo arithmetic for numbers with compatible units. Thanks to
[Isaac Devine](http://www.devinesystems.co.nz).
+* There's new parser logic for `@keyframes` directives. This will
+ improve the formatting of these directives, as well as catch more
+ errors when using them.
+
### Backwards Incompatibilities -- Must Read!
* Sass will now throw an error when `@extend` is used to extend a selector
@@ -418,6 +422,9 @@ of all directives, but will preserve any CSS rules.
* `percentage()`, `round()`, `ceil()`, `floor()`, and `abs()` now
take arguments named '$number' instead of '$value'.
+* Some invalid selectors now generate errors where before they would
+ be passed on to the generated CSS.
+
## 3.2.14 (Unreleased)
* Don't crash when parsing a directive with no name in the indented syntax.
View
17 lib/sass/engine.rb
@@ -30,6 +30,7 @@
require 'sass/tree/import_node'
require 'sass/tree/charset_node'
require 'sass/tree/at_root_node'
+require 'sass/tree/keyframes_block_node'
require 'sass/tree/visitors/base'
require 'sass/tree/visitors/perform'
require 'sass/tree/visitors/cssize'
@@ -655,11 +656,11 @@ def parse_line(parent, line, root)
parse_mixin_include(line, root)
end
else
- parse_property_or_rule(line)
+ parse_property_or_rule(parent, line)
end
end
- def parse_property_or_rule(line)
+ def parse_property_or_rule(parent, line)
scanner = Sass::Util::MultibyteStringScanner.new(line.text)
hack_char = scanner.scan(/[:\*\.]|\#(?!\{)/)
offset = line.offset
@@ -670,7 +671,11 @@ def parse_property_or_rule(line)
unless (res = parser.parse_interp_ident)
parsed = parse_interp(line.text, line.offset)
- return Tree::RuleNode.new(parsed, full_line_range(line))
+ if parent.is_a?(Tree::DirectiveNode) && parent.name == '@keyframes'
+ return Tree::KeyframesBlockNode.new(parsed)
+ else
+ return Tree::RuleNode.new(parsed, full_line_range(line))
+ end
end
ident_range = Sass::Source::Range.new(
@@ -704,7 +709,11 @@ def parse_property_or_rule(line)
ident_range.start_pos,
Sass::Source::Position.new(@line, to_parser_offset(line.offset) + line.text.length),
@options[:filename], @options[:importer])
- rule = Tree::RuleNode.new(res + interp_parsed, selector_range)
+ if parent.is_a?(Tree::DirectiveNode) && parent.name == '@keyframes'
+ rule = Tree::KeyframesBlockNode.new(res + interp_parsed)
+ else
+ rule = Tree::RuleNode.new(res + interp_parsed, selector_range)
+ end
rule << Tree::CommentNode.new([trailing], :silent) if trailing
rule
end
View
15 lib/sass/scss/css_parser.rb
@@ -16,14 +16,17 @@ def interpolation; nil; end
def use_css_import?; true; end
def block_child(context)
+ old_block_context, @block_context = @block_context, context
case context
- when :ruleset
- declaration
- when :stylesheet
- directive || ruleset
- when :directive
- directive || declaration_or_ruleset
+ when :stylesheet; directive || ruleset
+ when :keyframes; directive || keyframes_block
+ when :keyframes_block; directive || declaration
+ when :ruleset; declaration
+ when :directive; declaration_or_ruleset
+ else raise "[BUG] Unknown block_child context #{context.inspect}"
end
+ ensure
+ @block_context = old_block_context
end
def nested_properties!(node, space)
View
55 lib/sass/scss/parser.rb
@@ -176,7 +176,7 @@ def process_comment(text, node)
DIRECTIVES = Set[:mixin, :include, :function, :return, :debug, :warn, :for,
:each, :while, :if, :else, :extend, :import, :media, :charset, :content,
- :_moz_document, :at_root]
+ :_moz_document, :at_root, :keyframes]
PREFIXED_DIRECTIVES = Set[:supports]
@@ -281,7 +281,8 @@ def for_directive(start_pos)
to = sass_script(:parse)
ss
- block(node(Sass::Tree::ForNode.new(var, from, to, exclusive), start_pos), :directive)
+ block(node(Sass::Tree::ForNode.new(var, from, to, exclusive), start_pos),
+ @block_context || :directive)
end
def each_directive(start_pos)
@@ -299,19 +300,19 @@ def each_directive(start_pos)
list = sass_script(:parse)
ss
- block(node(Sass::Tree::EachNode.new(vars, list), start_pos), :directive)
+ block(node(Sass::Tree::EachNode.new(vars, list), start_pos), @block_context || :directive)
end
def while_directive(start_pos)
expr = sass_script(:parse)
ss
- block(node(Sass::Tree::WhileNode.new(expr), start_pos), :directive)
+ block(node(Sass::Tree::WhileNode.new(expr), start_pos), @block_context || :directive)
end
def if_directive(start_pos)
expr = sass_script(:parse)
ss
- node = block(node(Sass::Tree::IfNode.new(expr), start_pos), :directive)
+ node = block(node(Sass::Tree::IfNode.new(expr), start_pos), @block_context || :directive)
pos = @scanner.pos
line = @line
ss
@@ -331,7 +332,7 @@ def else_block(node)
ss
else_node = block(
node(Sass::Tree::IfNode.new((sass_script(:parse) if tok(/if/))), start_pos),
- :directive)
+ @block_context || :directive)
node.add_else(else_node)
pos = @scanner.pos
line = @line
@@ -530,6 +531,25 @@ def at_root_directive_list
arr
end
+ def keyframes_directive(start_pos)
+ name = expr!(:interp_ident)
+ node = node(Sass::Tree::DirectiveNode.new(['@keyframes '] + name), start_pos)
+ ss
+ block(node, :keyframes)
+ end
+
+ def keyframes_block
+ start_pos = source_position
+ # Keyframes actually support a very restricted subset of all
+ # expressions, but we allow the general case for backwards and
+ # forwards compatibility, and so we get free support for
+ # interpolation.
+ return unless (value = expr)
+ ss
+ node = node(Sass::Tree::KeyframesBlockNode.new(value), start_pos)
+ block(node, :keyframes_block)
+ end
+
# http://www.w3.org/TR/css3-conditional/
def supports_directive(name, start_pos)
condition = expr!(:supports_condition)
@@ -651,9 +671,18 @@ def block_contents(node, context)
end
def block_child(context)
- return variable || directive if context == :function
- return variable || directive || ruleset if context == :stylesheet
- variable || directive || declaration_or_ruleset
+ old_block_context, @block_context = @block_context, context
+ case context
+ when :function; variable || directive
+ when :stylesheet; variable || directive || ruleset
+ when :keyframes; variable || directive || keyframes_block
+ when :keyframes_block; variable || directive || declaration
+ when :ruleset, :directive,
+ :property; variable || directive || declaration_or_ruleset
+ else raise "[BUG] Unknown block_child context #{context}"
+ end
+ ensure
+ @block_context = old_block_context
end
def has_children?(child_or_array)
@@ -775,14 +804,10 @@ def reference_combinator
end
def simple_selector_sequence
- # Returning expr by default allows for stuff like
- # http://www.w3.org/TR/css3-animations/#keyframes-
-
start_pos = source_position
- e = element_name || id_selector ||
+ return unless (e = element_name || id_selector ||
class_selector || placeholder_selector || attrib || pseudo ||
- parent_selector || interpolation_selector
- return expr(!:allow_var) unless e
+ parent_selector || interpolation_selector)
res = [e]
# The tok(/\*/) allows the "E*" hack
View
2  lib/sass/scss/static_parser.rb
@@ -60,7 +60,7 @@ def interp_ident(ident = IDENT); (s = tok(ident)) && [s]; end
def use_css_import?; true; end
def special_directive(name, start_pos)
- return unless %w[media import charset -moz-document].include?(name)
+ return unless %w[media import charset -moz-document keyframes].include?(name)
super
end
View
15 lib/sass/tree/keyframes_block_node.rb
@@ -0,0 +1,15 @@
+module Sass::Tree
+ # A static node reprenting a rule within a `@keyframes` directive.
+ #
+ # @see Sass::Tree
+ class KeyframesBlockNode < Node
+ attr_accessor :value
+
+ attr_accessor :resolved_value
+
+ def initialize(value)
+ @value = value
+ super()
+ end
+ end
+end
View
21 lib/sass/tree/visitors/check_nesting.rb
@@ -122,7 +122,7 @@ def invalid_prop_child?(parent, child)
VALID_PROP_PARENTS = [Sass::Tree::RuleNode, Sass::Tree::PropNode,
Sass::Tree::MixinDefNode, Sass::Tree::DirectiveNode,
- Sass::Tree::MixinNode]
+ Sass::Tree::MixinNode, Sass::Tree::KeyframesBlockNode]
def invalid_prop_parent?(parent, child)
unless is_any_of?(parent, VALID_PROP_PARENTS)
"Properties are only allowed within rules, directives, mixin includes, or other properties." +
@@ -134,6 +134,25 @@ def invalid_return_parent?(parent, child)
"@return may only be used within a function." unless parent.is_a?(Sass::Tree::FunctionNode)
end
+ VALID_KEYFRAMES_CHILDREN = CONTROL_NODES + [Sass::Tree::VariableNode,
+ Sass::Tree::CommentNode,
+ Sass::Tree::KeyframesBlockNode]
+ def invalid_directive_child?(parent, child)
+ return unless parent.name == '@keyframes'
+ unless is_any_of?(child, VALID_KEYFRAMES_CHILDREN)
+ 'Only keyframes blocks (e.g. "15% { ... }") are allowed within @keyframes.'
+ end
+ end
+
+ VALID_KEYFRAMES_BLOCK_CHILDREN = CONTROL_NODES + [Sass::Tree::VariableNode,
+ Sass::Tree::CommentNode,
+ Sass::Tree::PropNode]
+ def invalid_keyframesblock_child?(parent, child)
+ unless is_any_of?(child, VALID_KEYFRAMES_BLOCK_CHILDREN)
+ "Only properties are allowed within @keyframes blocks."
+ end
+ end
+
private
def is_any_of?(val, classes)
View
4 lib/sass/tree/visitors/convert.rb
@@ -103,6 +103,10 @@ def visit_directive(node)
res + yield + "\n"
end
+ def visit_keyframesblock(node)
+ "#{tab_str}#{interp_to_src(node.value).rstrip}#{yield}"
+ end
+
def visit_each(node)
vars = node.vars.map {|var| "$#{dasherize(var)}"}.join(", ")
"#{tab_str}@each #{vars} in #{node.list.to_sass(@options)}#{yield}"
View
5 lib/sass/tree/visitors/deep_copy.rb
@@ -91,6 +91,11 @@ def visit_directive(node)
yield
end
+ def visit_keyframesblock(node)
+ node.value = node.value.map {|c| c.is_a?(Sass::Script::Tree::Node) ? c.deep_copy : c}
+ yield
+ end
+
def visit_media(node)
node.query = node.query.map {|c| c.is_a?(Sass::Script::Tree::Node) ? c.deep_copy : c}
yield
View
8 lib/sass/tree/visitors/perform.rb
@@ -492,6 +492,14 @@ def visit_cssimport(node)
yield
end
+ def visit_keyframesblock(node)
+ node.resolved_value = run_interp(node.value)
+ with_environment Sass::Environment.new(@environment) do
+ node.children = node.children.map {|c| visit(c)}.flatten
+ node
+ end
+ end
+
private
def run_interp_no_strip(text)
View
5 lib/sass/tree/visitors/set_options.rb
@@ -118,6 +118,11 @@ def visit_cssimport(node)
yield
end
+ def visit_keyframesblock(node)
+ node.value.each {|c| c.options = @options if c.is_a?(Sass::Script::Tree::Node)}
+ yield
+ end
+
def visit_supports(node)
node.condition.options = @options
yield
View
23 lib/sass/tree/visitors/to_css.rb
@@ -189,25 +189,12 @@ def visit_directive(node)
node.children.each do |child|
next if child.invisible?
if node.style == :compact
+ output " " unless first
if child.is_a?(Sass::Tree::PropNode)
- with_tabs(first || was_prop ? 0 : @tabs + 1) do
- visit(child)
- output(' ')
- end
+ with_tabs(0) {visit(child)}
else
- if was_prop
- erase! 1
- output "\n"
- end
-
- if first
- lstrip {with_tabs(@tabs + 1) {visit(child)}}
- else
- with_tabs(@tabs + 1) {visit(child)}
- end
-
+ with_tabs(0) {visit(child)}
rstrip!
- output "\n"
end
was_prop = child.is_a?(Sass::Tree::PropNode)
first = false
@@ -246,6 +233,10 @@ def visit_cssimport(node)
visit_directive(node)
end
+ def visit_keyframesblock(node)
+ visit_directive(node)
+ end
+
def visit_prop(node)
return if node.resolved_value.empty?
tab_str = ' ' * (@tabs + node.tabs)
View
24 test/sass/conversion_test.rb
@@ -1795,6 +1795,30 @@ def test_function_var_kwargs_with_list
SCSS
end
+ def test_keyframes
+ assert_renders(<<SASS, <<SCSS)
+@keyframes bounce
+ from
+ top: 100px
+ 25%
+ top: 50px
+ to
+ top: 0px
+SASS
+@keyframes bounce {
+ from {
+ top: 100px;
+ }
+ 25% {
+ top: 50px;
+ }
+ to {
+ top: 0px;
+ }
+}
+SCSS
+ end
+
## Regression Tests
def test_list_in_args
View
27 test/sass/engine_test.rb
@@ -165,6 +165,7 @@ class SassEngineTest < Test::Unit::TestCase
"=simple\n .simple\n color: red\n+simple\n color: blue" => ['Mixin "simple" does not accept a content block.', 4],
"@import \"foo\" // bar" => "Invalid CSS after \"\"foo\" \": expected media query list, was \"// bar\"",
"@at-root\n a: b" => "Properties are only allowed within rules, directives, mixin includes, or other properties.",
+ "10%\n a: b" => ["Invalid CSS after \"\": expected selector, was \"10%\"", 1],
# Regression tests
"a\n b:\n c\n d" => ["Illegal nesting: Only properties may be nested beneath properties.", 3],
@@ -827,7 +828,7 @@ def test_directive
assert_equal("@a {\n #b {\n a: b; }\n #b #c {\n d: e; } }\n",
render("@a\n #b\n :a b\n #c\n :d e"))
- assert_equal("@a { #b { a: b; }\n #b #c { d: e; } }\n",
+ assert_equal("@a { #b { a: b; } #b #c { d: e; } }\n",
render("@a\n #b\n :a b\n #c\n :d e", :style => :compact))
assert_equal("@a {\n #b {\n a: b;\n }\n #b #c {\n d: e;\n }\n}\n",
render("@a\n #b\n :a b\n #c\n :d e", :style => :expanded))
@@ -851,15 +852,33 @@ def test_directive
:g h
END
rendered = <<END
-@a { b: c;
- #d { e: f; }
- g: h; }
+@a { b: c; #d { e: f; } g: h; }
END
assert_equal(rendered, render(to_render, :style => :compact))
assert_equal("@a{b:c;#d{e:f}g:h}\n", render(to_render, :style => :compressed))
end
+ def test_keyframes
+ assert_equal(<<CSS, render(<<SCSS))
+@keyframes bounce {
+ from {
+ top: 100px; }
+ 50% {
+ top: 50px; }
+ to {
+ top: 0px; } }
+CSS
+@keyframes bounce
+ from
+ top: 100px
+ 50%
+ top: 50px
+ to
+ top: 0px
+SCSS
+ end
+
def test_property_hacks
assert_equal(<<CSS, render(<<SASS))
foo {
View
193 test/sass/scss/css_test.rb
@@ -653,6 +653,127 @@ def test_supports
SCSS
end
+ def test_keyframes_nested
+ assert_equal(<<CSS, render(<<SCSS, :style => :nested))
+@keyframes bounce {
+ from {
+ top: 100px; }
+ 50% {
+ top: 50px; }
+ to {
+ top: 0px; } }
+CSS
+@keyframes bounce {
+ from {
+ top: 100px;
+ }
+ 50% {
+ top: 50px;
+ }
+ to {
+ top: 0px;
+ }
+}
+SCSS
+ end
+
+ def test_keyframes_compact
+ assert_equal(<<CSS, render(<<SCSS, :style => :compact))
+@keyframes bounce { from { top: 100px; } 50% { top: 50px; } to { top: 0px; } }
+CSS
+@keyframes bounce {
+ from {
+ top: 100px;
+ }
+ 50% {
+ top: 50px;
+ }
+ to {
+ top: 0px;
+ }
+}
+SCSS
+ end
+
+ def test_keyframes_compressed
+ assert_equal(<<CSS, render(<<SCSS, :style => :compressed))
+@keyframes bounce{from{top:100px}50%{top:50px}to{top:0px}}
+CSS
+@keyframes bounce {
+ from {
+ top: 100px;
+ }
+ 50% {
+ top: 50px;
+ }
+ to {
+ top: 0px;
+ }
+}
+SCSS
+ end
+
+ def test_keyframes_expanded
+ assert_equal(<<CSS, render(<<SCSS, :style => :expanded))
+@keyframes bounce {
+ from {
+ top: 100px;
+ }
+ 50% {
+ top: 50px;
+ }
+ to {
+ top: 0px;
+ }
+}
+CSS
+@keyframes bounce {
+ from {
+ top: 100px;
+ }
+ 50% {
+ top: 50px;
+ }
+ to {
+ top: 0px;
+ }
+}
+SCSS
+ end
+
+ def test_keyframes_with_comments
+ assert_equal(<<CSS, render(<<SCSS))
+@keyframes bounce {
+ from {
+ top: 100px; }
+ /* loud comment */
+ 50% {
+ top: 50px; }
+ 75% {
+ top: 25px; }
+ to {
+ top: 0px; } }
+CSS
+@keyframes /* weird loud comment 1 */ bounce /* weird loud comment 2 */ {
+ // silent comment
+ from {
+ top: 100px;
+ }
+ /* loud comment */
+ 50% {
+ top: 50px;
+ }
+ 75% // weird silent comment
+ {
+ top: 25px;
+ }
+ to /* weird loud comment 3 */ {
+ top: 0px;
+ }
+}
+SCSS
+ end
+
## Selectors
# Taken from http://dev.w3.org/csswg/selectors4/#overview
@@ -887,14 +1008,6 @@ def test_selectors_with_newlines
assert_selector_parses("E, F\nG, H")
end
- def test_expression_fallback_selectors
- assert_selector_parses('0%')
- assert_selector_parses('60%')
- assert_selector_parses('100%')
- assert_selector_parses('12px')
- assert_selector_parses('"foo"')
- end
-
def test_functional_pseudo_selectors
assert_selector_parses(':foo("bar")')
assert_selector_parses(':foo(bar)')
@@ -1014,6 +1127,70 @@ def test_error_with_windows_newlines
assert_equal 1, e.sass_line
end
+ def test_no_properties_in_keyframes
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Invalid CSS after " a: b": expected "{", was ";"
+MESSAGE
+@keyframes bounce {
+ a: b;
+}
+SCSS
+ end
+
+ def test_no_rules_in_keyframes
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Invalid CSS after "...frames bounce {": expected "}", was ".foo {a: b}"
+MESSAGE
+@keyframes bounce {
+ .foo {a: b}
+}
+SCSS
+ end
+
+ def test_no_rules_in_keyframes_rules
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Invalid CSS after " .foo ": expected ":", was "{a: b}"
+MESSAGE
+@keyframes bounce {
+ top {
+ .foo {a: b}
+ }
+}
+SCSS
+ end
+
+ def test_no_media_in_keyframes
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Only keyframes blocks (e.g. "15% { ... }") are allowed within @keyframes.
+MESSAGE
+@keyframes bounce {
+ @media screen {}
+}
+SCSS
+ end
+
+ def test_no_media_in_keyframes_rules
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Only properties are allowed within @keyframes blocks.
+MESSAGE
+@keyframes bounce {
+ top {
+ @media screen {}
+ }
+}
+SCSS
+ end
+
+ def test_no_expression_selectors
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Invalid CSS after "": expected selector or at-rule, was "50% {"
+MESSAGE
+50% {
+ a: b;
+}
+SCSS
+ end
+
## Regressions
def test_double_space_string
View
68 test/sass/scss/scss_test.rb
@@ -663,6 +663,46 @@ def test_unknown_directive_bubbling
SCSS
end
+ def test_keyframes_with_dynamic_values
+ assert_equal(<<CSS, render(<<SCSS))
+@keyframes bounce {
+ 50% {
+ top: 50px; } }
+CSS
+@keyframes bounce {
+ \#{10% + 40%} {
+ top: 50px;
+ }
+}
+SCSS
+ end
+
+ def test_keyframes_with_control_directives
+ assert_equal(<<CSS, render(<<SCSS))
+@keyframes bounce {
+ 10% {
+ top: 100px; }
+ to {
+ top: 50px; } }
+CSS
+@keyframes bounce {
+ @if true {
+ 10% {top: 100px}
+ } @else {
+ 20% {top: 50px}
+ }
+
+ to {
+ @if true {
+ top: 50px;
+ } @else {
+ top: 100px;
+ }
+ }
+}
+SCSS
+ end
+
## Namespace Properties
def test_namespace_properties
@@ -1882,10 +1922,10 @@ def test_function_keyword_splat_must_have_string_keys
def test_basic_selector_interpolation
assert_equal <<CSS, render(<<SCSS)
-foo 3 baz {
+foo a12 baz {
a: b; }
CSS
-foo \#{1 + 2} baz {a: b}
+foo \#{a + 1 + 2} baz {a: b}
SCSS
assert_equal <<CSS, render(<<SCSS)
foo.bar baz {
@@ -2848,6 +2888,28 @@ def test_at_root_plus_extend
## Errors
+ def test_no_extend_in_keyframes
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Only keyframes blocks (e.g. "15% { ... }") are allowed within @keyframes.
+MESSAGE
+@keyframes bounce {
+ @extend %foo;
+}
+SCSS
+ end
+
+ def test_no_extend_in_keyframes_rules
+ assert_raise_message(Sass::SyntaxError, <<MESSAGE.rstrip) {render <<SCSS}
+Only properties are allowed within @keyframes blocks.
+MESSAGE
+@keyframes bounce {
+ top {
+ @extend %foo;
+ }
+}
+SCSS
+ end
+
def test_nested_mixin_def_is_scoped
render <<SCSS
foo {
@@ -3314,7 +3376,7 @@ def test_newlines_removed_from_selectors_when_compressed
def test_if_error_line
assert_raise_line(2) {render(<<SCSS)}
-@if true {foo: bar}
+@if true {a {foo: bar}}
}
SCSS
end
Please sign in to comment.
Something went wrong with that request. Please try again.