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

[JiaLiPassion]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• [] Docs have been added / updated (for bug fixes / features)

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.

[mhevery]

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

[JiaLiPassion]

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

[mhevery]

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

[JiaLiPassion]

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

[JiaLiPassion]

@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.

[mhevery]

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

[JiaLiPassion]

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

[JiaLiPassion]

@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.

[JiaLiPassion]

@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.

[PierreDuc]

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]

@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]

@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]

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

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}