Skip to content

Commit

Permalink
Preserve style order in prefixProperty
Browse files Browse the repository at this point in the history
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.

  Khan/aphrodite#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
Khan/aphrodite#200 to ensure that the order is
properly preserved throughout this package.
  • Loading branch information
lencioni committed Mar 6, 2017
1 parent 34fcf25 commit cb6a664
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 7 deletions.
2 changes: 1 addition & 1 deletion modules/static/createPrefixer.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ StaticData
style[property] = processedValue
}

prefixProperty(prefixMap, property, style)
style = prefixProperty(prefixMap, property, style)
}
}

Expand Down
28 changes: 22 additions & 6 deletions modules/utils/prefixProperty.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
28 changes: 28 additions & 0 deletions test/dynamic/createPrefixer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
});
})
})
28 changes: 28 additions & 0 deletions test/static/createPrefixer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)'] }
Expand Down

0 comments on commit cb6a664

Please sign in to comment.