fix(runtime-dom): fix patching for attributes starting with on

yyx990803 · yyx990803 · commit 6eb339931185 · 2020-04-10T11:57:07.000-04:00
fix #949 BREAKING CHANGE: Only props starting with `on` followed by an uppercase letter or a non-letter character are considered event listeners.
diff --git 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
@@ -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
@@ -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
@@ -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,30 +106,32 @@ 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)
     await timeout()
     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
@@ -1,47 +1,47 @@
-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;'
     )
   })
 
   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
@@ -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
@@ -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<Node, Element>['patchProp'] = (
   el,
   key,
@@ -31,7 +33,12 @@ export const patchProp: RendererOptions<Node, Element>['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
@@ -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 = <T extends object, U extends object>(
   a: T,

Original file line number	Diff line number	Diff line change
`@@ -66,12 +66,12 @@ export function emit(`
`66`	`66`	`}`
`67`	`67`	`}`
`68`	`68`
`69`		- let handler = props[`on${event}`] \|\| props[`on${capitalize(event)}`]
	`69`	+ let handler = props[`on${capitalize(event)}`]
`70`	`70`	`// for v-model update:xxx events, also trigger kebab-case equivalent`
`71`	`71`	`// for props passed via kebab-case`
`72`	`72`	`if (!handler && event.indexOf('update:') === 0) {`
`73`	`73`	`event = hyphenate(event)`
`74`		- handler = props[`on${event}`] \|\| props[`on${capitalize(event)}`]
	`74`	+ handler = props[`on${capitalize(event)}`]
`75`	`75`	`}`
`76`	`76`	`if (handler) {`
`77`	`77`	`callWithAsyncErrorHandling(`