From 6eb339931185a57cc36ddb6e12314a5283948169 Mon Sep 17 00:00:00 2001 From: Evan You Date: Fri, 10 Apr 2020 11:57:07 -0400 Subject: [PATCH] fix(runtime-dom): fix patching for attributes starting with `on` fix #949 BREAKING CHANGE: Only props starting with `on` followed by an uppercase letter or a non-letter character are considered event listeners. --- .../__tests__/componentEmits.spec.ts | 14 ++++--- packages/runtime-core/src/componentEmits.ts | 4 +- .../__tests__/modules/attrs.spec.ts | 28 +++++++++----- .../__tests__/modules/events.spec.ts | 38 ++++++++++--------- .../__tests__/modules/style.spec.ts | 20 +++++----- packages/runtime-dom/src/modules/events.ts | 12 +----- packages/runtime-dom/src/patchProp.ts | 11 +++++- packages/shared/src/index.ts | 3 +- 8 files changed, 72 insertions(+), 58 deletions(-) diff --git a/packages/runtime-core/__tests__/componentEmits.spec.ts b/packages/runtime-core/__tests__/componentEmits.spec.ts index 2807366b..d2692840 100644 --- a/packages/runtime-core/__tests__/componentEmits.spec.ts +++ b/packages/runtime-core/__tests__/componentEmits.spec.ts @@ -8,23 +8,27 @@ import { isEmitListener } from '../src/componentEmits' describe('component: emit', () => { mockWarn() - test('trigger both raw event and capitalize handlers', () => { + test('trigger handlers', () => { const Foo = defineComponent({ render() {}, created() { // the `emit` function is bound on component instances this.$emit('foo') this.$emit('bar') + this.$emit('!baz') } }) const onfoo = jest.fn() const onBar = jest.fn() - const Comp = () => h(Foo, { onfoo, onBar }) + const onBaz = jest.fn() + const Comp = () => h(Foo, { onfoo, onBar, ['on!baz']: onBaz }) render(h(Comp), nodeOps.createElement('div')) - expect(onfoo).toHaveBeenCalled() + expect(onfoo).not.toHaveBeenCalled() + // only capitalized or special chars are considerd event listeners expect(onBar).toHaveBeenCalled() + expect(onBaz).toHaveBeenCalled() }) // for v-model:foo-bar usage in DOM templates @@ -125,9 +129,9 @@ describe('component: emit', () => { test('isEmitListener', () => { expect(isEmitListener(['click'], 'onClick')).toBe(true) - expect(isEmitListener(['click'], 'onclick')).toBe(true) + expect(isEmitListener(['click'], 'onclick')).toBe(false) expect(isEmitListener({ click: null }, 'onClick')).toBe(true) - expect(isEmitListener({ click: null }, 'onclick')).toBe(true) + expect(isEmitListener({ click: null }, 'onclick')).toBe(false) expect(isEmitListener(['click'], 'onBlick')).toBe(false) expect(isEmitListener({ click: null }, 'onBlick')).toBe(false) }) diff --git a/packages/runtime-core/src/componentEmits.ts b/packages/runtime-core/src/componentEmits.ts index 0cd348dd..093ec408 100644 --- a/packages/runtime-core/src/componentEmits.ts +++ b/packages/runtime-core/src/componentEmits.ts @@ -66,12 +66,12 @@ export function emit( } } - let handler = props[`on${event}`] || props[`on${capitalize(event)}`] + let handler = props[`on${capitalize(event)}`] // for v-model update:xxx events, also trigger kebab-case equivalent // for props passed via kebab-case if (!handler && event.indexOf('update:') === 0) { event = hyphenate(event) - handler = props[`on${event}`] || props[`on${capitalize(event)}`] + handler = props[`on${capitalize(event)}`] } if (handler) { callWithAsyncErrorHandling( diff --git a/packages/runtime-dom/__tests__/modules/attrs.spec.ts b/packages/runtime-dom/__tests__/modules/attrs.spec.ts index 91d59888..9df8d14b 100644 --- a/packages/runtime-dom/__tests__/modules/attrs.spec.ts +++ b/packages/runtime-dom/__tests__/modules/attrs.spec.ts @@ -1,27 +1,37 @@ -import { patchAttr, xlinkNS } from '../../src/modules/attrs' +import { patchProp } from '../../src/patchProp' +import { xlinkNS } from '../../src/modules/attrs' describe('attrs', () => { test('xlink attributes', () => { const el = document.createElementNS('http://www.w3.org/2000/svg', 'use') - patchAttr(el, 'xlink:href', 'a', true) + patchProp(el, 'xlink:href', null, 'a', true) expect(el.getAttributeNS(xlinkNS, 'href')).toBe('a') - patchAttr(el, 'xlink:href', null, true) + patchProp(el, 'xlink:href', 'a', null, true) expect(el.getAttributeNS(xlinkNS, 'href')).toBe(null) }) test('boolean attributes', () => { const el = document.createElement('input') - patchAttr(el, 'readonly', true, false) + patchProp(el, 'readonly', null, true) expect(el.getAttribute('readonly')).toBe('') - patchAttr(el, 'readonly', false, false) + patchProp(el, 'readonly', true, false) expect(el.getAttribute('readonly')).toBe(null) }) test('attributes', () => { const el = document.createElement('div') - patchAttr(el, 'id', 'a', false) - expect(el.getAttribute('id')).toBe('a') - patchAttr(el, 'id', null, false) - expect(el.getAttribute('id')).toBe(null) + patchProp(el, 'foo', null, 'a') + expect(el.getAttribute('foo')).toBe('a') + patchProp(el, 'foo', 'a', null) + expect(el.getAttribute('foo')).toBe(null) + }) + + // #949 + test('onxxx but non-listener attributes', () => { + const el = document.createElement('div') + patchProp(el, 'onwards', null, 'a') + expect(el.getAttribute('onwards')).toBe('a') + patchProp(el, 'onwards', 'a', null) + expect(el.getAttribute('onwards')).toBe(null) }) }) diff --git a/packages/runtime-dom/__tests__/modules/events.spec.ts b/packages/runtime-dom/__tests__/modules/events.spec.ts index cb053482..2aced378 100644 --- a/packages/runtime-dom/__tests__/modules/events.spec.ts +++ b/packages/runtime-dom/__tests__/modules/events.spec.ts @@ -1,4 +1,4 @@ -import { patchEvent } from '../../src/modules/events' +import { patchProp } from '../../src/patchProp' const timeout = () => new Promise(r => setTimeout(r)) @@ -7,7 +7,7 @@ describe(`events`, () => { const el = document.createElement('div') const event = new Event('click') const fn = jest.fn() - patchEvent(el, 'onClick', null, fn, null) + patchProp(el, 'onClick', null, fn) el.dispatchEvent(event) await timeout() el.dispatchEvent(event) @@ -22,9 +22,9 @@ describe(`events`, () => { const event = new Event('click') const prevFn = jest.fn() const nextFn = jest.fn() - patchEvent(el, 'onClick', null, prevFn, null) + patchProp(el, 'onClick', null, prevFn) el.dispatchEvent(event) - patchEvent(el, 'onClick', prevFn, nextFn, null) + patchProp(el, 'onClick', prevFn, nextFn) await timeout() el.dispatchEvent(event) await timeout() @@ -39,7 +39,7 @@ describe(`events`, () => { const event = new Event('click') const fn1 = jest.fn() const fn2 = jest.fn() - patchEvent(el, 'onClick', null, [fn1, fn2], null) + patchProp(el, 'onClick', null, [fn1, fn2]) el.dispatchEvent(event) await timeout() expect(fn1).toHaveBeenCalledTimes(1) @@ -50,8 +50,8 @@ describe(`events`, () => { const el = document.createElement('div') const event = new Event('click') const fn = jest.fn() - patchEvent(el, 'onClick', null, fn, null) - patchEvent(el, 'onClick', fn, null, null) + patchProp(el, 'onClick', null, fn) + patchProp(el, 'onClick', fn, null) el.dispatchEvent(event) await timeout() expect(fn).not.toHaveBeenCalled() @@ -67,7 +67,7 @@ describe(`events`, () => { once: true } } - patchEvent(el, 'onClick', null, nextValue, null) + patchProp(el, 'onClick', null, nextValue) el.dispatchEvent(event) await timeout() el.dispatchEvent(event) @@ -86,8 +86,8 @@ describe(`events`, () => { once: true } } - patchEvent(el, 'onClick', null, prevFn, null) - patchEvent(el, 'onClick', prevFn, nextValue, null) + patchProp(el, 'onClick', null, prevFn) + patchProp(el, 'onClick', prevFn, nextValue) el.dispatchEvent(event) await timeout() el.dispatchEvent(event) @@ -106,8 +106,8 @@ describe(`events`, () => { once: true } } - patchEvent(el, 'onClick', null, nextValue, null) - patchEvent(el, 'onClick', nextValue, null, null) + patchProp(el, 'onClick', null, nextValue) + patchProp(el, 'onClick', nextValue, null) el.dispatchEvent(event) await timeout() el.dispatchEvent(event) @@ -115,21 +115,23 @@ describe(`events`, () => { expect(fn).not.toHaveBeenCalled() }) - it('should assign native onclick attribute', async () => { + it('should support native onclick', async () => { const el = document.createElement('div') const event = new Event('click') - const fn = ((window as any)._nativeClickSpy = jest.fn()) - patchEvent(el, 'onclick', null, '_nativeClickSpy()' as any) + // string should be set as attribute + const fn = ((window as any).__globalSpy = jest.fn()) + patchProp(el, 'onclick', null, '__globalSpy(1)') el.dispatchEvent(event) await timeout() - expect(fn).toHaveBeenCalledTimes(1) + delete (window as any).__globalSpy + expect(fn).toHaveBeenCalledWith(1) const fn2 = jest.fn() - patchEvent(el, 'onclick', null, fn2) + patchProp(el, 'onclick', '__globalSpy(1)', fn2) el.dispatchEvent(event) await timeout() expect(fn).toHaveBeenCalledTimes(1) - expect(fn2).toHaveBeenCalledTimes(1) + expect(fn2).toHaveBeenCalledWith(event) }) }) diff --git a/packages/runtime-dom/__tests__/modules/style.spec.ts b/packages/runtime-dom/__tests__/modules/style.spec.ts index 08667275..a3174fca 100644 --- a/packages/runtime-dom/__tests__/modules/style.spec.ts +++ b/packages/runtime-dom/__tests__/modules/style.spec.ts @@ -1,39 +1,39 @@ -import { patchStyle } from '../../src/modules/style' +import { patchProp } from '../../src/patchProp' describe(`module style`, () => { it('string', () => { const el = document.createElement('div') - patchStyle(el, {}, 'color:red') + patchProp(el, 'style', {}, 'color:red') expect(el.style.cssText.replace(/\s/g, '')).toBe('color:red;') }) it('plain object', () => { const el = document.createElement('div') - patchStyle(el, {}, { color: 'red' }) + patchProp(el, 'style', {}, { color: 'red' }) expect(el.style.cssText.replace(/\s/g, '')).toBe('color:red;') }) it('camelCase', () => { const el = document.createElement('div') - patchStyle(el, {}, { marginRight: '10px' }) + patchProp(el, 'style', {}, { marginRight: '10px' }) expect(el.style.cssText.replace(/\s/g, '')).toBe('margin-right:10px;') }) it('remove if falsy value', () => { const el = document.createElement('div') - patchStyle(el, { color: 'red' }, { color: undefined }) + patchProp(el, 'style', { color: 'red' }, { color: undefined }) expect(el.style.cssText.replace(/\s/g, '')).toBe('') }) it('!important', () => { const el = document.createElement('div') - patchStyle(el, {}, { color: 'red !important' }) + patchProp(el, 'style', {}, { color: 'red !important' }) expect(el.style.cssText.replace(/\s/g, '')).toBe('color:red!important;') }) it('camelCase with !important', () => { const el = document.createElement('div') - patchStyle(el, {}, { marginRight: '10px !important' }) + patchProp(el, 'style', {}, { marginRight: '10px !important' }) expect(el.style.cssText.replace(/\s/g, '')).toBe( 'margin-right:10px!important;' ) @@ -41,7 +41,7 @@ describe(`module style`, () => { it('object with multiple entries', () => { const el = document.createElement('div') - patchStyle(el, {}, { color: 'red', marginRight: '10px' }) + patchProp(el, 'style', {}, { color: 'red', marginRight: '10px' }) expect(el.style.getPropertyValue('color')).toBe('red') expect(el.style.getPropertyValue('margin-right')).toBe('10px') }) @@ -65,13 +65,13 @@ describe(`module style`, () => { it('CSS custom properties', () => { const el = mockElementWithStyle() - patchStyle(el as any, {}, { '--theme': 'red' } as any) + patchProp(el as any, 'style', {}, { '--theme': 'red' } as any) expect(el.style.getPropertyValue('--theme')).toBe('red') }) it('auto vendor prefixing', () => { const el = mockElementWithStyle() - patchStyle(el as any, {}, { transition: 'all 1s' }) + patchProp(el as any, 'style', {}, { transition: 'all 1s' }) expect(el.style.WebkitTransition).toBe('all 1s') }) }) diff --git a/packages/runtime-dom/src/modules/events.ts b/packages/runtime-dom/src/modules/events.ts index 0ac69479..6789934c 100644 --- a/packages/runtime-dom/src/modules/events.ts +++ b/packages/runtime-dom/src/modules/events.ts @@ -1,4 +1,4 @@ -import { EMPTY_OBJ, isString } from '@vue/shared' +import { EMPTY_OBJ } from '@vue/shared' import { ComponentInternalInstance, callWithAsyncErrorHandling @@ -71,16 +71,6 @@ export function patchEvent( nextValue: EventValueWithOptions | EventValue | null, instance: ComponentInternalInstance | null = null ) { - // support native onxxx handlers - if (rawName in el) { - if (isString(nextValue)) { - el.setAttribute(rawName, nextValue) - } else { - ;(el as any)[rawName] = nextValue - } - return - } - const name = rawName.slice(2).toLowerCase() const prevOptions = prevValue && 'options' in prevValue && prevValue.options const nextOptions = nextValue && 'options' in nextValue && nextValue.options diff --git a/packages/runtime-dom/src/patchProp.ts b/packages/runtime-dom/src/patchProp.ts index 013dfbf0..71d64155 100644 --- a/packages/runtime-dom/src/patchProp.ts +++ b/packages/runtime-dom/src/patchProp.ts @@ -3,9 +3,11 @@ import { patchStyle } from './modules/style' import { patchAttr } from './modules/attrs' import { patchDOMProp } from './modules/props' import { patchEvent } from './modules/events' -import { isOn } from '@vue/shared' +import { isOn, isString } from '@vue/shared' import { RendererOptions } from '@vue/runtime-core' +const nativeOnRE = /^on[a-z]/ + export const patchProp: RendererOptions['patchProp'] = ( el, key, @@ -31,7 +33,12 @@ export const patchProp: RendererOptions['patchProp'] = ( if (key.indexOf('onUpdate:') < 0) { patchEvent(el, key, prevValue, nextValue, parentComponent) } - } else if (!isSVG && key in el) { + } else if ( + !isSVG && + key in el && + // onclick="foo" needs to be set as an attribute to work + !(nativeOnRE.test(key) && isString(nextValue)) + ) { patchDOMProp( el, key, diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index fd4e149d..bfda315a 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -24,7 +24,8 @@ export const NOOP = () => {} */ export const NO = () => false -export const isOn = (key: string) => key[0] === 'o' && key[1] === 'n' +const onRE = /^on[^a-z]/ +export const isOn = (key: string) => onRE.test(key) export const extend = ( a: T,