fix: simplify style logic in template compiler (#314)

* fix: simplify style logic in template compiler

* fix: remove unused API to fix linting error

* fix: use camelCase instead of kebab-case for css properties

* docs: add comments to clarify the style changes

packages/lwc-engine/src/3rdparty/snabbdom/types.ts

@@ -76,7 +76,7 @@ export interface VNodeData {
   props?: Props;
   attrs?: Attrs;
   class?: Classes;
-  style?: VNodeStyle;
+  style?: VNodeStyle | string;
 //   dataset?: Dataset;
   on?: On;
 //   hero?: Hero;

packages/lwc-engine/src/framework/modules/styles.ts

@@ -2,20 +2,17 @@ import assert from "../assert";
 import {
     isString,
     isUndefined,
-    StringCharCodeAt,
 } from '../language';
 import { EmptyObject } from '../utils';
-import { VNode, Module } from "../../3rdparty/snabbdom/types";
+import { VNode, Module, VNodeStyle } from "../../3rdparty/snabbdom/types";
 import { removeAttribute } from '../dom/element';
 
-const DashCharCode = 45;
-
 function updateStyle(oldVnode: VNode, vnode: VNode) {
-    const { data: { style: newStyle } } = vnode;
+    const { style: newStyle } = vnode.data;
     if (isUndefined(newStyle)) {
         return;
     }
-    let { data: { style: oldStyle } } = oldVnode;
+    let { style: oldStyle } = oldVnode.data;
     if (oldStyle === newStyle) {
         return;
     }
@@ -27,13 +24,14 @@ function updateStyle(oldVnode: VNode, vnode: VNode) {
     let name: string;
     const elm = (vnode.elm as HTMLElement);
     const { style } = elm;
-    if (isUndefined(newStyle) || newStyle as any === '') {
+    if (isUndefined(newStyle) || newStyle === '') {
         removeAttribute.call(elm, 'style');
     } else if (isString(newStyle)) {
+        // The style property is a string when defined via an expression in the template.
         style.cssText = newStyle;
     } else {
         if (!isUndefined(oldStyle)) {
-            for (name in oldStyle) {
+            for (name in oldStyle as VNodeStyle) {
                 if (!(name in newStyle)) {
                     style.removeProperty(name);
                 }
@@ -42,16 +40,13 @@ function updateStyle(oldVnode: VNode, vnode: VNode) {
             oldStyle = EmptyObject;
         }
 
+        // The style property is an object when defined as a string in the template. The compiler
+        // takes care of transforming the inline style into an object. It's faster to set the
+        // different style properties individually instead of via a string.
         for (name in newStyle) {
             const cur = newStyle[name];
-            if (cur !== (oldStyle as any)[name]) {
-                if (StringCharCodeAt.call(name, 0) === DashCharCode && StringCharCodeAt.call(name, 1) === DashCharCode) {
-                    // if the name is prefixed with --, it will be considered a variable, and setProperty() is needed
-                    style.setProperty(name, cur);
-                } else {
-                    // @ts-ignore
-                    style[name] = cur;
-                }
+            if (cur !== (oldStyle as VNodeStyle)[name]) {
+                style[name] = cur;
             }
         }
     }

packages/lwc-template-compiler/package.json

@@ -26,8 +26,6 @@
     "babel-traverse": "^6.26.0",
     "babel-types": "^6.26.0",
     "babylon": "^6.17.0",
-    "camelcase": "^4.1.0",
-    "css-parse": "^2.0.0",
     "decamelize": "^1.2.0",
     "he": "^1.1.1",
     "parse5-with-errors": "^4.0.1"

packages/lwc-template-compiler/src/__tests__/fixtures/base/style-static/expected.js

@@ -8,10 +8,7 @@ export default function tmpl($api, $cmp, $slotset, $ctx) {
                 styleMap: {
                     fontSize: '12px',
                     color: 'red',
-                    marginLeft: '5px',
-                    marginRight: '5px',
-                    marginTop: '10px',
-                    marginBottom: '10px'
+                    margin: '10px 5px 10px'
                 },
                 key: 1
             },

packages/lwc-template-compiler/src/__tests__/parser.spec.ts

@@ -75,10 +75,7 @@ describe('class and style', () => {
         expect(root.children[0].styleMap).toEqual({
             fontSize: '12px',
             color: 'red',
-            marginLeft: '5px',
-            marginRight: '5px',
-            marginTop: '10px',
-            marginBottom: '10px',
+            margin: '10px 5px 10px'
         });
     });
 

packages/lwc-template-compiler/src/parser/__tests__/style.spec.ts

@@ -0,0 +1,68 @@
+import { parseStyleText, parseClassNames } from '../style';
+
+describe('parseStyleText', () => {
+    it('should parse simple style text', () => {
+        const res = parseStyleText('color: blue');
+        expect(res).toEqual({ color: 'blue' });
+    });
+
+    it('should parse simple style text with trailing coma', () => {
+        const res = parseStyleText('color: blue;');
+        expect(res).toEqual({
+            color: 'blue'
+        });
+    });
+
+    it('should parse simple style with multiple values', () => {
+        const res = parseStyleText('box-shadow: 10px 5px 5px black;');
+        expect(res).toEqual({
+            boxShadow: '10px 5px 5px black'
+        });
+    });
+
+    it('should parse multiple declaration', () => {
+        const res = parseStyleText(`font-size: 12px;background: blue; color:red  ;`);
+        expect(res).toEqual({
+            fontSize: '12px',
+            background: 'blue',
+            color: 'red'
+        });
+    });
+
+    it('should parse css functions', () => {
+        const res = parseStyleText(`background-color:rgba(255,0,0,0.3)`);
+        expect(res).toEqual({
+            backgroundColor: 'rgba(255,0,0,0.3)'
+        });
+    });
+
+    it('should support base 64 encoded strings', () => {
+        const image = 'url("data:image/webp;base64,AAAAAAAAAAA")';
+        const res = parseStyleText(`background: ${image}`);
+        expect(res).toEqual({
+            background: image
+        });
+    });
+});
+
+describe('parseClassNames', () => {
+    it('should support a single class', () => {
+        const res = parseClassNames('foo');
+        expect(res).toEqual({ foo: true });
+    });
+
+    it('should support simple class list', () => {
+        const res = parseClassNames('foo bar');
+        expect(res).toEqual({ foo: true, bar: true });
+    });
+
+    it('should support simple class list with trailing spaces', () => {
+        const res = parseClassNames('  foo bar ');
+        expect(res).toEqual({ foo: true, bar: true });
+    });
+
+    it('should support simple class list multiple spaces', () => {
+        const res = parseClassNames('foo  bar');
+        expect(res).toEqual({ foo: true, bar: true });
+    });
+});

packages/lwc-template-compiler/src/parser/index.ts

@@ -29,7 +29,7 @@ import {
 } from './expression';
 
 import {
-    parseStyle,
+    parseStyleText,
     parseClassNames,
 } from './style';
 
@@ -258,7 +258,7 @@ export default function parse(source: string, state: State): {
             if (styleAttribute.type === IRAttributeType.Expression) {
                 element.style = styleAttribute.value;
             } else if (styleAttribute.type === IRAttributeType.String) {
-                element.styleMap = parseStyle(styleAttribute.value);
+                element.styleMap = parseStyleText(styleAttribute.value);
             }
         }
     }