Merge a9b8948 into a23b921

preactjs · May 9, 2019 · 81daf0d · 81daf0d
2 parents a23b921 + a9b8948
commit 81daf0d
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 81 deletions.
diff --git a/src/diff/props.js b/src/diff/props.js
@@ -1,5 +1,6 @@
-import { IS_NON_DIMENSIONAL } from '../constants';
+import { IS_NON_DIMENSIONAL, EMPTY_OBJ } from '../constants';
 import options from '../options';
+import { assign } from '../util';
 
 /**
  * Diff the old and new properties of a VNode and apply changes to the DOM node
@@ -27,7 +28,7 @@ export function diffProps(dom, newProps, oldProps, isSvg) {
 	}
 }
 
-const CAMEL_REG = /-?(?=[A-Z])/g;
+const CAMEL_REG = /[A-Z]/g;
 const XLINK_NS = 'http://www.w3.org/1999/xlink';
 
 /**
@@ -42,36 +43,21 @@ function setProperty(dom, name, value, oldValue, isSvg) {
 	name = isSvg ? (name==='className' ? 'class' : name) : (name==='class' ? 'className' : name);
 
 	if (name==='style') {
-
-		/* Possible golfing activities for setting styles:
-		 *   - we could just drop String style values. They're not supported in other VDOM libs.
-		 *   - assigning to .style sets .style.cssText - TODO: benchmark this, might not be worth the bytes.
-		 *   - assigning also casts to String, and ignores invalid values. This means assigning an Object clears all styles.
-		 */
-		let s = dom.style;
-
-		if (typeof value==='string') {
-			s.cssText = value;
-		}
-		else {
-			if (typeof oldValue==='string') s.cssText = '';
-			else {
-				// remove values not in the new list
-				for (let i in oldValue) {
-					if (value==null || !(i in value)) s.setProperty(i.replace(CAMEL_REG, '-'), '');
-				}
-			}
-			for (let i in value) {
-				const v = value[i];
-				if (oldValue==null || v!==oldValue[i]) {
-					s.setProperty(i.replace(CAMEL_REG, '-'), typeof v==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (v + 'px') : v);
-				}
+		const set = assign(assign({}, oldValue), value);
+		for (let i in set) {
+			if ((value || EMPTY_OBJ)[i] === (oldValue || EMPTY_OBJ)[i]) {
+				continue;
 			}
+			dom.style.setProperty(
+				(i[0] === '-' && i[1] === '-') ? i : i.replace(CAMEL_REG, '-$&'),
+				(value && (i in value))
+					? (typeof set[i]==='number' && IS_NON_DIMENSIONAL.test(i)===false)
+						? set[i] + 'px'
+						: set[i]
+					: ''
+			);
 		}
 	}
-	else if (name==='dangerouslySetInnerHTML') {
-		return;
-	}
 	// Benchmark for comparison: https://esbench.com/bench/574c954bdb965b9a00965ac6
 	else if (name[0]==='o' && name[1]==='n') {
 		let useCapture = name !== (name=name.replace(/Capture$/, ''));
@@ -89,7 +75,7 @@ function setProperty(dom, name, value, oldValue, isSvg) {
 	else if (name!=='list' && name!=='tagName' && !isSvg && (name in dom)) {
 		dom[name] = value==null ? '' : value;
 	}
-	else if (typeof value!=='function') {
+	else if (typeof value!=='function' && name!=='dangerouslySetInnerHTML') {
 		if (name!==(name = name.replace(/^xlink:?/, ''))) {
 			if (value==null || value===false) {
 				dom.removeAttributeNS(XLINK_NS, name.toLowerCase());

diff --git a/test/_util/helpers.js b/test/_util/helpers.js
@@ -139,7 +139,18 @@ export function sortCss(cssText) {
 	return cssText.split(';')
 		.filter(Boolean)
 		.map(s => s.replace(/^\s+|\s+$/g, '').replace(/(\s*:\s*)/g, ': '))
-		.sort((a, b) => a[0]==='-' ? -1 : b[0]==='-' ? 1 : a.localeCompare(b))
+		.sort((a, b) => {
+			// CSS Variables are typically positioned at the start
+			if (a[0]==='-') {
+				// If both are a variable we just compare them
+				if (b[0]==='-') return a.localeCompare(b);
+				return -1;
+			}
+			// b is a css var
+			if (b[0]==='-') return 1;
+
+			return a.localeCompare(b);
+		})
 		.join('; ') + ';';
 }
 

diff --git a/test/browser/render.test.js b/test/browser/render.test.js
@@ -336,52 +336,6 @@ describe('render()', () => {
 	});
 
 	describe('style attribute', () => {
-		it('should apply style as String', () => {
-			render(<div style="top: 5px; position: relative;" />, scratch);
-			expect(scratch.childNodes[0].style.cssText)
-				.to.equal('top: 5px; position: relative;');
-		});
-
-		it('should not call CSSStyleDeclaration.setProperty for style strings', () => {
-			render(<div style="top: 5px; position: relative;" />, scratch);
-			sinon.stub(scratch.firstChild.style, 'setProperty');
-			render(<div style="top: 10px; position: absolute;" />, scratch);
-			expect(scratch.firstChild.style.setProperty).to.not.be.called;
-		});
-
-		it('should properly switch from string styles to object styles and back', () => {
-			render((
-				<div style="display: inline;">test</div>
-			), scratch);
-
-			let root = scratch.firstChild;
-			expect(root.style.cssText).to.equal('display: inline;');
-
-			render((
-				<div style={{ color: 'red' }} />
-			), scratch);
-
-			expect(root.style.cssText).to.equal('color: red;');
-
-			render((
-				<div style="color: blue" />
-			), scratch);
-
-			expect(root.style.cssText).to.equal('color: blue;');
-
-			render((
-				<div style={{ color: 'yellow' }} />
-			), scratch);
-
-			expect(root.style.cssText).to.equal('color: yellow;');
-
-			render((
-				<div style="display: block" />
-			), scratch);
-
-			expect(root.style.cssText).to.equal('display: block;');
-		});
-
 		it('should serialize style objects', () => {
 			const styleObj = {
 				color: 'rgb(255, 255, 255)',
@@ -441,11 +395,10 @@ describe('render()', () => {
 		});
 
 		it('should remove old styles', () => {
-			render(<div style="color: red;" />, scratch);
-			let s = scratch.firstChild.style;
-			sinon.spy(s, 'setProperty');
+			render(<div style={{ color: 'red' }} />, scratch);
 			render(<div style={{ background: 'blue' }} />, scratch);
-			expect(s.setProperty).to.be.calledOnce;
+			expect(scratch.firstChild.style).to.have.property('color').that.equals('');
+			expect(scratch.firstChild.style).to.have.property('background').that.equals('blue');
 		});
 
 		// Skip test if the currently running browser doesn't support CSS Custom Properties
@@ -460,6 +413,18 @@ describe('render()', () => {
 				render(<div style={{ '--foo': '100px', width: 'var(--foo)' }}>test</div>, scratch);
 				expect(sortCss(scratch.firstChild.style.cssText)).to.equal('--foo: 100px; width: var(--foo);');
 			});
+
+			it('css vars should not be transformed into dash-separated', () => {
+				render(<div style={{ '--fooBar': 1, '--foo-baz': 2, opacity: 'var(--fooBar)', zIndex: 'var(--foo-baz)' }}>test</div>, scratch);
+				expect(sortCss(scratch.firstChild.style.cssText)).to.equal('--foo-baz: 2; --fooBar: 1; opacity: var(--fooBar); z-index: var(--foo-baz);');
+			});
+
+			it('should call CSSStyleDeclaration.setProperty for css vars', () => {
+				render(<div style={{ padding: '10px' }} />, scratch);
+				sinon.stub(scratch.firstChild.style, 'setProperty');
+				render(<div style={{ '--foo': '10px', padding: 'var(--foo)' }} />, scratch);
+				expect(scratch.firstChild.style.setProperty).to.be.calledWith('--foo', '10px');
+			});
 		}
 	});