The Wayback Machine - https://web.archive.org/web/20190322181116/https://github.com/angular/angular/pull/18993
Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(platform-browser): simple version of zone aware addEventListener #18993

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@JiaLiPassion
Copy link
Contributor

JiaLiPassion commented Sep 1, 2017

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: 18753

What is the new behavior?

EventHandler will run into the correct zone which will be the zone when addEventListener is invoked

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

fix #18753.

Add a simple version of zone aware addEventListener.

  1. create a globalEventHandler to handle all events to avoid creating to many closure function.
  2. keep zone when addEventListener, but for better performance, we don't use zone.js mechanism to scheduleEventTask, so ZoneSpec.onInvokeTask, ZoneSpec.onScheduleTask, ZoneSpec.onCancelTask will not be invoked.
  3. when call addEventListener, if the Zone.current is ngZone, we just use native addEventListener for better performance (which will be the most cases).
  4. Add blackListedEvents so user can decide which events they want to run outside of ngZone.
    Here is a sample to run all scroll event handler outside of ngZone.
// before load polyfill.js
<script>
// black list scroll event handler for addEventListener
Zone[Zone.__symbol__('BLACK_LISTED_EVENTS')] = ['scroll'];
// black list scroll event handler for onscroll
const targets = [window, Document.prototype, HTMLBodyElement.prototype, HTMLElement.prototype];
 __Zone_ignore_on_properties = [];
targets.forEach(function(target) {
  __Zone_ignore_on_properties.push({
    target: target,
    ignoreProperties: ['scroll']
  });
});
</script>

@mhevery , please review.

@googlebot googlebot added the cla: yes label Sep 1, 2017

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:event branch from 8141be5 to 4ab2c5b Sep 1, 2017

if (!blackListedEvents) {
return false;
}
return blackListedEvents.filter(blackListedEvent => blackListedEvent === eventName).length > 0;

This comment has been minimized.

@mhevery

mhevery Sep 1, 2017

Member

Could we turn the events into map. Calling filter all the time has negative perf implication. This would be faster. blackListMap.hasOwnProperty(eventName)

This comment has been minimized.

@JiaLiPassion

JiaLiPassion Sep 2, 2017

Author Contributor

@mhevery , got it, I will use map to check.

};
// if zonejs is loaded and current zone is not ngZone
// we keep Zone.current on target for later restoration.
const isBlackListed = isBlackListedEvent(eventName);

This comment has been minimized.

@mhevery

mhevery Sep 1, 2017

Member

PERF: We should delay checking if it is black-listed until after we know we have Zone.

This comment has been minimized.

@JiaLiPassion

JiaLiPassion Sep 2, 2017

Author Contributor

@mhevery , ok, I will check it after zoneJsLoaded is true.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:event branch from 8bd5352 to eaab36c Sep 2, 2017

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Sep 2, 2017

@mhevery , I have updated the commits.

  1. use blackListedMap to check eventName is blackListed or not.
  2. check isBlackListedEvent after zoneJsLoaded is true.
  3. add test cases for add multiple listeners and add duplicate listener.

please review.

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

const blackListedMap: {[key: string]: string} = Zone && Zone[__symbol__('BLACK_LISTED_EVENTS')];

This comment has been minimized.

@mhevery

mhevery Sep 2, 2017

Member

Sorry I meant that you would convert the array into the map, rather than expect that the data is provided as a map.

This comment has been minimized.

@JiaLiPassion

JiaLiPassion Sep 2, 2017

Author Contributor

@mhevery , got it, sorry for misunderstanding, I have updated the source, please review.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:event branch from eaab36c to 6c01b2c Sep 2, 2017

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Sep 2, 2017

@mhevery , I have updated the source and write a sample code to describe how to set blackListedEvents for both addEventListener and onProperties, maybe we should document this somewhere.

Please review.

@mhevery

mhevery approved these changes Sep 2, 2017

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Sep 5, 2017

@mhevery , I found a bug which is the same one with angular/zone.js#839,

the issue can be described as the following case.

const handler1 = (e: any)=> {
        // handler remove itself 
        remover1 && remover1();
 };

const handler2 = (e: any) => {
   console.log('handler2');
};

const manager = new EventManager([domEventPlugin], new FakeNgZone());
remover1 = manager.addEventListener(element, 'click', handler1); 
remover2 = manager.addEventListener(element, 'click', handler2);
 
 getDOM().dispatchEvent(element, dispatchedEvent);

in previous version, handler2 will not be invoked because we remove the handler1 in handler1.

The modified code in dom_event.ts is copy the listeners array if necessary(only when listeners array length > 1)

// a global listener to handle all dom event,
// so we do not need to create a closure everytime
const globalListener = function(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++) {
      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);
      }
    }
  }
};

it is the same with angular/zone.js#839 , please review.

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:event branch from 6c01b2c to 7ed0c53 Sep 5, 2017

@JiaLiPassion JiaLiPassion force-pushed the JiaLiPassion:event branch from 7ed0c53 to 5bcd96e Sep 5, 2017

@mhevery mhevery closed this in ed1175f Sep 6, 2017

@PierreDuc

This comment has been minimized.

Copy link

PierreDuc commented Sep 15, 2017

Little late to the party, but doesn't the introduction of a globalEventHandler introduce a regression with using stopImmediatePropagation. This won't work anymore, because all the handlers on the same element will be called regardless. I also see issues once it's possible to add event options like capture, passive and definitely once.

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Sep 15, 2017

@PierreDuc ,

  1. about the stopImmediatePropagation, I will check it
  2. about the capture, passive, once, before this PR, those options have not been handled either, so I don't think this PR bring more issues.
@PierreDuc

This comment has been minimized.

Copy link

PierreDuc commented Sep 15, 2017

@JiaLiPassion I guess sample code will be fine:

Let's say your AppComponent has this template:

<app-foo></app-foo>
<app-foo></app-foo>

And your FooComponent looks like this:

@Component({
  selector: 'app-foo',
  template: ``
})
export class FooComponent {
 
  @HostListener('document:click', ['$event'])
  public onClick(event: MouseEvent): void {
      console.log('document has been clicked');
      event.stopImmediatePropagation();
  }

}

Then even though you call stopImmediatePropagation, the listener is called twice. I would expect only the first listener to be called.

@JiaLiPassion

This comment has been minimized.

Copy link
Contributor Author

JiaLiPassion commented Sep 15, 2017

@PierreDuc , got it, thank you, I can fix this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.