Skip to content

Commit

Permalink
fix(ssr): properly handle invalid and numeric style properties
Browse files Browse the repository at this point in the history
Ignores values that are not string or numbers, and append px as default
unit to appropriate properties.

There will still be certain cases where the user simply provides an
invalid string value to a property which will be too costly to detect
(it's possible by using the `cssstyle` package, but very heavy). But
in such cases the user would already notice the style is not working
on the client, so it's not really worth it for the perf implications.

fix #9231
  • Loading branch information
yyx990803 committed Jan 11, 2019
1 parent fe2b27f commit 7d9cfeb
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 4 deletions.
25 changes: 22 additions & 3 deletions src/platforms/web/server/modules/style.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* @flow */

import { escape } from '../util'
import { escape, noUnitNumericStyleProps } from '../util'
import { hyphenate } from 'shared/util'
import { getStyle } from 'web/util/style'

Expand All @@ -11,15 +11,34 @@ export function genStyle (style: Object): string {
const hyphenatedKey = hyphenate(key)
if (Array.isArray(value)) {
for (let i = 0, len = value.length; i < len; i++) {
styleText += `${hyphenatedKey}:${value[i]};`
styleText += normalizeValue(hyphenatedKey, value[i])
}
} else {
styleText += `${hyphenatedKey}:${value};`
styleText += normalizeValue(hyphenatedKey, value)
}
}
return styleText
}

function normalizeValue(key: string, value: any): string {
if (typeof value === 'string') {
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.

This comment has been minimized.

Copy link
@Justineo

Justineo Jan 11, 2019

Member

It doesn't seems to be so...?

document.body.style.width = 1000
document.body.style.width // -> ""

This comment has been minimized.

Copy link
@yyx990803

yyx990803 Jan 11, 2019

Author Member

Hmm... it only auto appends px if the document does NOT have <!DOCTYPE html> 🤯
Guess we'll need to revert that...

This comment has been minimized.

Copy link
@yyx990803

yyx990803 Jan 11, 2019

Author Member

I was testing in am HTML file without <!DOCTYPE html> and it triggers quirksmode which supports setting numbers on Element.style... :/

This comment has been minimized.

Copy link
@yyx990803

yyx990803 Jan 11, 2019

Author Member

I think the correct behavior would be not rendering numbers if the property requires a unit, since otherwise there will be a mismatch between server and client behavior. This is implemented in 17d8bcb.

if (noUnitNumericStyleProps[key]) {
return `${key}:${value};`
} else {
return `${key}:${value}px;`
}
} else {
// invalid values
return ``
}
}

export default function renderStyle (vnode: VNodeWithData): ?string {
const styleText = genStyle(getStyle(vnode, false))
if (styleText !== '') {
Expand Down
45 changes: 45 additions & 0 deletions src/platforms/web/server/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,48 @@ export function escape (s: string) {
function escapeChar (a) {
return ESC[a] || a
}

export const noUnitNumericStyleProps = {
"animation-iteration-count": true,
"border-image-outset": true,
"border-image-slice": true,
"border-image-width": true,
"box-flex": true,
"box-flex-group": true,
"box-ordinal-group": true,
"column-count": true,
"columns": true,
"flex": true,
"flex-grow": true,
"flex-positive": true,
"flex-shrink": true,
"flex-negative": true,
"flex-order": true,
"grid-row": true,
"grid-row-end": true,
"grid-row-span": true,
"grid-row-start": true,
"grid-column": true,
"grid-column-end": true,
"grid-column-span": true,
"grid-column-start": true,
"font-weight": true,
"line-clamp": true,
"line-height": true,
"opacity": true,
"order": true,
"orphans": true,
"tab-size": true,
"widows": true,
"z-index": true,
"zoom": true,
// SVG
"fill-opacity": true,
"flood-opacity": true,
"stop-opacity": true,
"stroke-dasharray": true,
"stroke-dashoffset": true,
"stroke-miterlimit": true,
"stroke-opacity": true,
"stroke-width": true
}
1 change: 0 additions & 1 deletion src/platforms/web/util/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,3 @@ export function getStyle (vnode: VNodeWithData, checkChild: boolean): Object {
}
return res
}

41 changes: 41 additions & 0 deletions test/ssr/ssr-string.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1499,6 +1499,47 @@ describe('SSR: renderToString', () => {
done()
})
})

it('invalid style value', done => {
renderVmWithOptions({
template: '<div :style="style"><p :style="style2"/></div>',
data: {
// all invalid, should not even have "style" attribute
style: {
opacity: {},
color: null
},
// mix of valid and invalid
style2: {
opacity: 0,
color: null
}
}
}, result => {
expect(result).toContain(
'<div data-server-rendered="true"><p style="opacity:0;"></p></div>'
)
done()
})
})

it('numeric style value', done => {
renderVmWithOptions({
template: '<div :style="style"></div>',
data: {
style: {
opacity: 0, // opacity should display as-is
top: 0, // top and margin should be converted to '0px'

This comment has been minimized.

Copy link
@JounQin

JounQin Jan 11, 2019

Contributor

Why not keep 0 but converting to 0px?

This comment has been minimized.

Copy link
@manniL

manniL Jan 11, 2019

Contributor

Indeed! Having just 0 would be sufficient according to the W3C specs (more info at this SO answer).

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

function renderVmWithOptions (options, cb) {
Expand Down

0 comments on commit 7d9cfeb

Please sign in to comment.