Merge 5fb41c5 into 719a2e8

preactjs · May 7, 2019 · 1007304 · 1007304
2 parents 719a2e8 + 5fb41c5
commit 1007304
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 77 deletions.
diff --git a/src/diff/props.js b/src/diff/props.js
@@ -1,4 +1,4 @@
-import { IS_NON_DIMENSIONAL } from '../constants';
+import { IS_NON_DIMENSIONAL, EMPTY_OBJ } from '../constants';
 import options from '../options';
 
 /**
@@ -27,7 +27,6 @@ export function diffProps(dom, newProps, oldProps, isSvg) {
 	}
 }
 
-const CAMEL_REG = /-?(?=[A-Z])/g;
 const XLINK_NS = 'http://www.w3.org/1999/xlink';
 
 /**
@@ -39,39 +38,28 @@ const XLINK_NS = 'http://www.w3.org/1999/xlink';
  * @param {boolean} isSvg Whether or not this DOM node is an SVG node or not
  */
 function setProperty(dom, name, value, oldValue, isSvg) {
+	let v;
+
 	name = isSvg ? (name==='className' ? 'class' : name) : (name==='class' ? 'className' : name);
 
 	if (name==='style') {
+		// Always clear the previous styles
+		dom.style.cssText = EMPTY_OBJ;
 
-		/* 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;
+		for (let i in value) {
+			v = value[i];
+			v = typeof v==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (v + 'px') : v;
 
-		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 css vars, just define them with `setProperty`;
+			if (/^--/.test(i)) {
+				dom.style.setProperty(i, v);
 			}
-			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);
-				}
+			else {
+				// Bench for setter vs (setProperty + regex) https://esbench.com/bench/5ccb5a284cd7e6009ef6232e
+				dom.style[i] = v;
 			}
 		}
 	}
-	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 +77,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/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,13 @@ 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('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');
+			});
 		}
 	});