Use separate scope for variables within a mixin (#10453)

Also contains another fix related to how variable values are replaced in
mixins.

Change-Id: I7a53e0a62041da6557eea10e124e64a55c7823f4

6 files changed, 94 insertions, 8 deletions

diff --git a/theme-compiler/src/com/vaadin/sass/internal/ScssStylesheet.java b/theme-compiler/src/com/vaadin/sass/internal/ScssStylesheet.java
index 2a5d617a6b..6c759167c7 100644
--- a/theme-compiler/src/com/vaadin/sass/internal/ScssStylesheet.java
+++ b/theme-compiler/src/com/vaadin/sass/internal/ScssStylesheet.java
@@ -20,6 +20,7 @@ import java.io.File;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -217,9 +218,7 @@ public class ScssStylesheet extends Node {
 
         node.traverse();
 
-        @SuppressWarnings("unchecked")
-        HashMap<String, VariableNode> variableScope = (HashMap<String, VariableNode>) variables
-                .clone();
+        Map<String, VariableNode> variableScope = openVariableScope();
 
         // the size of the child list may change on each iteration: current node
         // may get deleted and possibly other nodes have been inserted where it
@@ -232,8 +231,7 @@ public class ScssStylesheet extends Node {
             }
         }
 
-        variables.clear();
-        variables.putAll(variableScope);
+        closeVariableScope(variableScope);
 
         // clean up insert point so that processing of the next block will
         // insert after that block
@@ -248,6 +246,34 @@ public class ScssStylesheet extends Node {
         }
     }
 
+    /**
+     * Start a new scope for variables. Any variables set or modified after
+     * opening a new scope are only valid until the scope is closed, at which
+     * time they are replaced with their old values.
+     * 
+     * @return old scope to give to a paired {@link #closeVariableScope(Map)}
+     *         call at the end of the scope (unmodifiable map).
+     */
+    public static Map<String, VariableNode> openVariableScope() {
+        @SuppressWarnings("unchecked")
+        HashMap<String, VariableNode> variableScope = (HashMap<String, VariableNode>) variables
+                .clone();
+        return Collections.unmodifiableMap(variableScope);
+    }
+
+    /**
+     * End a scope for variables, replacing all active variables with those from
+     * the original scope (obtained from {@link #openVariableScope()}).
+     * 
+     * @param originalScope
+     *            original scope
+     */
+    public static void closeVariableScope(
+            Map<String, VariableNode> originalScope) {
+        variables.clear();
+        variables.putAll(originalScope);
+    }
+
     public void removeEmptyBlocks(Node node) {
         // depth first for avoiding re-checking parents of removed nodes
         for (Node child : new ArrayList<Node>(node.getChildren())) {
diff --git a/theme-compiler/src/com/vaadin/sass/internal/tree/MixinNode.java b/theme-compiler/src/com/vaadin/sass/internal/tree/MixinNode.java
index d92e3ebe9d..9fa7e369c0 100644
--- a/theme-compiler/src/com/vaadin/sass/internal/tree/MixinNode.java
+++ b/theme-compiler/src/com/vaadin/sass/internal/tree/MixinNode.java
@@ -18,6 +18,7 @@ package com.vaadin.sass.internal.tree;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Map;
 
 import com.vaadin.sass.internal.ScssStylesheet;
 import com.vaadin.sass.internal.parser.LexicalUnitImpl;
@@ -59,14 +60,23 @@ public class MixinNode extends Node implements IVariableNode {
         this.arglist = arglist;
     }
 
+    /**
+     * Replace variable references with their values in the mixin argument list.
+     */
     @Override
     public void replaceVariables(ArrayList<VariableNode> variables) {
         for (final VariableNode var : variables) {
             for (final LexicalUnitImpl arg : new ArrayList<LexicalUnitImpl>(
                     arglist)) {
-                if (arg.getLexicalUnitType() == LexicalUnitImpl.SCSS_VARIABLE
-                        && arg.getStringValue().equals(var.getName())) {
-                    arg.replaceValue(var.getExpr());
+                LexicalUnitImpl unit = arg;
+                // only perform replace in the value if separate argument name
+                // and value
+                if (unit.getNextLexicalUnit() != null) {
+                    unit = unit.getNextLexicalUnit();
+                }
+                if (unit.getLexicalUnitType() == LexicalUnitImpl.SCSS_VARIABLE
+                        && unit.getStringValue().equals(var.getName())) {
+                    unit.replaceValue(var.getExpr());
                 }
             }
 
@@ -86,8 +96,15 @@ public class MixinNode extends Node implements IVariableNode {
     @Override
     public void traverse() {
         try {
+            // limit variable scope to the mixin
+            Map<String, VariableNode> variableScope = ScssStylesheet
+                    .openVariableScope();
+
             replaceVariables(ScssStylesheet.getVariables());
             MixinNodeHandler.traverse(this);
+
+            ScssStylesheet.closeVariableScope(variableScope);
+
         } catch (Exception e) {
             e.printStackTrace();
         }
diff --git a/theme-compiler/tests/resources/automatic/css/chained_mixins.css b/theme-compiler/tests/resources/automatic/css/chained_mixins.css
new file mode 100644
index 0000000000..73a270a1ab
--- /dev/null
+++ b/theme-compiler/tests/resources/automatic/css/chained_mixins.css
@@ -0,0 +1,3 @@
+.bar-link {
+	a: b;
+}
diff --git a/theme-compiler/tests/resources/automatic/css/mixin_variables.css b/theme-compiler/tests/resources/automatic/css/mixin_variables.css
new file mode 100644
index 0000000000..d879a080b7
--- /dev/null
+++ b/theme-compiler/tests/resources/automatic/css/mixin_variables.css
@@ -0,0 +1,15 @@
+.foo {
+	color: purple;
+}
+
+.baz {
+	color: red;
+}
+
+.foobar {
+	color: green;
+}
+
+.foobaz {
+	color: red;
+}
diff --git a/theme-compiler/tests/resources/automatic/scss/chained_mixins.scss b/theme-compiler/tests/resources/automatic/scss/chained_mixins.scss
new file mode 100644
index 0000000000..007d611149
--- /dev/null
+++ b/theme-compiler/tests/resources/automatic/scss/chained_mixins.scss
@@ -0,0 +1,15 @@
+/* based on reindeer nativebutton */
+
+@mixin foo-link($styleName : bar) {
+
+.#{$styleName}-link {
+	a: b;
+	}
+	
+}
+
+@mixin foo($styleName : bar) {
+	@include foo-link($styleName);
+}
+
+@include foo;
diff --git a/theme-compiler/tests/resources/automatic/scss/mixin_variables.scss b/theme-compiler/tests/resources/automatic/scss/mixin_variables.scss
new file mode 100644
index 0000000000..28cde661dd
--- /dev/null
+++ b/theme-compiler/tests/resources/automatic/scss/mixin_variables.scss
@@ -0,0 +1,10 @@
+@mixin color($color : red, $tag : bar) {
+	.#{$tag} {
+		color: $color;
+	}
+}
+$color : green;
+@include color($color: purple, $tag : foo);
+@include color($tag : baz);
+@include color($color: $color, $tag : foobar);
+@include color($tag : foobaz);