From cb6a664862af51ef3742722e4bd653d070163dd1 Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Mon, 6 Mar 2017 11:46:51 -0800 Subject: [PATCH] Preserve style order in prefixProperty While attempting to update Aphrodite to inline-style-prefixer 3.0.0, I ran into an issue with prefixAll putting the prefixes in the wrong order. Specifically, they came after the un-prefixed style, which is not what we want because we want the standard style to have precedence in browsers that have it implemented. https://github.com/Khan/aphrodite/pull/205 After a little bit of digging, I found that this was caused by the way prefixProperty was designed. To ensure that the prefixed styles are injected in the correct spot, we need to build a new object key-by-key and return it instead of mutating the style object that was passed in. This is likely to cause a performance hit if there are multiple styles to be prefixed in the same object. It might be worth making a pass to optimize this so all of the styles can be prefixed in one pass, but I'm going to leave that for another time. Although Object ordering is not guaranteed, it is generally sticky in most browsers so this seems like an improvement but not a total fix. This reliance on Object ordering will likely cause issues (different style order) when used on the server on older versions of Node (e.g. Node 4). There should probably be some effort similar to https://github.com/Khan/aphrodite/pull/200 to ensure that the order is properly preserved throughout this package. --- modules/static/createPrefixer.js | 2 +- modules/utils/prefixProperty.js | 28 ++++++++++++++++++++++------ test/dynamic/createPrefixer-test.js | 28 ++++++++++++++++++++++++++++ test/static/createPrefixer-test.js | 28 ++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/modules/static/createPrefixer.js b/modules/static/createPrefixer.js index 0953e28..2c53953 100644 --- a/modules/static/createPrefixer.js +++ b/modules/static/createPrefixer.js @@ -43,7 +43,7 @@ StaticData style[property] = processedValue } - prefixProperty(prefixMap, property, style) + style = prefixProperty(prefixMap, property, style) } } diff --git a/modules/utils/prefixProperty.js b/modules/utils/prefixProperty.js index 76dc5cc..a51f217 100644 --- a/modules/utils/prefixProperty.js +++ b/modules/utils/prefixProperty.js @@ -5,11 +5,27 @@ export default function prefixProperty( prefixProperties: Object, property: string, style: Object -): void { - if (prefixProperties.hasOwnProperty(property)) { - const requiredPrefixes = prefixProperties[property] - for (let i = 0, len = requiredPrefixes.length; i < len; ++i) { - style[requiredPrefixes[i] + capitalizeString(property)] = style[property] - } +): Object { + if (!prefixProperties.hasOwnProperty(property)) { + return style } + + // We need to preserve the order of the styles while inserting new prefixed + // styles. Object order is not guaranteed, but this is better than nothing. + // Note that this is brittle and is likely to break in older versions of + // Node (e.g. Node 4). + const newStyle = {} + Object.keys(style).forEach((styleProperty) => { + if (styleProperty === property) { + // We've found the style we need to prefix. + const requiredPrefixes = prefixProperties[property] + for (let i = 0, len = requiredPrefixes.length; i < len; ++i) { + newStyle[requiredPrefixes[i] + capitalizeString(property)] = style[property] + } + } + + newStyle[styleProperty] = style[styleProperty] + }) + + return newStyle } diff --git a/test/dynamic/createPrefixer-test.js b/test/dynamic/createPrefixer-test.js index a5f7e33..5f4132a 100644 --- a/test/dynamic/createPrefixer-test.js +++ b/test/dynamic/createPrefixer-test.js @@ -386,5 +386,33 @@ describe('Dynamic Prefixer', () => { } expect(Prefixer.prefixAll(input)).to.eql(output) }) + + it('puts the prefixes in the correct order', () => { + const input = { userSelect: 'none' } + const order = [ + 'WebkitUserSelect', + 'MozUserSelect', + 'msUserSelect', + 'userSelect' + ] + expect(Object.keys(Prefixer.prefixAll(input))).to.eql(order) + }); + + it('does not mess up the order of other styles', () => { + const input = { + color: 'red', + userSelect: 'none', + border: 0 + } + const order = [ + 'color', + 'WebkitUserSelect', + 'MozUserSelect', + 'msUserSelect', + 'userSelect', + 'border' + ] + expect(Object.keys(Prefixer.prefixAll(input))).to.eql(order) + }); }) }) diff --git a/test/static/createPrefixer-test.js b/test/static/createPrefixer-test.js index 8a75fed..a53a9d8 100644 --- a/test/static/createPrefixer-test.js +++ b/test/static/createPrefixer-test.js @@ -21,6 +21,34 @@ describe('Static Prefixer', () => { expect(prefixAll(input)).to.eql(output) }) + it('puts the prefixes in the correct order', () => { + const input = { userSelect: 'none' } + const order = [ + 'WebkitUserSelect', + 'MozUserSelect', + 'msUserSelect', + 'userSelect' + ] + expect(Object.keys(prefixAll(input))).to.eql(order) + }); + + it('does not mess up the order of other styles', () => { + const input = { + color: 'red', + userSelect: 'none', + border: 0 + } + const order = [ + 'color', + 'WebkitUserSelect', + 'MozUserSelect', + 'msUserSelect', + 'userSelect', + 'border' + ] + expect(Object.keys(prefixAll(input))).to.eql(order) + }); + it('should use dash-cased alternative values in array', () => { const input = { marginLeft: 'calc(30deg)' } const output = { marginLeft: ['-webkit-calc(30deg)', '-moz-calc(30deg)', 'calc(30deg)'] }