Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(shared): consistently remove boolean attributes for falsy values #4348

Merged
merged 1 commit into from
Aug 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/compiler-ssr/__tests__/ssrElement.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ describe('ssr: element', () => {
expect(getCompiledString(`<input type="checkbox" :checked="checked">`))
.toMatchInlineSnapshot(`
"\`<input type=\\"checkbox\\"\${
(_ctx.checked) ? \\" checked\\" : \\"\\"
(_ssrIncludeBooleanAttr(_ctx.checked)) ? \\" checked\\" : \\"\\"
}>\`"
`)
})
Expand Down
24 changes: 12 additions & 12 deletions packages/compiler-ssr/__tests__/ssrVModel.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ describe('ssr: v-model', () => {
expect(
compileWithWrapper(`<input type="radio" value="foo" v-model="bar">`).code
).toMatchInlineSnapshot(`
"const { ssrLooseEqual: _ssrLooseEqual, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")
"const { ssrLooseEqual: _ssrLooseEqual, ssrIncludeBooleanAttr: _ssrIncludeBooleanAttr, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")

return function ssrRender(_ctx, _push, _parent, _attrs) {
_push(\`<div\${
_ssrRenderAttrs(_attrs)
}><input type=\\"radio\\" value=\\"foo\\"\${
(_ssrLooseEqual(_ctx.bar, \\"foo\\")) ? \\" checked\\" : \\"\\"
(_ssrIncludeBooleanAttr(_ssrLooseEqual(_ctx.bar, \\"foo\\"))) ? \\" checked\\" : \\"\\"
}></div>\`)
}"
`)
Expand All @@ -52,15 +52,15 @@ describe('ssr: v-model', () => {
test('<input type="checkbox">', () => {
expect(compileWithWrapper(`<input type="checkbox" v-model="bar">`).code)
.toMatchInlineSnapshot(`
"const { ssrLooseContain: _ssrLooseContain, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")
"const { ssrLooseContain: _ssrLooseContain, ssrIncludeBooleanAttr: _ssrIncludeBooleanAttr, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")

return function ssrRender(_ctx, _push, _parent, _attrs) {
_push(\`<div\${
_ssrRenderAttrs(_attrs)
}><input type=\\"checkbox\\"\${
((Array.isArray(_ctx.bar))
(_ssrIncludeBooleanAttr((Array.isArray(_ctx.bar))
? _ssrLooseContain(_ctx.bar, null)
: _ctx.bar) ? \\" checked\\" : \\"\\"
: _ctx.bar)) ? \\" checked\\" : \\"\\"
}></div>\`)
}"
`)
Expand All @@ -69,15 +69,15 @@ describe('ssr: v-model', () => {
compileWithWrapper(`<input type="checkbox" value="foo" v-model="bar">`)
.code
).toMatchInlineSnapshot(`
"const { ssrLooseContain: _ssrLooseContain, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")
"const { ssrLooseContain: _ssrLooseContain, ssrIncludeBooleanAttr: _ssrIncludeBooleanAttr, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")

return function ssrRender(_ctx, _push, _parent, _attrs) {
_push(\`<div\${
_ssrRenderAttrs(_attrs)
}><input type=\\"checkbox\\" value=\\"foo\\"\${
((Array.isArray(_ctx.bar))
(_ssrIncludeBooleanAttr((Array.isArray(_ctx.bar))
? _ssrLooseContain(_ctx.bar, \\"foo\\")
: _ctx.bar) ? \\" checked\\" : \\"\\"
: _ctx.bar)) ? \\" checked\\" : \\"\\"
}></div>\`)
}"
`)
Expand All @@ -87,13 +87,13 @@ describe('ssr: v-model', () => {
`<input type="checkbox" :true-value="foo" :false-value="bar" v-model="baz">`
).code
).toMatchInlineSnapshot(`
"const { ssrLooseEqual: _ssrLooseEqual, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")
"const { ssrLooseEqual: _ssrLooseEqual, ssrIncludeBooleanAttr: _ssrIncludeBooleanAttr, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")

return function ssrRender(_ctx, _push, _parent, _attrs) {
_push(\`<div\${
_ssrRenderAttrs(_attrs)
}><input type=\\"checkbox\\"\${
(_ssrLooseEqual(_ctx.baz, _ctx.foo)) ? \\" checked\\" : \\"\\"
(_ssrIncludeBooleanAttr(_ssrLooseEqual(_ctx.baz, _ctx.foo))) ? \\" checked\\" : \\"\\"
}></div>\`)
}"
`)
Expand All @@ -103,13 +103,13 @@ describe('ssr: v-model', () => {
`<input type="checkbox" true-value="foo" false-value="bar" v-model="baz">`
).code
).toMatchInlineSnapshot(`
"const { ssrLooseEqual: _ssrLooseEqual, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")
"const { ssrLooseEqual: _ssrLooseEqual, ssrIncludeBooleanAttr: _ssrIncludeBooleanAttr, ssrRenderAttrs: _ssrRenderAttrs } = require(\\"@vue/server-renderer\\")

return function ssrRender(_ctx, _push, _parent, _attrs) {
_push(\`<div\${
_ssrRenderAttrs(_attrs)
}><input type=\\"checkbox\\"\${
(_ssrLooseEqual(_ctx.baz, \\"foo\\")) ? \\" checked\\" : \\"\\"
(_ssrIncludeBooleanAttr(_ssrLooseEqual(_ctx.baz, \\"foo\\"))) ? \\" checked\\" : \\"\\"
}></div>\`)
}"
`)
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-ssr/src/runtimeHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export const SSR_RENDER_ATTRS = Symbol(`ssrRenderAttrs`)
export const SSR_RENDER_ATTR = Symbol(`ssrRenderAttr`)
export const SSR_RENDER_DYNAMIC_ATTR = Symbol(`ssrRenderDynamicAttr`)
export const SSR_RENDER_LIST = Symbol(`ssrRenderList`)
export const SSR_INCLUDE_BOOLEAN_ATTR = Symbol(`ssrIncludeBooleanAttr`)
export const SSR_LOOSE_EQUAL = Symbol(`ssrLooseEqual`)
export const SSR_LOOSE_CONTAIN = Symbol(`ssrLooseContain`)
export const SSR_RENDER_DYNAMIC_MODEL = Symbol(`ssrRenderDynamicModel`)
Expand All @@ -28,6 +29,7 @@ export const ssrHelpers = {
[SSR_RENDER_ATTR]: `ssrRenderAttr`,
[SSR_RENDER_DYNAMIC_ATTR]: `ssrRenderDynamicAttr`,
[SSR_RENDER_LIST]: `ssrRenderList`,
[SSR_INCLUDE_BOOLEAN_ATTR]: `ssrIncludeBooleanAttr`,
[SSR_LOOSE_EQUAL]: `ssrLooseEqual`,
[SSR_LOOSE_CONTAIN]: `ssrLooseContain`,
[SSR_RENDER_DYNAMIC_MODEL]: `ssrRenderDynamicModel`,
Expand Down
8 changes: 6 additions & 2 deletions packages/compiler-ssr/src/transforms/ssrTransformElement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ import {
SSR_RENDER_DYNAMIC_ATTR,
SSR_RENDER_ATTRS,
SSR_INTERPOLATE,
SSR_GET_DYNAMIC_MODEL_PROPS
SSR_GET_DYNAMIC_MODEL_PROPS,
SSR_INCLUDE_BOOLEAN_ATTR
} from '../runtimeHelpers'
import { SSRTransformContext, processChildren } from '../ssrCodegenTransform'

Expand Down Expand Up @@ -237,7 +238,10 @@ export const ssrTransformElement: NodeTransform = (node, context) => {
if (isBooleanAttr(attrName)) {
openTag.push(
createConditionalExpression(
value,
createCallExpression(
context.helper(SSR_INCLUDE_BOOLEAN_ATTR),
[value]
),
createSimpleExpression(' ' + attrName, true),
createSimpleExpression('', true),
false /* no newline */
Expand Down
12 changes: 12 additions & 0 deletions packages/runtime-dom/__tests__/patchAttrs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@ describe('runtime-dom: attrs patching', () => {
expect(el.getAttribute('readonly')).toBe('')
patchProp(el, 'readonly', true, false)
expect(el.getAttribute('readonly')).toBe(null)
patchProp(el, 'readonly', false, '')
expect(el.getAttribute('readonly')).toBe('')
patchProp(el, 'readonly', '', 0)
expect(el.getAttribute('readonly')).toBe(null)
patchProp(el, 'readonly', 0, '0')
expect(el.getAttribute('readonly')).toBe('')
patchProp(el, 'readonly', '0', false)
expect(el.getAttribute('readonly')).toBe(null)
patchProp(el, 'readonly', false, 1)
expect(el.getAttribute('readonly')).toBe('')
patchProp(el, 'readonly', 1, undefined)
expect(el.getAttribute('readonly')).toBe(null)
})

test('attributes', () => {
Expand Down
12 changes: 12 additions & 0 deletions packages/runtime-dom/__tests__/patchProps.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ describe('runtime-dom: props patching', () => {
expect(el.multiple).toBe(true)
patchProp(el, 'multiple', null, null)
expect(el.multiple).toBe(false)
patchProp(el, 'multiple', null, true)
expect(el.multiple).toBe(true)
patchProp(el, 'multiple', null, 0)
expect(el.multiple).toBe(false)
patchProp(el, 'multiple', null, '0')
expect(el.multiple).toBe(true)
patchProp(el, 'multiple', null, false)
expect(el.multiple).toBe(false)
patchProp(el, 'multiple', null, 1)
expect(el.multiple).toBe(true)
patchProp(el, 'multiple', null, undefined)
expect(el.multiple).toBe(false)
})

test('innerHTML unmount prev children', () => {
Expand Down
9 changes: 7 additions & 2 deletions packages/runtime-dom/src/modules/attrs.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { isSpecialBooleanAttr, makeMap, NOOP } from '@vue/shared'
import {
includeBooleanAttr,
isSpecialBooleanAttr,
makeMap,
NOOP
} from '@vue/shared'
import {
compatUtils,
ComponentInternalInstance,
Expand Down Expand Up @@ -28,7 +33,7 @@ export function patchAttr(
// note we are only checking boolean attributes that don't have a
// corresponding dom prop of the same name here.
const isBoolean = isSpecialBooleanAttr(key)
if (value == null || (isBoolean && value === false)) {
if (value == null || (isBoolean && !includeBooleanAttr(value))) {
el.removeAttribute(key)
} else {
el.setAttribute(key, isBoolean ? '' : value)
Expand Down
5 changes: 3 additions & 2 deletions packages/runtime-dom/src/modules/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// This can come from explicit usage of v-html or innerHTML as a prop in render

import { warn, DeprecationTypes, compatUtils } from '@vue/runtime-core'
import { includeBooleanAttr } from '@vue/shared'

// functions. The user is responsible for using them with only trusted content.
export function patchDOMProp(
Expand Down Expand Up @@ -41,9 +42,9 @@ export function patchDOMProp(

if (value === '' || value == null) {
const type = typeof el[key]
if (value === '' && type === 'boolean') {
if (type === 'boolean') {
// e.g. <select multiple> compiles to { multiple: '' }
el[key] = true
el[key] = includeBooleanAttr(value)
return
} else if (value == null && type === 'string') {
// e.g. <div :id="null">
Expand Down
6 changes: 4 additions & 2 deletions packages/server-renderer/__tests__/ssrRenderAttrs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ describe('ssr: renderAttrs', () => {
expect(
ssrRenderAttrs({
checked: true,
multiple: false
multiple: false,
readonly: 0,
disabled: ''
})
).toBe(` checked`) // boolean attr w/ false should be ignored
).toBe(` checked disabled`) // boolean attr w/ false should be ignored
})

test('ignore falsy values', () => {
Expand Down
3 changes: 2 additions & 1 deletion packages/server-renderer/src/helpers/ssrRenderAttrs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
isOn,
isSSRSafeAttrName,
isBooleanAttr,
includeBooleanAttr,
makeMap
} from '@vue/shared'

Expand Down Expand Up @@ -52,7 +53,7 @@ export function ssrRenderDynamicAttr(
? key // preserve raw name on custom elements
: propsToAttrMap[key] || key.toLowerCase()
if (isBooleanAttr(attrKey)) {
return value === false ? `` : ` ${attrKey}`
return includeBooleanAttr(value) ? ` ${attrKey}` : ``
} else if (isSSRSafeAttrName(attrKey)) {
return value === '' ? ` ${attrKey}` : ` ${attrKey}="${escapeHtml(value)}"`
} else {
Expand Down
1 change: 1 addition & 0 deletions packages/server-renderer/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export {
export { ssrInterpolate } from './helpers/ssrInterpolate'
export { ssrRenderList } from './helpers/ssrRenderList'
export { ssrRenderSuspense } from './helpers/ssrRenderSuspense'
export { includeBooleanAttr as ssrIncludeBooleanAttr } from '@vue/shared'

// v-model helpers
export {
Expand Down
8 changes: 8 additions & 0 deletions packages/shared/src/domAttrConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ export const isBooleanAttr = /*#__PURE__*/ makeMap(
`checked,muted,multiple,selected`
)

/**
* Boolean attributes should be included if the value is truthy or ''.
* e.g. <select multiple> compiles to { multiple: '' }
*/
export function includeBooleanAttr(value: unknown): boolean {
return !!value || value === ''
}

const unsafeAttrCharRE = /[>/="'\u0009\u000a\u000c\u0020]/
const attrValidationCache: Record<string, boolean> = {}

Expand Down