Skip to content

Commit

Permalink
fix(ssr): should not render invalid numeric style values
Browse files Browse the repository at this point in the history
Reverts part of 7d9cfeb - browsers only auto append units to
numbers when in quirksmode, so in standard mode, numbers set to
style properties that require units are invalid and should not
be rendered.
  • Loading branch information
yyx990803 committed Jan 11, 2019
1 parent 7d9cfeb commit 17d8bcb
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 15 deletions.
15 changes: 4 additions & 11 deletions src/platforms/web/server/modules/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,11 @@ export function genStyle (style: Object): string {
}

function normalizeValue(key: string, value: any): string {
if (typeof value === 'string') {
if (
typeof value === 'string' ||
(typeof value === 'number' && noUnitNumericStyleProps[key])
) {
return `${key}:${value};`
} else if (typeof value === 'number') {
// Handle numeric values.
// Turns out all evergreen browsers plus IE11 already support setting plain
// numbers on the style object and will automatically convert it to px if
// applicable, so we should support that on the server too.
if (noUnitNumericStyleProps[key]) {
return `${key}:${value};`
} else {
return `${key}:${value}px;`
}
} else {
// invalid values
return ``
Expand Down
8 changes: 4 additions & 4 deletions test/ssr/ssr-string.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1528,14 +1528,14 @@ describe('SSR: renderToString', () => {
template: '<div :style="style"></div>',
data: {
style: {
opacity: 0, // opacity should display as-is
top: 0, // top and margin should be converted to '0px'
marginTop: 10
opacity: 0, // valid, opacity is unit-less
top: 0, // invalid, top requires unit

This comment has been minimized.

Copy link
@Justineo

Justineo Jan 12, 2019

Member

Units are optional for 0 length values in CSS, so I think in this case it should be rendered. Both top:0 and top:0px are fine.

document.body.style.top = 0
document.body.style.top // '0px'
document.body.style.top = 1
document.body.style.top // '0px'

This comment has been minimized.

Copy link
@yyx990803

yyx990803 Jan 12, 2019

Author Member

good point! aef5b4e

This comment has been minimized.

Copy link
@Justineo

Justineo Jan 12, 2019

Member

Shall we care about value validation at all? I think not outputing false/null/undefined should be enough. Though 0 is valid for length values, but it's not valid for other value types. eg.

document.body.style.transitionDelay = 0
document.body.style.transitionDelay // ""
marginTop: '10px' // valid
}
}
}, result => {
expect(result).toContain(
'<div data-server-rendered="true" style="opacity:0;top:0px;margin-top:10px;"></div>'
'<div data-server-rendered="true" style="opacity:0;margin-top:10px;"></div>'
)
done()
})
Expand Down

0 comments on commit 17d8bcb

Please sign in to comment.