From f1511181ea26fa54f6eaa3a56ce1ae8aeb63b2a9 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Sun, 5 May 2019 00:57:45 +0300 Subject: [PATCH 1/2] Rewrite the style part (-50B) --- src/diff/props.js | 30 ++++++++++++++---------------- test/browser/render.test.js | 5 ++--- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/diff/props.js b/src/diff/props.js index be54d277472..f605e0f585c 100644 --- 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'; /** @@ -24,8 +24,6 @@ export function diffProps(dom, newProps, oldProps, isSvg) { } } -let CAMEL_REG = /-?(?=[A-Z])/g; - /** * Set a property value on a DOM node * @param {import('../internal').PreactElement} dom The DOM node to modify @@ -51,24 +49,24 @@ function setProperty(dom, name, value, oldValue, isSvg) { 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, '-'), ''); - } - } + + // Always clear the previous styles + s.cssText = EMPTY_OBJ; + for (let i in value) { 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); + v = typeof v==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (v + 'px') : v; + + // For css vars, just define them with `setProperty`; + if (/^--/.test(i)) { + s.setProperty(i, v); + } + else { + s[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$/, '')); @@ -90,7 +88,7 @@ function setProperty(dom, name, value, oldValue, isSvg) { if (name!==(name = name.replace(/^xlink:?/, ''))) dom.removeAttributeNS('http://www.w3.org/1999/xlink', name.toLowerCase()); else dom.removeAttribute(name); } - else if (typeof value!=='function') { + else if (typeof value!=='function' && name != 'dangerouslySetInnerHTML') { if (name!==(name = name.replace(/^xlink:?/, ''))) dom.setAttributeNS('http://www.w3.org/1999/xlink', name.toLowerCase(), value); else dom.setAttribute(name, value); } diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 49be953e49a..1be013b942a 100644 --- a/test/browser/render.test.js +++ b/test/browser/render.test.js @@ -442,10 +442,9 @@ describe('render()', () => { it('should remove old styles', () => { render(
, scratch); - let s = scratch.firstChild.style; - sinon.spy(s, 'setProperty'); render(
, 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 From 2aa2980a372dbfa38580740f7d9f46a75a027bb7 Mon Sep 17 00:00:00 2001 From: cristianbote Date: Mon, 6 May 2019 00:46:22 +0300 Subject: [PATCH 2/2] Completely remove the support for string styles and update tests --- src/diff/props.js | 39 ++++++++++---------------- test/browser/render.test.js | 55 ++++++------------------------------- 2 files changed, 22 insertions(+), 72 deletions(-) diff --git a/src/diff/props.js b/src/diff/props.js index c5685fd8ea8..caa189c970e 100644 --- a/src/diff/props.js +++ b/src/diff/props.js @@ -38,36 +38,25 @@ 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; - - if (typeof value==='string') { - s.cssText = value; - } - else { - - // Always clear the previous styles - s.cssText = EMPTY_OBJ; - - for (let i in value) { - v = value[i]; - v = typeof v==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (v + 'px') : v; + for (let i in value) { + v = value[i]; + v = typeof v==='number' && IS_NON_DIMENSIONAL.test(i)===false ? (v + 'px') : v; - // For css vars, just define them with `setProperty`; - if (/^--/.test(i)) { - s.setProperty(i, v); - } - else { - s[i] = v; - } + // For css vars, just define them with `setProperty`; + if (/^--/.test(i)) { + dom.style.setProperty(i, v); + } + else { + // Bench for setter vs (setProperty + regex) https://esbench.com/bench/5ccb5a284cd7e6009ef6232e + dom.style[i] = v; } } } diff --git a/test/browser/render.test.js b/test/browser/render.test.js index 1be013b942a..fe647c5c782 100644 --- 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(
, scratch); - expect(scratch.childNodes[0].style.cssText) - .to.equal('top: 5px; position: relative;'); - }); - - it('should not call CSSStyleDeclaration.setProperty for style strings', () => { - render(
, scratch); - sinon.stub(scratch.firstChild.style, 'setProperty'); - render(
, scratch); - expect(scratch.firstChild.style.setProperty).to.not.be.called; - }); - - it('should properly switch from string styles to object styles and back', () => { - render(( -
test
- ), scratch); - - let root = scratch.firstChild; - expect(root.style.cssText).to.equal('display: inline;'); - - render(( -
- ), scratch); - - expect(root.style.cssText).to.equal('color: red;'); - - render(( -
- ), scratch); - - expect(root.style.cssText).to.equal('color: blue;'); - - render(( -
- ), scratch); - - expect(root.style.cssText).to.equal('color: yellow;'); - - render(( -
- ), scratch); - - expect(root.style.cssText).to.equal('display: block;'); - }); - it('should serialize style objects', () => { const styleObj = { color: 'rgb(255, 255, 255)', @@ -441,7 +395,7 @@ describe('render()', () => { }); it('should remove old styles', () => { - render(
, scratch); + render(
, scratch); render(
, scratch); expect(scratch.firstChild.style).to.have.property('color').that.equals(''); expect(scratch.firstChild.style).to.have.property('background').that.equals('blue'); @@ -459,6 +413,13 @@ describe('render()', () => { render(
test
, scratch); expect(sortCss(scratch.firstChild.style.cssText)).to.equal('--foo: 100px; width: var(--foo);'); }); + + it('should call CSSStyleDeclaration.setProperty for css vars', () => { + render(
, scratch); + sinon.stub(scratch.firstChild.style, 'setProperty'); + render(
, scratch); + expect(scratch.firstChild.style.setProperty).to.be.calledWith('--foo', '10px'); + }); } });