Skip to content

Commit

Permalink
fix: resolve event listeners not correct when registered outside of n…
Browse files Browse the repository at this point in the history 
…gZone (#33711)

Close #33687.

PR Close #33711
  • Loading branch information
@JiaLiPassion @kara
JiaLiPassion authored and kara committed Nov 11, 2019
1 parent c540061 commit 9045e3e
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 221 deletions.
6 changes: 3 additions & 3 deletions integration/_payload-limits.json
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"master": { "master": {
"uncompressed": { "uncompressed": {
"runtime-es2015": 1485, "runtime-es2015": 1485,
"main-es2015": 143181, "main-es2015": 141575,
"polyfills-es2015": 36808 "polyfills-es2015": 36808
} }
} }
Expand All @@ -21,7 +21,7 @@
"master": { "master": {
"uncompressed": { "uncompressed": {
"runtime-es2015": 1485, "runtime-es2015": 1485,
"main-es2015": 149315, "main-es2015": 147719,
"polyfills-es2015": 36808 "polyfills-es2015": 36808
} }
} }
Expand Down Expand Up @@ -64,4 +64,4 @@
} }
} }
} }
} }
19 changes: 14 additions & 5 deletions packages/core/test/debug/debug_node_spec.ts
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@


import {CommonModule, NgIfContext, ɵgetDOM as getDOM} from '@angular/common'; import {CommonModule, NgIfContext, ɵgetDOM as getDOM} from '@angular/common';
import {Component, DebugElement, DebugNode, Directive, ElementRef, EmbeddedViewRef, EventEmitter, HostBinding, Injectable, Input, NO_ERRORS_SCHEMA, OnInit, Output, Renderer2, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core'; import {Component, DebugElement, DebugNode, Directive, ElementRef, EmbeddedViewRef, EventEmitter, HostBinding, Injectable, Input, NO_ERRORS_SCHEMA, OnInit, Output, Renderer2, TemplateRef, ViewChild, ViewContainerRef} from '@angular/core';
import {NgZone} from '@angular/core/src/zone';
import {ComponentFixture, TestBed, async} from '@angular/core/testing'; import {ComponentFixture, TestBed, async} from '@angular/core/testing';
import {By} from '@angular/platform-browser/src/dom/debug/by'; import {By} from '@angular/platform-browser/src/dom/debug/by';
import {hasClass} from '@angular/platform-browser/testing/src/browser_util'; import {hasClass} from '@angular/platform-browser/testing/src/browser_util';
Expand Down Expand Up @@ -795,13 +796,21 @@ class TestCmptWithPropInterpolation {
@Component({template: ''}) @Component({template: ''})
class TestComponent implements OnInit { class TestComponent implements OnInit {
count = 0; count = 0;
eventObj: any; eventObj1: any;
constructor(private renderer: Renderer2, private elementRef: ElementRef) {} eventObj2: any;
constructor(
private renderer: Renderer2, private elementRef: ElementRef, private ngZone: NgZone) {}


ngOnInit() { ngOnInit() {
this.renderer.listen(this.elementRef.nativeElement, 'click', (event: any) => { this.renderer.listen(this.elementRef.nativeElement, 'click', (event: any) => {
this.count++; this.count++;
this.eventObj = event; this.eventObj1 = event;
});
this.ngZone.runOutsideAngular(() => {
this.renderer.listen(this.elementRef.nativeElement, 'click', (event: any) => {
this.count++;
this.eventObj2 = event;
});
}); });
} }
} }
Expand All @@ -816,8 +825,8 @@ class TestCmptWithPropInterpolation {
const event = {value: true}; const event = {value: true};
fixture.detectChanges(); fixture.detectChanges();
fixture.debugElement.triggerEventHandler('click', event); fixture.debugElement.triggerEventHandler('click', event);
expect(fixture.componentInstance.count).toBe(1); expect(fixture.componentInstance.count).toBe(2);
expect(fixture.componentInstance.eventObj).toBe(event); expect(fixture.componentInstance.eventObj2).toBe(event);
} }
}); });


Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/view/component_view_spec.ts
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {callMostRecentEventListenerHandler, compViewDef, createAndGetRootNodes,
* We map addEventListener to the Zones internal name. This is because we want to be fast * We map addEventListener to the Zones internal name. This is because we want to be fast
* and bypass the zone bookkeeping. We know that we can do the bookkeeping faster. * and bypass the zone bookkeeping. We know that we can do the bookkeeping faster.
*/ */
const addEventListener = '__zone_symbol__addEventListener' as 'addEventListener'; const addEventListener = 'addEventListener';


{ {
describe(`Component Views`, () => { describe(`Component Views`, () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/view/element_spec.ts
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {ARG_TYPE_VALUES, callMostRecentEventListenerHandler, checkNodeInlineOrDy
* We map addEventListener to the Zones internal name. This is because we want to be fast * We map addEventListener to the Zones internal name. This is because we want to be fast
* and bypass the zone bookkeeping. We know that we can do the bookkeeping faster. * and bypass the zone bookkeeping. We know that we can do the bookkeeping faster.
*/ */
const addEventListener = '__zone_symbol__addEventListener' as 'addEventListener'; const addEventListener = 'addEventListener';
const removeEventListener = '__zone_symbol__removeEventListener' as 'removeEventListener'; const removeEventListener = 'removeEventListener';


{ {
describe(`View Elements`, () => { describe(`View Elements`, () => {
Expand Down
214 changes: 6 additions & 208 deletions packages/platform-browser/src/dom/events/dom_events.ts
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -6,227 +6,25 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */


import {DOCUMENT, isPlatformServer} from '@angular/common'; import {DOCUMENT} from '@angular/common';
import {Inject, Injectable, NgZone, Optional, PLATFORM_ID} from '@angular/core'; import {Inject, Injectable} from '@angular/core';


import {EventManagerPlugin} from './event_manager'; import {EventManagerPlugin} from './event_manager';


/**
* Detect if Zone is present. If it is then use simple zone aware 'addEventListener'
* since Angular can do much more
* efficient bookkeeping than Zone can, because we have additional information. This speeds up
* addEventListener by 3x.
*/
const __symbol__ =
(() => (typeof Zone !== 'undefined') && (Zone as any)['__symbol__'] ||
function(v: string): string { return '__zone_symbol__' + v; })();
const ADD_EVENT_LISTENER: 'addEventListener' = __symbol__('addEventListener');
const REMOVE_EVENT_LISTENER: 'removeEventListener' = __symbol__('removeEventListener');

const symbolNames: {[key: string]: string} = {};

const FALSE = 'FALSE';
const ANGULAR = 'ANGULAR';
const NATIVE_ADD_LISTENER = 'addEventListener';
const NATIVE_REMOVE_LISTENER = 'removeEventListener';

// use the same symbol string which is used in zone.js
const stopSymbol = '__zone_symbol__propagationStopped';
const stopMethodSymbol = '__zone_symbol__stopImmediatePropagation';

const unpatchedMap: {[key: string]: string}|undefined = (() => {
const unpatchedEvents =
(typeof Zone !== 'undefined') && (Zone as any)[__symbol__('UNPATCHED_EVENTS')];
if (unpatchedEvents) {
const unpatchedEventMap: {[eventName: string]: string} = {};
unpatchedEvents.forEach((eventName: string) => { unpatchedEventMap[eventName] = eventName; });
return unpatchedEventMap;
}
return undefined;
})();

const isUnpatchedEvent = function(eventName: string) {
if (!unpatchedMap) {
return false;
}
return unpatchedMap.hasOwnProperty(eventName);
};

interface TaskData {
zone: any;
handler: Function;
}

// a global listener to handle all dom event,
// so we do not need to create a closure every time
const globalListener = function(this: any, event: Event) {
const symbolName = symbolNames[event.type];
if (!symbolName) {
return;
}
const taskDatas: TaskData[] = this[symbolName];
if (!taskDatas) {
return;
}
const args: any = [event];
if (taskDatas.length === 1) {
// if taskDatas only have one element, just invoke it
const taskData = taskDatas[0];
if (taskData.zone !== Zone.current) {
// only use Zone.run when Zone.current not equals to stored zone
return taskData.zone.run(taskData.handler, this, args);
} else {
return taskData.handler.apply(this, args);
}
} else {
// copy tasks as a snapshot to avoid event handlers remove
// itself or others
const copiedTasks = taskDatas.slice();
for (let i = 0; i < copiedTasks.length; i++) {
// if other listener call event.stopImmediatePropagation
// just break
if ((event as any)[stopSymbol] === true) {
break;
}
const taskData = copiedTasks[i];
if (taskData.zone !== Zone.current) {
// only use Zone.run when Zone.current not equals to stored zone
taskData.zone.run(taskData.handler, this, args);
} else {
taskData.handler.apply(this, args);
}
}
}
};

@Injectable() @Injectable()
export class DomEventsPlugin extends EventManagerPlugin { export class DomEventsPlugin extends EventManagerPlugin {
constructor( constructor(@Inject(DOCUMENT) doc: any) { super(doc); }
@Inject(DOCUMENT) doc: any, private ngZone: NgZone,
@Optional() @Inject(PLATFORM_ID) platformId: {}|null) {
super(doc);

if (!platformId || !isPlatformServer(platformId)) {
this.patchEvent();
}
}

private patchEvent() {
if (typeof Event === 'undefined' || !Event || !Event.prototype) {
return;
}
if ((Event.prototype as any)[stopMethodSymbol]) {
// already patched by zone.js
return;
}
const delegate = (Event.prototype as any)[stopMethodSymbol] =
Event.prototype.stopImmediatePropagation;
Event.prototype.stopImmediatePropagation = function(this: any) {
if (this) {
this[stopSymbol] = true;
}

// We should call native delegate in case in some environment part of
// the application will not use the patched Event. Also we cast the
// "arguments" to any since "stopImmediatePropagation" technically does not
// accept any arguments, but we don't know what developers pass through the
// function and we want to not break these calls.
delegate && delegate.apply(this, arguments as any);
};
}


// This plugin should come last in the list of plugins, because it accepts all // This plugin should come last in the list of plugins, because it accepts all
// events. // events.
supports(eventName: string): boolean { return true; } supports(eventName: string): boolean { return true; }


addEventListener(element: HTMLElement, eventName: string, handler: Function): Function { addEventListener(element: HTMLElement, eventName: string, handler: Function): Function {
/** element.addEventListener(eventName, handler as EventListener, false);
* This code is about to add a listener to the DOM. If Zone.js is present, than return () => this.removeEventListener(element, eventName, handler as EventListener);
* `addEventListener` has been patched. The patched code adds overhead in both
* memory and speed (3x slower) than native. For this reason if we detect that
* Zone.js is present we use a simple version of zone aware addEventListener instead.
* The result is faster registration and the zone will be restored.
* But ZoneSpec.onScheduleTask, ZoneSpec.onInvokeTask, ZoneSpec.onCancelTask
* will not be invoked
* We also do manual zone restoration in element.ts renderEventHandlerClosure method.
*
* NOTE: it is possible that the element is from different iframe, and so we
* have to check before we execute the method.
*/
const self = this;
const zoneJsLoaded = element[ADD_EVENT_LISTENER];
let callback: EventListener = handler as EventListener;
// if zonejs is loaded and current zone is not ngZone
// we keep Zone.current on target for later restoration.
if (zoneJsLoaded && (!NgZone.isInAngularZone() || isUnpatchedEvent(eventName))) {
let symbolName = symbolNames[eventName];
if (!symbolName) {
symbolName = symbolNames[eventName] = __symbol__(ANGULAR + eventName + FALSE);
}
let taskDatas: TaskData[] = (element as any)[symbolName];
const globalListenerRegistered = taskDatas && taskDatas.length > 0;
if (!taskDatas) {
taskDatas = (element as any)[symbolName] = [];
}

const zone = isUnpatchedEvent(eventName) ? Zone.root : Zone.current;
if (taskDatas.length === 0) {
taskDatas.push({zone: zone, handler: callback});
} else {
let callbackRegistered = false;
for (let i = 0; i < taskDatas.length; i++) {
if (taskDatas[i].handler === callback) {
callbackRegistered = true;
break;
}
}
if (!callbackRegistered) {
taskDatas.push({zone: zone, handler: callback});
}
}

if (!globalListenerRegistered) {
element[ADD_EVENT_LISTENER](eventName, globalListener, false);
}
} else {
element[NATIVE_ADD_LISTENER](eventName, callback, false);
}
return () => this.removeEventListener(element, eventName, callback);
} }


removeEventListener(target: any, eventName: string, callback: Function): void { removeEventListener(target: any, eventName: string, callback: Function): void {
let underlyingRemove = target[REMOVE_EVENT_LISTENER]; return target.removeEventListener(eventName, callback as EventListener);
// zone.js not loaded, use native removeEventListener
if (!underlyingRemove) {
return target[NATIVE_REMOVE_LISTENER].apply(target, [eventName, callback, false]);
}
let symbolName = symbolNames[eventName];
let taskDatas: TaskData[] = symbolName && target[symbolName];
if (!taskDatas) {
// addEventListener not using patched version
// just call native removeEventListener
return target[NATIVE_REMOVE_LISTENER].apply(target, [eventName, callback, false]);
}
// fix issue 20532, should be able to remove
// listener which was added inside of ngZone
let found = false;
for (let i = 0; i < taskDatas.length; i++) {
// remove listener from taskDatas if the callback equals
if (taskDatas[i].handler === callback) {
found = true;
taskDatas.splice(i, 1);
break;
}
}
if (found) {
if (taskDatas.length === 0) {
// all listeners are removed, we can remove the globalListener from target
underlyingRemove.apply(target, [eventName, globalListener, false]);
}
} else {
// not found in taskDatas, the callback may be added inside of ngZone
// use native remove listener to remove the callback
target[NATIVE_REMOVE_LISTENER].apply(target, [eventName, callback, false]);
}
} }
} }
4 changes: 2 additions & 2 deletions packages/platform-browser/test/dom/events/event_manager_spec.ts
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {createMouseEvent, el} from '../../../testing/src/browser_util';
beforeEach(() => { beforeEach(() => {
doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument(); doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument();
zone = new NgZone({}); zone = new NgZone({});
domEventPlugin = new DomEventsPlugin(doc, zone, null); domEventPlugin = new DomEventsPlugin(doc);
}); });


it('should delegate event bindings to plugins that are passed in from the most generic one to the most specific one', it('should delegate event bindings to plugins that are passed in from the most generic one to the most specific one',
Expand Down Expand Up @@ -322,7 +322,7 @@ import {createMouseEvent, el} from '../../../testing/src/browser_util';
it('should only trigger one Change detection when bubbling', (done: DoneFn) => { it('should only trigger one Change detection when bubbling', (done: DoneFn) => {
doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument(); doc = getDOM().supportsDOMEvents() ? document : getDOM().createHtmlDocument();
zone = new NgZone({shouldCoalesceEventChangeDetection: true}); zone = new NgZone({shouldCoalesceEventChangeDetection: true});
domEventPlugin = new DomEventsPlugin(doc, zone, null); domEventPlugin = new DomEventsPlugin(doc);
const element = el('<div></div>'); const element = el('<div></div>');
const child = el('<div></div>'); const child = el('<div></div>');
element.appendChild(child); element.appendChild(child);
Expand Down

0 comments on commit 9045e3e

Please sign in to comment.