preserveObjectStacking: true - causes error in creating group from ActiveSelection

[nateevans]

Version

2.3.4

Test Case

https://jsfiddle.net/nateevans/jayc67sk/ (click the "Add & Group" button)

Steps to reproduce

1. Create canvas with preserveObjectStacking: true
2. Add some objects to the canvas
3. Create a new ActiveSelection with those objects
4. Set that selection to active with canvas.setActiveObject
5. Call canvas.getActiveObject().toGroup() to create a group

Expected Behavior

A new group should be created with the selected objects and it should be actively selected.

Actual Behavior

Uncaught RangeError: Maximum call stack size exceeded occurs

It seems the uncaught error is result of recursive calls to _set here:

fabric.js/src/shapes/active_selection.class.js

Line 105 in 068c815

if (key === 'canvas') {

Possible fix may lay in this area??

fabric.js/src/canvas.class.js

Line 359 in c541a27

if (activeObjects.length > 0 && !this.preserveObjectStacking) {

The code in the fiddle works as expected with preserveObjectStacking: false, and the error does not occur when you do not pass the canvas object to the new ActiveSelection. But when you do not pass the canvas, the group is not created properly.

I may be using the API incorrectly, not sure. But seems if the code works with preserveObjectStacking: false, it is probably not intended to error when preserveObjectStacking: true

[asturur]

i ll look into it, looks a bug.

OFF TOPIC:
I have seen your reference to printsites.com, i feel like mentioning this library i released, since i did not find anything similar in the web:
https://www.npmjs.com/package/changedpi

it allows you to change the DPI of image generated by canvas, quickly in the browsers.
Generally ( but i may be wrong ) who needs to change dpi always fallback on some server side approach to use available tools, while this is quick and effective.

[asturur]

var rect = new fabric.Rect({
    width: 40,
    height: 40,
    originX: 'left',
    originY: 'top'
  });
  
  canvas.add(rect);
  
  var circle = new fabric.Ellipse({
    width: 40,
    height: 40,
    originX: 'left',
    originY: 'top',
    rx: 40,
    ry: 40
  });
  
  canvas.add(circle);

  var objs = canvas.getObjects();
  
  var selection = new fabric.ActiveSelection(objs, {canvas: canvas});
  
  canvas.setActiveObject(selection);
   canvas.getActiveObject().toGroup();

this is the minimal amount of code to reproduce the bug in kitchensink.

[asturur]

i do not see any difference with preserveObjectStacking false

[asturur]

Ok the usual JS mutability problem that took 2 hours of my life.
getObjects return a reference to the canvas.object and the canvas.remove in toGroup is causing the error.

this needs to change once for all.

[nateevans]

Thanks for the reference to https://www.npmjs.com/package/changedpi @asturur looks interesting!

I don't work with that company anymore, but might still have a use for it. 👍