From ee46cdae1fe61e4cbe96b9aa8a55e5a2be706f88 Mon Sep 17 00:00:00 2001 From: "JiaLi.Passion" Date: Sat, 22 Apr 2017 09:18:27 +0900 Subject: [PATCH 1/7] fix(patch): fix #746, need to patch all on properties without duplicate --- lib/common/utils.ts | 29 ++++-- test/browser/browser.spec.ts | 196 +++++++++++++++++++---------------- 2 files changed, 126 insertions(+), 99 deletions(-) diff --git a/lib/common/utils.ts b/lib/common/utils.ts index 7e74ea1db..0e8b33fb3 100644 --- a/lib/common/utils.ts +++ b/lib/common/utils.ts @@ -126,7 +126,7 @@ export function patchProperty(obj: any, prop: string) { } if (target.hasOwnProperty(_prop)) { return target[_prop]; - } else { + } else if (originalDescGet) { // result will be null when use inline event attribute, // such as // because the onclick function is internal raw uncompiled handler @@ -140,27 +140,34 @@ export function patchProperty(obj: any, prop: string) { } return value; } + return null; }; Object.defineProperty(obj, prop, desc); } export function patchOnProperties(obj: any, properties: string[]) { + const hash: any = {}; if (properties) { for (let i = 0; i < properties.length; i++) { - patchProperty(obj, 'on' + properties[i]); + const prop = 'on' + properties[i]; + hash[prop] = true; } - } else { - const onProperties = []; - for (const prop in obj) { - if (prop.substr(0, 2) == 'on') { - onProperties.push(prop); - } - } - for (let j = 0; j < onProperties.length; j++) { - patchProperty(obj, onProperties[j]); + } + + const onProperties = []; + for (const prop in obj) { + if (prop.substr(0, 2) == 'on') { + onProperties.push(prop); } } + for (let j = 0; j < onProperties.length; j++) { + hash[onProperties[j]] = true; + } + + Object.keys(hash).forEach(prop => { + patchProperty(obj, prop); + }); } const EVENT_TASKS = zoneSymbol('eventTasks'); diff --git a/test/browser/browser.spec.ts b/test/browser/browser.spec.ts index cf83eb01c..16511db28 100644 --- a/test/browser/browser.spec.ts +++ b/test/browser/browser.spec.ts @@ -84,94 +84,114 @@ describe('Zone', function() { expect(confirmSpy).toHaveBeenCalledWith('confirmMsg'); }); - describe('DOM onProperty hooks', ifEnvSupports(canPatchOnProperty, function() { - let mouseEvent = document.createEvent('Event'); - let hookSpy: Spy, eventListenerSpy: Spy; - const zone = rootZone.fork({ - name: 'spy', - onScheduleTask: (parentZoneDelegate: ZoneDelegate, currentZone: Zone, - targetZone: Zone, task: Task): any => { - hookSpy(); - return parentZoneDelegate.scheduleTask(targetZone, task); - } - }); - - beforeEach(function() { - mouseEvent.initEvent('mousedown', true, true); - hookSpy = jasmine.createSpy('hook'); - eventListenerSpy = jasmine.createSpy('eventListener'); - }); - - it('window onclick should be in zone', - ifEnvSupports( - () => { - return canPatchOnProperty(window, 'onmousedown'); - }, - function() { - zone.run(function() { - window.onmousedown = eventListenerSpy; - }); - - window.dispatchEvent(mouseEvent); - - expect(hookSpy).toHaveBeenCalled(); - expect(eventListenerSpy).toHaveBeenCalled(); - window.removeEventListener('mousedown', eventListenerSpy); - })); - - it('document onclick should be in zone', - ifEnvSupports( - () => { - return canPatchOnProperty(Document.prototype, 'onmousedown'); - }, - function() { - zone.run(function() { - document.onmousedown = eventListenerSpy; - }); - - document.dispatchEvent(mouseEvent); - - expect(hookSpy).toHaveBeenCalled(); - expect(eventListenerSpy).toHaveBeenCalled(); - document.removeEventListener('mousedown', eventListenerSpy); - })); - - it('SVGElement onclick should be in zone', - ifEnvSupports( - () => { - return typeof SVGElement !== 'undefined' && - canPatchOnProperty(SVGElement.prototype, 'onmousedown'); - }, - function() { - const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); - document.body.appendChild(svg); - zone.run(function() { - svg.onmousedown = eventListenerSpy; - }); - - svg.dispatchEvent(mouseEvent); - - expect(hookSpy).toHaveBeenCalled(); - expect(eventListenerSpy).toHaveBeenCalled(); - svg.removeEventListener('mouse', eventListenerSpy); - document.body.removeChild(svg); - })); - - it('get window onerror should not throw error', - ifEnvSupports( - () => { - return canPatchOnProperty(window, 'onerror'); - }, - function() { - const testFn = function() { - let onerror = window.onerror; - window.onerror = function() {}; - onerror = window.onerror; - }; - expect(testFn()).not.toThrow(); - })); - - })); + describe( + 'DOM onProperty hooks', + ifEnvSupports( + () => { + return canPatchOnProperty(HTMLElement.prototype, 'onclick'); + }, + function() { + let mouseEvent = document.createEvent('Event'); + let hookSpy: Spy, eventListenerSpy: Spy; + const zone = rootZone.fork({ + name: 'spy', + onScheduleTask: (parentZoneDelegate: ZoneDelegate, currentZone: Zone, + targetZone: Zone, task: Task): any => { + hookSpy(); + return parentZoneDelegate.scheduleTask(targetZone, task); + } + }); + + beforeEach(function() { + mouseEvent.initEvent('mousedown', true, true); + hookSpy = jasmine.createSpy('hook'); + eventListenerSpy = jasmine.createSpy('eventListener'); + }); + + it('window onclick should be in zone', + ifEnvSupports( + () => { + return canPatchOnProperty(window, 'onmousedown'); + }, + function() { + zone.run(function() { + window.onmousedown = eventListenerSpy; + }); + + window.dispatchEvent(mouseEvent); + + expect(hookSpy).toHaveBeenCalled(); + expect(eventListenerSpy).toHaveBeenCalled(); + window.removeEventListener('mousedown', eventListenerSpy); + })); + + it('window onresize should be patched', + ifEnvSupports( + () => { + return canPatchOnProperty(window, 'onmousedown'); + }, + function() { + window.onresize = eventListenerSpy; + const innerResizeProp: any = (window as any)[zoneSymbol('_onresize')]; + expect(innerResizeProp).toBeTruthy(); + innerResizeProp(); + expect(eventListenerSpy).toHaveBeenCalled(); + window.removeEventListener('resize', eventListenerSpy); + })); + + it('document onclick should be in zone', + ifEnvSupports( + () => { + return canPatchOnProperty(Document.prototype, 'onmousedown'); + }, + function() { + zone.run(function() { + document.onmousedown = eventListenerSpy; + }); + + document.dispatchEvent(mouseEvent); + + expect(hookSpy).toHaveBeenCalled(); + expect(eventListenerSpy).toHaveBeenCalled(); + document.removeEventListener('mousedown', eventListenerSpy); + })); + + it('SVGElement onclick should be in zone', + ifEnvSupports( + () => { + return typeof SVGElement !== 'undefined' && + canPatchOnProperty(SVGElement.prototype, 'onmousedown'); + }, + function() { + const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); + document.body.appendChild(svg); + zone.run(function() { + svg.onmousedown = eventListenerSpy; + }); + + svg.dispatchEvent(mouseEvent); + + expect(hookSpy).toHaveBeenCalled(); + expect(eventListenerSpy).toHaveBeenCalled(); + svg.removeEventListener('mouse', eventListenerSpy); + document.body.removeChild(svg); + })); + + it('get window onerror should not throw error', + ifEnvSupports( + () => { + return canPatchOnProperty(window, 'onerror'); + }, + function() { + const testFn = function() { + let onerror = window.onerror; + window.onerror = function() {}; + onerror = window.onerror; + }; + expect(testFn).not.toThrow(); + })); + + })); describe('eventListener hooks', function() { let button: HTMLButtonElement; From 447e062886269390b0570a91f5650a205ec83c5e Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 22 Apr 2017 11:17:46 +0900 Subject: [PATCH 2/7] fix(patch): should check originalDesc.get value is null --- lib/common/utils.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/common/utils.ts b/lib/common/utils.ts index 0e8b33fb3..be3ebd51e 100644 --- a/lib/common/utils.ts +++ b/lib/common/utils.ts @@ -134,11 +134,13 @@ export function patchProperty(obj: any, prop: string) { // the property is accessed, https://github.com/angular/zone.js/issues/525 // so we should use original native get to retrieve the handler let value = originalDescGet.apply(this); - value = desc.set.apply(this, [value]); - if (typeof target['removeAttribute'] === 'function') { - target.removeAttribute(prop); + if (value) { + desc.set.apply(this, [value]); + if (typeof target['removeAttribute'] === 'function') { + target.removeAttribute(prop); + } + return value; } - return value; } return null; }; From 17e7c7d08dd24309fdb472a6075300e5a6a4678f Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 22 Apr 2017 12:32:40 +0900 Subject: [PATCH 3/7] fix(patch): revert to original version --- lib/common/utils.ts | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/lib/common/utils.ts b/lib/common/utils.ts index be3ebd51e..499e11294 100644 --- a/lib/common/utils.ts +++ b/lib/common/utils.ts @@ -149,27 +149,21 @@ export function patchProperty(obj: any, prop: string) { } export function patchOnProperties(obj: any, properties: string[]) { - const hash: any = {}; if (properties) { for (let i = 0; i < properties.length; i++) { - const prop = 'on' + properties[i]; - hash[prop] = true; + patchProperty(obj, 'on' + properties[i]); } - } - - const onProperties = []; - for (const prop in obj) { - if (prop.substr(0, 2) == 'on') { - onProperties.push(prop); + } else { + const onProperties = []; + for (const prop in obj) { + if (prop.substr(0, 2) == 'on') { + onProperties.push(prop); + } + } + for (let j = 0; j < onProperties.length; j++) { + patchProperty(obj, onProperties[j]); } } - for (let j = 0; j < onProperties.length; j++) { - hash[onProperties[j]] = true; - } - - Object.keys(hash).forEach(prop => { - patchProperty(obj, prop); - }); } const EVENT_TASKS = zoneSymbol('eventTasks'); From 638944959aab816991f59b14089fd4d2cc0cef75 Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 22 Apr 2017 12:36:04 +0900 Subject: [PATCH 4/7] fix(patch): revert to original version --- test/browser/browser.spec.ts | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/test/browser/browser.spec.ts b/test/browser/browser.spec.ts index 16511db28..13ce15bcb 100644 --- a/test/browser/browser.spec.ts +++ b/test/browser/browser.spec.ts @@ -125,20 +125,6 @@ describe('Zone', function() { window.removeEventListener('mousedown', eventListenerSpy); })); - it('window onresize should be patched', - ifEnvSupports( - () => { - return canPatchOnProperty(window, 'onmousedown'); - }, - function() { - window.onresize = eventListenerSpy; - const innerResizeProp: any = (window as any)[zoneSymbol('_onresize')]; - expect(innerResizeProp).toBeTruthy(); - innerResizeProp(); - expect(eventListenerSpy).toHaveBeenCalled(); - window.removeEventListener('resize', eventListenerSpy); - })); - it('document onclick should be in zone', ifEnvSupports( () => { From 70c7f7d7d2d15e16105adc212d96aa129968c492 Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 22 Apr 2017 12:49:18 +0900 Subject: [PATCH 5/7] fix(patch): patch resize of window --- lib/browser/property-descriptor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/browser/property-descriptor.ts b/lib/browser/property-descriptor.ts index 09e9f9fdc..86d369cac 100644 --- a/lib/browser/property-descriptor.ts +++ b/lib/browser/property-descriptor.ts @@ -23,7 +23,7 @@ export function propertyDescriptorPatch(_global: any) { if (canPatchViaPropertyDescriptor()) { // for browsers that we can patch the descriptor: Chrome & Firefox if (isBrowser) { - patchOnProperties(window, eventNames); + patchOnProperties(window, eventNames.concat(['resize'])); patchOnProperties(Document.prototype, eventNames); if (typeof(window)['SVGElement'] !== 'undefined') { patchOnProperties((window)['SVGElement'].prototype, eventNames); From 9fdeabe703d7ffa7be59f56dd6efefc73f9a774b Mon Sep 17 00:00:00 2001 From: "JiaLi.Passion" Date: Sun, 23 Apr 2017 01:25:56 +0900 Subject: [PATCH 6/7] fix(test): refactor test cases for patch on properties --- test/browser/browser.spec.ts | 197 +++++++++++++++++------------------ 1 file changed, 95 insertions(+), 102 deletions(-) diff --git a/test/browser/browser.spec.ts b/test/browser/browser.spec.ts index 13ce15bcb..c2ccdfe18 100644 --- a/test/browser/browser.spec.ts +++ b/test/browser/browser.spec.ts @@ -21,17 +21,20 @@ function promiseUnhandleRejectionSupport() { } function canPatchOnProperty(obj: any, prop: string) { - if (!obj) { - return false; - } - const desc = Object.getOwnPropertyDescriptor(obj, prop); - if (!desc || !desc.configurable) { - return false; - } - return true; -} + const func = function() { + if (!obj) { + return false; + } + const desc = Object.getOwnPropertyDescriptor(obj, prop); + if (!desc || !desc.configurable) { + return false; + } + return true; + }; -(canPatchOnProperty as any).message = 'patchOnProperties'; + (func as any).message = 'patchOnProperties'; + return func; +} let supportsPassive = false; try { @@ -86,98 +89,88 @@ describe('Zone', function() { describe( 'DOM onProperty hooks', - ifEnvSupports( - () => { - return canPatchOnProperty(HTMLElement.prototype, 'onclick'); - }, - function() { - let mouseEvent = document.createEvent('Event'); - let hookSpy: Spy, eventListenerSpy: Spy; - const zone = rootZone.fork({ - name: 'spy', - onScheduleTask: (parentZoneDelegate: ZoneDelegate, currentZone: Zone, - targetZone: Zone, task: Task): any => { - hookSpy(); - return parentZoneDelegate.scheduleTask(targetZone, task); - } - }); - - beforeEach(function() { - mouseEvent.initEvent('mousedown', true, true); - hookSpy = jasmine.createSpy('hook'); - eventListenerSpy = jasmine.createSpy('eventListener'); - }); - - it('window onclick should be in zone', - ifEnvSupports( - () => { - return canPatchOnProperty(window, 'onmousedown'); - }, - function() { - zone.run(function() { - window.onmousedown = eventListenerSpy; - }); - - window.dispatchEvent(mouseEvent); - - expect(hookSpy).toHaveBeenCalled(); - expect(eventListenerSpy).toHaveBeenCalled(); - window.removeEventListener('mousedown', eventListenerSpy); - })); - - it('document onclick should be in zone', - ifEnvSupports( - () => { - return canPatchOnProperty(Document.prototype, 'onmousedown'); - }, - function() { - zone.run(function() { - document.onmousedown = eventListenerSpy; - }); - - document.dispatchEvent(mouseEvent); - - expect(hookSpy).toHaveBeenCalled(); - expect(eventListenerSpy).toHaveBeenCalled(); - document.removeEventListener('mousedown', eventListenerSpy); - })); - - it('SVGElement onclick should be in zone', - ifEnvSupports( - () => { - return typeof SVGElement !== 'undefined' && - canPatchOnProperty(SVGElement.prototype, 'onmousedown'); - }, - function() { - const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); - document.body.appendChild(svg); - zone.run(function() { - svg.onmousedown = eventListenerSpy; - }); - - svg.dispatchEvent(mouseEvent); - - expect(hookSpy).toHaveBeenCalled(); - expect(eventListenerSpy).toHaveBeenCalled(); - svg.removeEventListener('mouse', eventListenerSpy); - document.body.removeChild(svg); - })); - - it('get window onerror should not throw error', - ifEnvSupports( - () => { - return canPatchOnProperty(window, 'onerror'); - }, - function() { - const testFn = function() { - let onerror = window.onerror; - window.onerror = function() {}; - onerror = window.onerror; - }; - expect(testFn).not.toThrow(); - })); - - })); + ifEnvSupports(canPatchOnProperty(HTMLElement.prototype, 'onclick'), function() { + let mouseEvent = document.createEvent('Event'); + let hookSpy: Spy, eventListenerSpy: Spy; + const zone = rootZone.fork({ + name: 'spy', + onScheduleTask: (parentZoneDelegate: ZoneDelegate, currentZone: Zone, targetZone: Zone, + task: Task): any => { + hookSpy(); + return parentZoneDelegate.scheduleTask(targetZone, task); + } + }); + + beforeEach(function() { + mouseEvent.initEvent('mousedown', true, true); + hookSpy = jasmine.createSpy('hook'); + eventListenerSpy = jasmine.createSpy('eventListener'); + }); + + it('window onclick should be in zone', + ifEnvSupports(canPatchOnProperty(window, 'onmousedown'), function() { + zone.run(function() { + window.onmousedown = eventListenerSpy; + }); + + window.dispatchEvent(mouseEvent); + + expect(hookSpy).toHaveBeenCalled(); + expect(eventListenerSpy).toHaveBeenCalled(); + window.removeEventListener('mousedown', eventListenerSpy); + })); + + it('window onresize should be patched', + ifEnvSupports(canPatchOnProperty(window, 'onmousedown'), function() { + window.onresize = eventListenerSpy; + const innerResizeProp: any = (window as any)[zoneSymbol('_onresize')]; + expect(innerResizeProp).toBeTruthy(); + innerResizeProp(); + expect(eventListenerSpy).toHaveBeenCalled(); + window.removeEventListener('resize', eventListenerSpy); + })); + + it('document onclick should be in zone', + ifEnvSupports(canPatchOnProperty(Document.prototype, 'onmousedown'), function() { + zone.run(function() { + document.onmousedown = eventListenerSpy; + }); + + document.dispatchEvent(mouseEvent); + + expect(hookSpy).toHaveBeenCalled(); + expect(eventListenerSpy).toHaveBeenCalled(); + document.removeEventListener('mousedown', eventListenerSpy); + })); + + it('SVGElement onclick should be in zone', + ifEnvSupports( + canPatchOnProperty(SVGElement && SVGElement.prototype, 'onmousedown'), function() { + const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); + document.body.appendChild(svg); + zone.run(function() { + svg.onmousedown = eventListenerSpy; + }); + + svg.dispatchEvent(mouseEvent); + + expect(hookSpy).toHaveBeenCalled(); + expect(eventListenerSpy).toHaveBeenCalled(); + svg.removeEventListener('mouse', eventListenerSpy); + document.body.removeChild(svg); + })); + + it('get window onerror should not throw error', + ifEnvSupports(canPatchOnProperty(window, 'onerror'), function() { + const testFn = function() { + let onerror = window.onerror; + window.onerror = function() {}; + onerror = window.onerror; + }; + expect(testFn).not.toThrow(); + })); + + })); describe('eventListener hooks', function() { let button: HTMLButtonElement; From d20d92348aef8af7cc344cb4a47ab26dd2ecbbdc Mon Sep 17 00:00:00 2001 From: "JiaLi.Passion" Date: Sun, 23 Apr 2017 08:28:13 +0900 Subject: [PATCH 7/7] fix(patch): add test case for first time access no descriptor on property --- test/browser/XMLHttpRequest.spec.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/browser/XMLHttpRequest.spec.ts b/test/browser/XMLHttpRequest.spec.ts index 1f92489a8..4944488d3 100644 --- a/test/browser/XMLHttpRequest.spec.ts +++ b/test/browser/XMLHttpRequest.spec.ts @@ -61,6 +61,11 @@ describe('XMLHttpRequest', function() { req.send(); }); + it('should return null when access ontimeout first time without error', function() { + let req: XMLHttpRequest = new XMLHttpRequest(); + expect(req.ontimeout).toBe(null); + }); + const supportsOnProgress = function() { return 'onprogress' in new XMLHttpRequest(); };