Angular-gridster2: Browser crash on reproducible setup on push

[kaihenry]

First, great work! thank you! I am currently experimenting with the grid for an app I am working on and I found what I think is a performance issue that crashes your browser (that's tested on Edge and Chrome). It can be reproduced using your demo-site as follows:

• set the grid with both min-grid-cols and max-grid-cols = 12
• create three rows with 1x1 widgets
• create a fourth widget in row four with full width (1 x 12)
• now try to change the height of this full width widget by pulling the "north"-handle up

This should crash the app. I added a screenshot of the setup. Hope this helps!

Regards,
Kai

[tiberiuzuld]

Hi @kaihenry ,
Yes and I think I know from where ... pushService
Will see if I can fix it.

Thanks

[klemenoslaj]

Hi @tiberiuzuld,

or maybe it is even due to angular change detection?
I mean, that is a lot of grid items, + columns + rows.

If that is indeed the case, then ngZone should be avoided for resizing and dragging(zone.runOutsideAngular).

[tiberiuzuld]

Hi @klemenoslaj ,

No is from Push Service the issue.
Did some debugging yesterday.
If you disable the Push to North or any other direction from options.
Then it will work perfectly.

I didn't found any fix solution yet.
If all 4 directions are enabled it enters in an infinite loop.

[klemenoslaj]

Ah ok :/

But either way, I still think it makes sense to consider triggering DOM events out of angular zone.
Should I make separate request for that? This can for sure be an issue with large applications.

[tiberiuzuld]

Don't think is necessary to run outside of angular.
And still need to update the preview element when dragging and resize.

[klemenoslaj]

What about applications with migration from AngularJS with ngUpgrade? They will be totally unable to use gridster2, even though the implementation is good for Angular, because $digest is triggered by ngZone then.

I will open new issue, where we can discuss that without polluting this issue.

[tiberiuzuld]

Hi @klemenoslaj ,
Fixed the browser crash.
Made a hard limit to stop the algorithm to go in a infinite loop when trying to find a viable push position.

Was not able to find a viable solution to fix the algorithm without going in a infinite loop.
With the example given by you the resize up will not be possible because of this limit.

I welcome any PR to fix the Push algorithm infinite loop in this example.

[tiberiuzuld]

Deployed the fix in 4.4.0

[NoMercy235]

Hello! Small update on this issue. I have made a change in GridsterPush service which seems to have gotten rid of the infinite loop.

In the private push method, at the following if statement:

if (this.pushedItemsTemp.indexOf(itemCollision) > -1) {
  makePush = false;
  break;
}

The indexOf is applied on object's references and I think it never checks correctly because of that since it will never find itemCollision in the temporary array and it will check the same items indefinitely.

Changing the if like this:

const compare = this.pushedItemsTemp.find((el: GridsterItemComponentInterface) => {
  return el.$item.x === itemCollision.$item.x && el.$item.y === itemCollision.$item.y;
});
if (compare) {
  makePush = false;
  break;
}

no longer causes the infinite loop to occur, but I'm not sure that this is the correct way to uniquely identify. Is there anything I'm missing here?

[tiberiuzuld]

Hmm possibly , try removing https://github.com/tiberiuzuld/angular-gridster2/blob/master/src/lib/gridsterPush.service.ts#L116 that limit of iterations and test it with the example in this ticket.

[NoMercy235]

It works when removing everything related to this.count. I placed a console.log statement in there and it's only called about 50 times, which I suppose it's normal behavior in that example.

[tiberiuzuld]

Yes it seems right.
Make a PR will check it out later today.

[tiberiuzuld]

So tested multiple use cases and works.

Released the fix in v4.7.1

Thanks @NoMercy235