Skip to content

Commit

Permalink
fix(runtime-dom): fix event timestamp check in iframes
Browse files Browse the repository at this point in the history 
fix #2513
fix #3933
close #5474
  • Loading branch information
@yyx990803
yyx990803 committed Oct 14, 2022
1 parent a71f9ac commit 5ee4053
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 75 deletions.
73 changes: 43 additions & 30 deletions packages/runtime-dom/__tests__/patchEvents.spec.ts
View file
Expand Up @@ -5,101 +5,95 @@ const timeout = () => new Promise(r => setTimeout(r))
describe(`runtime-dom: events patching`, () => {
it('should assign event handler', async () => {
const el = document.createElement('div')
const event = new Event('click')
const fn = jest.fn()
patchProp(el, 'onClick', null, fn)
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn).toHaveBeenCalledTimes(3)
})

it('should update event handler', async () => {
const el = document.createElement('div')
const event = new Event('click')
const prevFn = jest.fn()
const nextFn = jest.fn()
patchProp(el, 'onClick', null, prevFn)
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
patchProp(el, 'onClick', prevFn, nextFn)
await timeout()
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(prevFn).toHaveBeenCalledTimes(1)
expect(nextFn).toHaveBeenCalledTimes(2)
})

it('should support multiple event handlers', async () => {
const el = document.createElement('div')
const event = new Event('click')
const fn1 = jest.fn()
const fn2 = jest.fn()
patchProp(el, 'onClick', null, [fn1, fn2])
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn1).toHaveBeenCalledTimes(1)
expect(fn2).toHaveBeenCalledTimes(1)
})

it('should unassign event handler', async () => {
const el = document.createElement('div')
const event = new Event('click')
const fn = jest.fn()
patchProp(el, 'onClick', null, fn)
patchProp(el, 'onClick', fn, null)
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn).not.toHaveBeenCalled()
})

it('should support event option modifiers', async () => {
const el = document.createElement('div')
const event = new Event('click')
const fn = jest.fn()
patchProp(el, 'onClickOnceCapture', null, fn)
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn).toHaveBeenCalledTimes(1)
})

it('should unassign event handler with options', async () => {
const el = document.createElement('div')
const event = new Event('click')
const fn = jest.fn()
patchProp(el, 'onClickCapture', null, fn)
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn).toHaveBeenCalledTimes(1)

patchProp(el, 'onClickCapture', fn, null)
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn).toHaveBeenCalledTimes(1)
})

it('should support native onclick', async () => {
const el = document.createElement('div')
const event = new Event('click')

// string should be set as attribute
const fn = ((window as any).__globalSpy = jest.fn())
patchProp(el, 'onclick', null, '__globalSpy(1)')
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
delete (window as any).__globalSpy
expect(fn).toHaveBeenCalledWith(1)

const fn2 = jest.fn()
patchProp(el, 'onclick', '__globalSpy(1)', fn2)
const event = new Event('click')
el.dispatchEvent(event)
await timeout()
expect(fn).toHaveBeenCalledTimes(1)
Expand All @@ -108,13 +102,12 @@ describe(`runtime-dom: events patching`, () => {

it('should support stopImmediatePropagation on multiple listeners', async () => {
const el = document.createElement('div')
const event = new Event('click')
const fn1 = jest.fn((e: Event) => {
e.stopImmediatePropagation()
})
const fn2 = jest.fn()
patchProp(el, 'onClick', null, [fn1, fn2])
el.dispatchEvent(event)
el.dispatchEvent(new Event('click'))
await timeout()
expect(fn1).toHaveBeenCalledTimes(1)
expect(fn2).toHaveBeenCalledTimes(0)
Expand All @@ -125,35 +118,55 @@ describe(`runtime-dom: events patching`, () => {
const el1 = document.createElement('div')
const el2 = document.createElement('div')

const event = new Event('click')
// const event = new Event('click')
const prevFn = jest.fn()
const nextFn = jest.fn()

patchProp(el1, 'onClick', null, prevFn)
patchProp(el2, 'onClick', null, prevFn)

el1.dispatchEvent(event)
el2.dispatchEvent(event)
el1.dispatchEvent(new Event('click'))
el2.dispatchEvent(new Event('click'))
await timeout()
expect(prevFn).toHaveBeenCalledTimes(2)
expect(nextFn).toHaveBeenCalledTimes(0)

patchProp(el1, 'onClick', prevFn, nextFn)
patchProp(el2, 'onClick', prevFn, nextFn)

el1.dispatchEvent(event)
el2.dispatchEvent(event)
el1.dispatchEvent(new Event('click'))
el2.dispatchEvent(new Event('click'))
await timeout()
expect(prevFn).toHaveBeenCalledTimes(2)
expect(nextFn).toHaveBeenCalledTimes(2)

el1.dispatchEvent(event)
el2.dispatchEvent(event)
el1.dispatchEvent(new Event('click'))
el2.dispatchEvent(new Event('click'))
await timeout()
expect(prevFn).toHaveBeenCalledTimes(2)
expect(nextFn).toHaveBeenCalledTimes(4)
})

// vuejs/vue#6566
it('should not fire handler attached by the event itself', async () => {
const el = document.createElement('div')
const child = document.createElement('div')
el.appendChild(child)
document.body.appendChild(el)
const childFn = jest.fn()
const parentFn = jest.fn()

patchProp(child, 'onClick', null, () => {
childFn()
patchProp(el, 'onClick', null, parentFn)
})
child.dispatchEvent(new Event('click', { bubbles: true }))

await timeout()
expect(childFn).toHaveBeenCalled()
expect(parentFn).not.toHaveBeenCalled()
})

// #2841
test('should patch event correctly in web-components', async () => {
class TestElement extends HTMLElement {
Expand Down
72 changes: 27 additions & 45 deletions packages/runtime-dom/src/modules/events.ts
View file
Expand Up @@ -12,38 +12,6 @@ interface Invoker extends EventListener {

type EventValue = Function | Function[]

// Async edge case fix requires storing an event listener's attach timestamp.
const [_getNow, skipTimestampCheck] = /*#__PURE__*/ (() => {
let _getNow = Date.now
let skipTimestampCheck = false
if (typeof window !== 'undefined') {
// Determine what event timestamp the browser is using. Annoyingly, the
// timestamp can either be hi-res (relative to page load) or low-res
// (relative to UNIX epoch), so in order to compare time we have to use the
// same timestamp type when saving the flush timestamp.
if (Date.now() > document.createEvent('Event').timeStamp) {
// if the low-res timestamp which is bigger than the event timestamp
// (which is evaluated AFTER) it means the event is using a hi-res timestamp,
// and we need to use the hi-res version for event listeners as well.
_getNow = performance.now.bind(performance)
}
// #3485: Firefox <= 53 has incorrect Event.timeStamp implementation
// and does not fire microtasks in between event propagation, so safe to exclude.
const ffMatch = navigator.userAgent.match(/firefox\/(\d+)/i)
skipTimestampCheck = !!(ffMatch && Number(ffMatch[1]) <= 53)
}
return [_getNow, skipTimestampCheck]
})()

// To avoid the overhead of repeatedly calling performance.now(), we cache
// and use the same timestamp for all event listeners attached in the same tick.
let cachedNow: number = 0
const p = /*#__PURE__*/ Promise.resolve()
const reset = () => {
cachedNow = 0
}
const getNow = () => cachedNow || (p.then(reset), (cachedNow = _getNow()))

export function addEventListener(
el: Element,
event: string,
Expand Down Expand Up @@ -105,27 +73,41 @@ function parseName(name: string): [string, EventListenerOptions | undefined] {
return [event, options]
}

// To avoid the overhead of repeatedly calling Date.now(), we cache
// and use the same timestamp for all event listeners attached in the same tick.
let cachedNow: number = 0
const p = /*#__PURE__*/ Promise.resolve()
const getNow = () =>
cachedNow || (p.then(() => (cachedNow = 0)), (cachedNow = Date.now()))

function createInvoker(
initialValue: EventValue,
instance: ComponentInternalInstance | null
) {
const invoker: Invoker = (e: Event) => {
// async edge case #6566: inner click event triggers patch, event handler
const invoker: Invoker = (e: Event & { _vts?: number }) => {
// async edge case vuejs/vue#6566
// inner click event triggers patch, event handler
// attached to outer element during patch, and triggered again. This
// happens because browsers fire microtask ticks between event propagation.
// the solution is simple: we save the timestamp when a handler is attached,
// and the handler would only fire if the event passed to it was fired
// this no longer happens for templates in Vue 3, but could still be
// theoretically possible for hand-written render functions.
// the solution: we save the timestamp when a handler is attached,
// and also attach the timestamp to any event that was handled by vue
// for the first time (to avoid inconsistent event timestamp implementations
// or events fired from iframes, e.g. #2513)
// The handler would only fire if the event passed to it was fired
// AFTER it was attached.
const timeStamp = e.timeStamp || _getNow()

if (skipTimestampCheck || timeStamp >= invoker.attached - 1) {
callWithAsyncErrorHandling(
patchStopImmediatePropagation(e, invoker.value),
instance,
ErrorCodes.NATIVE_EVENT_HANDLER,
[e]
)
if (!e._vts) {
e._vts = Date.now()

This comment has been minimized.

Sign in to view

Copy link
@robert-wloch-iits

robert-wloch-iits Oct 18, 2022

@yyx990803 This change causes one of my unit tests to fail.
As described in that snippet Vue Composition API: Unit testing custom events of stubbed child components the test emits onDirtyChange with an event value true.

Prior to that e._vts = Date.now() line the test is passing.
With that change the test fails with the follow error log:

 FAIL  tests/unit/components/UnitTestContainerComponent.spec.ts > ContainerComponent.vue > sets NavigationTab's mark-as-dirty attribute to true when ChildComponentA emits "dirty-change"
 ❯ Object.invoker [as onDirtyChange] node_modules/@vue/runtime-dom/dist/runtime-dom.cjs.js:337:20
TypeError: Cannot create property '_vts' on boolean 'true'
 ❯ emitOnCustomEvent tests/unit/components/UnitTestContainerComponent.spec.ts:63:65
     61|   const eventEmittingKey = Object.keys(wrapper.wrapperElement).find((k) => !!wrapper.wrapperElement[k][eventHandler])
     62|   if (eventEmittingKey) {
     63|     await wrapper.wrapperElement[eventEmittingKey][eventHandler](eventValue)
       |                                                                 ^
     64|   } else {
     65|     throw new Error(`Event handler ${eventHandler} not found in wrapper.wrapperElement: ${wrapper.wrapperElement}`)
 ❯ tests/unit/components/UnitTestContainerComponent.spec.ts:24:10

Given the change is a patch level change, I didn't expect a breaking change.

This comment has been minimized.

Sign in to view

Copy link
@yyx990803

yyx990803 Oct 26, 2022

Author Member

The invoker is only attached to native DOM elements and is designed to handle native DOM events, so the code path is never supposed to work with non-Event values. The fact that it worked with boolean values is by accident and should not be considered a part of the API/semver contract.

Your emitOnCustomEvent implementation seems a bit weird since you seem to be trying to trigger a component event handler but instead you are grabbing the native event handler that is attached to the component's root DOM element.

I believe there are cleaner ways to author your tests using vue test utils, but either way your current usage is incorrect (it happened to work, but it really should not).
} else if (e._vts <= invoker.attached) {
return
}
callWithAsyncErrorHandling(
patchStopImmediatePropagation(e, invoker.value),
instance,
ErrorCodes.NATIVE_EVENT_HANDLER,
[e]
)
}
invoker.value = initialValue
invoker.attached = getNow()
Expand Down

0 comments on commit 5ee4053

Please sign in to comment.