Skip to content

fix(core): should use native addEventListener in ngZone#20672

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:nativeevent
Closed

fix(core): should use native addEventListener in ngZone#20672
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:nativeevent

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion commented Nov 28, 2017 •
edited
Loading

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: 20673
fix #20673

in #18993, there is an optimized version of addEventListener, but I used the wrong addEventListener when we run in ngZone, we should use native addEventListener to get better performance when the handler was added in ngZone.

And the other issue is in #18993, we support BLACK_LISTED_EVENTS, and if an event is blacklisted, it should not run in angular zone, but current version it will not work.

What is the new behavior?

When event handler was added in angular zone, use non patched addEventListener to get better performance.

And BLACK_LISTED_EVENTS should work and run outside of angular zone.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@mhevery , please review, thank you!

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release area: core Issues related to the framework runtime labels Nov 28, 2017
@JiaLiPassion JiaLiPassion force-pushed the nativeevent branch 4 times, most recently from 404b3b5 to b5bb8c1 Compare November 29, 2017 04:05
@mhevery mhevery closed this in 65a2cb8 Nov 29, 2017
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Nov 29, 2017 •
edited
Loading

This was reverted in ba850b3.

It breaks automated testing because it changes whether the callback is invoked in the Zone. Needs more investigation.

@mhevery mhevery reopened this Nov 29, 2017
mhevery added a commit that referenced this pull request Nov 29, 2017
@mhevery
Revert "fix(core): should use native addEventListener in ngZone (#20672…
ba850b3 
…)"

This reverts commit 65a2cb8.
@mhevery mhevery removed the action: merge The PR is ready for merge by the caretaker label Nov 29, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Dec 2, 2017
@JiaLiPassion @wKoza
fix(core): should use native addEventListener in ngZone (angular#20672)
76c7a4b 
PR Close angular#20672
wKoza pushed a commit to wKoza/angular that referenced this pull request Dec 2, 2017
@mhevery @wKoza
Revert "fix(core): should use native addEventListener in ngZone (angu…
1765736 
…lar#20672)"

This reverts commit 65a2cb8.
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Dec 8, 2017

@mhevery , I have updated the PR.

  • make a global eventTask only for trigger onScheduleTask/onInvokeTask/onCancelTask callback of NgZoneSpec. And use global eventTask (not schedule a new one every time) to get better performance.
  • because we use global event task, so I use some zone.js internal hacking method to make it work.

please review.

vikerman
vikerman suggested changes Dec 9, 2017
View reviewed changes
Comment thread packages/platform-browser/src/dom/events/dom_events.ts Outdated
Copy link
Copy Markdown
Contributor

@vikerman vikerman Dec 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name is undefined - Meant to be eventName?

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Dec 9, 2017

@vikerman , thank you , I have changed the code, please review.

@JiaLiPassion JiaLiPassion force-pushed the nativeevent branch 5 times, most recently from 6dc02ca to f937c26 Compare January 12, 2018 01:06
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Jan 12, 2018

@mhevery , @vikerman , I have updated the PR, please review and re-run the internal test cases, thank you!

@mhevery mhevery self-assigned this Jan 16, 2018
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jan 16, 2018

@JiaLiPassion @vikerman is on vacation until next week, so I don't think much will happen here.

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Jan 18, 2018

@JiaLiPassion @vikerman is on vacation until next week, so I don't think much will happen here.

@mhevery , got it! thank you.

@JiaLiPassion JiaLiPassion mentioned this pull request Jan 21, 2018
Closed
3 tasks
@glebmachine
Copy link
Copy Markdown

glebmachine commented Jan 22, 2018

@vikerman looking forward your review and merge 🤤

@ngbot
Copy link
Copy Markdown

ngbot Bot commented Jan 22, 2018

Hi @JiaLiPassion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@JiaLiPassion JiaLiPassion force-pushed the nativeevent branch 2 times, most recently from 17ceec1 to 74fc746 Compare January 26, 2018 08:10
@ngbot
Copy link
Copy Markdown

ngbot Bot commented May 31, 2018

Hi @JiaLiPassion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@jasonaden jasonaden added this to the needsTriage milestone Jan 29, 2019
@JiaLiPassion JiaLiPassion mentioned this pull request Nov 10, 2019
Closed
14 tasks
@JiaLiPassion JiaLiPassion mentioned this pull request Jul 26, 2020
Closed
12 tasks
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Jul 29, 2020
@JiaLiPassion
feat(platform-browser): can config zone/once/passive/capture in event…
0efb517 
… listener.

Close angular#19878

1. Can't pass `passive`, `capture`, `once` parameters to `addEventListener`.
2. Not easy to config use ngzone or noopzone with HostListener

user need to explicitly call `runOutsideOfAngular` to make sure  `eventhandler` run outsideof`ngZone`

```
@HostListener('window:resize', ['$event.target'])
  onResize(target: any) {
    this.ngZone.runOutsideOfAngular(() => {
       console.log('resize triggered');
    });
  }
```
or call `ngZone.run` explicitly make sure `eventhandler` run into `ngZone`.

```
@HostListener('window:resize', ['$event.target'])
  onResize(target: any) {
    this.ngZone.run(() => {
       console.log('resize triggered');
    });
  }
```

```
<div (mouseover)="mouseover();"></div>
```

```
mouseover() {
  this.ngZone.runOfAngular(() => {
    ...
  });
}
```

3. Can config which `zone` will event handler will run into in `@HostListener` decorator.
4. can config `passive`, `capture`, `once` parameters to `addEventListener`.

- in template, can config those parameter like below.

```
<div (mouseover.capture.once.passive)="mouseover();"></div>
```

even the handler was added not in `ngZone`.

```
<div (mouseover.ngZone)="mouseover();"></div>
```

```
 @HostListener('window:resize.ngZone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }
```

- Can config `noopZone` which will guarantee eventhandler run outside of `ngZone`
even the handler was added  in `ngZone`.

And it can also work with angular#20672 to get better performance.

```
<div (mouseover.noopZone)="mouseover();"></div>
```

```
 @HostListener('window:resize.noopZone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }
```
JiaLiPassion added a commit to JiaLiPassion/angular that referenced this pull request Jul 29, 2020
@JiaLiPassion
feat(platform-browser): can config zone/once/passive/capture in event…
d20b7d0 
… listener.

Close angular#19878

1. Can't pass `passive`, `capture`, `once` parameters to `addEventListener`.
2. Not easy to config use ngzone or noopzone with HostListener

user need to explicitly call `runOutsideOfAngular` to make sure  `eventhandler` run outsideof`ngZone`

```
@HostListener('window:resize', ['$event.target'])
  onResize(target: any) {
    this.ngZone.runOutsideOfAngular(() => {
       console.log('resize triggered');
    });
  }
```
or call `ngZone.run` explicitly make sure `eventhandler` run into `ngZone`.

```
@HostListener('window:resize', ['$event.target'])
  onResize(target: any) {
    this.ngZone.run(() => {
       console.log('resize triggered');
    });
  }
```

```
<div (mouseover)="mouseover();"></div>
```

```
mouseover() {
  this.ngZone.runOfAngular(() => {
    ...
  });
}
```

3. Can config which `zone` will event handler will run into in `@HostListener` decorator.
4. can config `passive`, `capture`, `once` parameters to `addEventListener`.

- in template, can config those parameter like below.

```
<div (mouseover.capture.once.passive)="mouseover();"></div>
```

even the handler was added not in `ngZone`.

```
<div (mouseover.ngZone)="mouseover();"></div>
```

```
 @HostListener('window:resize.ngZone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }
```

- Can config `noopZone` which will guarantee eventhandler run outside of `ngZone`
even the handler was added  in `ngZone`.

And it can also work with angular#20672 to get better performance.

```
<div (mouseover.noopZone)="mouseover();"></div>
```

```
 @HostListener('window:resize.noopZone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }
```
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Nov 19, 2020

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Nov 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@vikerman vikerman vikerman requested changes

@mhevery mhevery mhevery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mhevery mhevery

Labels

area: core Issues related to the framework runtime cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

BLACK_LISTED_EVENTS not work

6 participants

@JiaLiPassion @mhevery @glebmachine @vikerman @jasonaden @googlebot