swimlane / ngx-datatable

✨ A feature-rich yet lightweight data-table crafted for Angular
http://swimlane.github.io/ngx-datatable/
MIT License
4.63k stars 1.68k forks source link

Row Hover Event causing template bindings to be constantly recalculated #989

Open mrjamesriley opened 7 years ago

mrjamesriley commented 7 years ago

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

[x] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, post on Stackoverflow or Gitter

Current behavior

As you hover over the rows of a rendered table - it seems a form of change detection is kicking in, causing all template functions to be called multiple times a second. This is causing a performance problem by causing a lot more work than necessary to occur on the front end of our app.

Expected behavior

I'd expect that hovering over the table rows would not trigger all template bindings to be updated, as no data would have changed.

Reproduction of the problem

Let's say you have a simple ngx column:

    <ngx-datatable-column name="Company" width="300">
      <ng-template let-value="value" ngx-datatable-cell-template>
       {{ uppercase(value) }}
      </ng-template>
    </ngx-datatable-column>

With the corresponding component method for uppercase being:

  uppercase(word) {
    console.log("processing the uppercase...")
    return word.toUpperCase()
  }

On the rendered table, as you hover over the rows, you'll see that the console log message is appearing multiple times per second - demonstrating multiple calls.

What is the motivation / use case for changing the behavior? The triggering of the function bindings, even if fast individually, being ran constantly without need is causing a performance issue, and the page to even feel janky.

Please tell us about your environment: Mac OSX, latest Chrome

Still an issue with the latest version (10.2.3). It appears the issue was brought in with 10.1.0 - with the Row Hover Event feature. I've tested with 10.0.5 to confirm that the issue is not present there. I feel that this represents a performance regression, and for those who have desire to track hover events - it shouldn't be causing a 'digest loop' style recalculation when simply moving the mouse over table rows.

andrii-oleksyshyn commented 7 years ago

I experience similar issue with scrolling. I want to load data at once, calculate all bindings and not to trigger recalculation on every scroll.

wizarrc commented 7 years ago

@mrjamesriley changelog shows v10.1.0 adds activation via mouseenter event in addition to mouse click and keyboard arrow keys that were already triggering the activate event. This is by design now. See activate event.

@andrii-oleksyshyn Are you using scrollbarV? If so, all bindings will need to be recalculated every scroll in order to virtualize the scroll view. As far as triggering the event on scroll, I did testing, and the mouseenter event is only processed when the mouse moves irrespective of the scrolling. I did see some browser bugs in relation to scroll inertia but the overhead it creates isn't significant in my testing.

wizarrc commented 7 years ago

@mrjamesriley I did suggest a perf optimization where the event would be listened to outside of the Angular zone and only when there are listeners, i.e. attaching to the activate event, would the event be triggered back inside the Angular zone resulting in change detection. That would be more of a pay for play model, even though in my testing I didn't see much a difference. All that would be moot if you already listen to the activate event anyways. Would that help your performance woes?

mrjamesriley commented 7 years ago

Thanks for the response @wizarrc. In our case, we have no need for the activate of a row, especially on hover event. It really feels excessive to recalculate all bindings every time the mouse moves over a table - with it triggering many times a second. It causes the experience to quickly get janky. Is there not a way to disable this 'listener' - to opt out of the activate event so that it does not cause all bindings to be recalculated as frequently as it is doing to?

amcdnl commented 7 years ago

Ya, there is some changes we could make in terms of event delegation rather than individual bindings.

wizarrc commented 7 years ago

@mrjamesriley How are you measuring? I'm a bit perplexed. In my testing, it will only trigger once when entering a new cell, and that's it. My concerns were related to scrolling, and after testing, it turns out that it doesn't trigger an event if you are scrolling and do not move the mouse. You keep referring to a hover event. I'm not aware of such an event. The event I am referring to is mouseenter event that might feel like a hover event. The event that I am referring to by its very nature, only triggers once per cell, think of it like a mouse click every time the mouse moves over a new cell.

That said, there are some ways that you could better handle that to make it even faster, but I'm not sure how you are noticing it. Also, I don't see how that's different than say a mouse click on a cell causing the same effect. There are so many other inefficient code that causes recalculation that this one seems small in comparison.

wizarrc commented 7 years ago

@amcdnl I proposed a solution to this perf issue here if you are interested.

mrjamesriley commented 6 years ago

@wizarrc @amcdnl

Hello, I'm still having problems here. The issue is as I describe in the initial post, but to hopefully clarify the issue. This is the problem I'm seeing:

  1. I have a table with 25 rows showing. A few of the columns are using fast-running functions to dynamically generate or modify the data that is displayed (e.g the uppercase() function in my original post).
  2. As I scroll down the page, or simply move my mouse across the table - the 'change detection' is kicking in multiple times a second. The mouseenter event may only trigger once per cell, but as the mouse passes over multiple rows - this is being triggered multiple times.
  3. This is causing all 25 rows to have their template functions re-ran multiple times a second, an unnecessary and undesired consequence.

Now in reality, I have multiple tables on a single page - and the impact of the hover triggering change detection is that every visible row, of every table on the page is having it's template functions re-ran. This is resulting in a page that suffers a performance hit every time the cursor passes over any row. Can you see how this is a problem?

Maybe I'm overlooking something here - but the desired result is that merely scrolling through a page (or moving the cursor around), should not have have the multiple tables constantly re-running their template functions (the unnecessary computation should be spared).

To clarify, by 'template functions', I'm referring to the uppercase() function in my example code in the original post. By 'change detection' I'm referring to this function (in this example) being re-ran for all rows in all tables, as the cursor enters each row. Is it not possible to disable the hover/mouseenter event from kicking off the change detection?

Appreciate your time

555ea commented 6 years ago

@wizarrc @amcdnl

Hello!

Are there any updates on removing unnecessary change detection calls on events like mouseenter, when there's no subscribers to them? This is a big performance hit, when using [virtualization]="false", which is slow for MS Edge and mobile devices.

If you have 100 items, there would be 100 rowClass calls, 100 filter and methods call on a single mouseenter, which may quickly get very slow.

555ea commented 6 years ago

@mrjamesriley There is a quick workaround for intercepting mouseenter events and not passing them down to rows, like

window.addEventListener('mouseenter', function (event) {
  event.stopPropagation();
}, true);

Other option is to use angular Pipes, they get cached and won't be called not even on a click event, if the data hasn't changed

atakchidi commented 6 years ago

I did a pr that will fix this issue for those who like me don't need mouseenter events in their app

karthilxg commented 4 years ago

Like @555ea mentioned intercepting mouseenter event from a parent component or window during capturing phase and stopping it avoids executing the methods like rowClass multiple times per each row.

But the workaround only solves part of the problem, any keydown events when a row is focused still executes the rowClass methods multiple times.

  @ViewChild('datatable', { static: false }) public datatable: ElementRef;

  public ngAfterViewInit() {
    this.dataTable.nativeElement.addEventListener('mouseenter', this.onMouseEnter.bind(this), true);
  }

  public ngOnDestroy() {
    this.dataTable.nativeElement.removeEventListener('mouseenter', this.onMouseEnter.bind(this));
  }

  private onMouseEnter(event: MouseEvent) {
    event.stopPropagation();
  }

Here's the StackBlitz for reproducing the issue.

CmpHDL commented 1 year ago

@surya-pabbineedi This is still a major problem.