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

Huge performance impact on Component's host document click #11857

Open
Namek opened this Issue Sep 23, 2016 · 13 comments

Comments

Projects
None yet
5 participants
@Namek
Copy link

Namek commented Sep 23, 2016

I'm submitting a ... (check one with "x")

[x] bug report
[ ] feature request
[ ] support request

Problem
Performance impact when having few hundres instantiations of same component that registers on document.click event through host property in @Component directive.

@Component({
  selector: 'value-edit',
  template: ``,
  host: {
    "(document: click)": "onClickOff($event)",
  }
)
export class ValueEditComponent {
  onClickOff(globalEvent) {
    // to make sure it doesn't affect performance we keep it EMPTY!
  }
}

Behaviour

Every single click wherever on the page (document) takes big amount of time, like 2-3 seconds in my case. The screenshot below shows a sequence of: wait ~5 seconds, click, wait few seconds and stop recording. The click is the huge green column on the screenshot:

tnft3

Expected behaviour
Every single click shouldn't take seconds of processing.

Reproduction of the problem

Plunker here

Just commenting out "(document: click)": "onClickOff($event)", fixes the performance issue on click.

Environment:

  • Angular version: 2.0.0
  • Browser: latest stable Chrome 53.0.2785.116 m
  • Language: TypeScript 2.0.x

posted StackOverflow question

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Sep 23, 2016

You want to add a global event listener when the component is created.

The you instantiate the component hundreds of time.

The error is in your application.

Only leaving this open to update the doc.

@Namek

This comment has been minimized.

Copy link

Namek commented Sep 23, 2016

@vicb document.addEventListener is the way to go for browsers. What about NativeScript?

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Sep 23, 2016

NativeScript

not sure what you mean here ??

The point is that you should add the global event listener only once.

@Namek

This comment has been minimized.

Copy link

Namek commented Sep 23, 2016

not sure what you mean here ??

document as a variable is not available on NativeScript, right? But nevermind that part for now.

I don't see a reason why this would not be a bug. Because why is this syntax (document: click) even possible? What is this for?

You want to add a global event listener when the component is created.

I believe host property is exactly for this purpose, isn't it?

The you instantiate the component hundreds of time.

Sounds like a workaround.

The error is in your application.

Please someone prove that I'm doing wrong here.

@angular angular locked and limited conversation to collaborators Sep 23, 2016

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Sep 23, 2016

Your question sounds like a support request.

Please use the issue tracker only for bugs and feature requests.

Use gitter and StackOverflow for support request.

@tbosch

This comment has been minimized.

Copy link
Member

tbosch commented Oct 12, 2016

We should not take 1-2 seconds for only a couple hundred empty event listeners. I think this is a valid bug....

@tbosch

This comment has been minimized.

Copy link
Member

tbosch commented Oct 12, 2016

Might be related to how we setup the zones for event listeners, see #12017

@tbosch

This comment has been minimized.

Copy link
Member

tbosch commented Nov 3, 2016

#12692 should improve this situation. Could you try again once this is in?

@tbosch

This comment has been minimized.

Copy link
Member

tbosch commented Nov 11, 2016

This is in now, please try again...

@tbosch

This comment has been minimized.

Copy link
Member

tbosch commented Nov 12, 2016

Also 73593d4 should help this a bit...

@angular angular unlocked this conversation Nov 12, 2016

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Nov 12, 2016

73593d4 will only help on instantiation and not on each click.

While there are certainly things we can do to improve the situation, could you please describe your use case in more details ?
I fail to understand why you would want a click anywhere in doc to trigger the handler for all the cells ?

What we could do is to make sure the change detection is called only once after the 30_30 onClickOff have been executed instead of 30_30 times after each single onClickOff.
But again, I still fail to understand why you would need to call 30*30 methods each time a click occur ?

@Namek

This comment has been minimized.

Copy link

Namek commented Dec 14, 2016

Hello again, problem persists both on instantiation and each click everywhere in the application.

While I do have workaround, to me it seems there is a problem because every other component instance registering on document events (using host property) will cause performance issue in whole application, no matter if the click is inside the component or anywhere else.

If you feel this can't be fixed then simply close this one. I just report here that setting host prop to event on document is useless because of major problem with firing change detection on whole application, no matter if there's changeDetection: ChangeDetectionStrategy.OnPush in the component or no.

Use case

Pricing table.

A component which displays price and after being clicked it shows input and tick+cross buttons. It emits event after value is edited and accepted.

image

Workaround

I simply register on document for events. It's probably not cool for native ionic apps or whatever mobile, which discourage using anything like nativeElement or native addEventListener calls.

setEventListeners(shouldBeRegistered: Boolean) {
  if (shouldBeRegistered && !this.documentClickEventHandler) {
    this.documentClickEventHandler = evt => {
      if (this.lastHostClickEvent !== evt && this.show) {
        this.cancelEditable()
      }
    }
    document.addEventListener('click', this.documentClickEventHandler)
  }
  else if (!shouldBeRegistered && !!this.documentClickEventHandler) {
    document.removeEventListener('click', this.documentClickEventHandler)
    this.documentClickEventHandler = null
  }

  if (shouldBeRegistered && !this.documentKeyupEventHandler) {
    this.documentKeyupEventHandler = evt => {
      if (evt.which === 27/*ESCAPE*/) {
        this.cancelEditable()
      }
      else if (evt.which === 13/*ENTER*/) {
        this.callSave()
      }
    }
    document.addEventListener('keyup', this.documentKeyupEventHandler)
  }
  else if (!shouldBeRegistered && !!this.documentKeyupEventHandler) {
    document.removeEventListener('keyup', this.documentKeyupEventHandler)
    this.documentKeyupEventHandler = null
  }
}

edcarroll added a commit to edcarroll/ng2-semantic-ui that referenced this issue Aug 19, 2017

mcosta74 added a commit to edcarroll/ng2-semantic-ui that referenced this issue Aug 20, 2017

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018

@Angelinsky7

This comment has been minimized.

Copy link

Angelinsky7 commented Aug 13, 2018

i've got the exact same issue with @HostListener('window:pointermove', ['$event']) !!!
if i only have 10 (or so) component using it everything is fine but when more components are added to he DOM all the application hang and the performance becomes very bad !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment