activeGroup object sort is incorrect

[Kienz]

The zIndex of activeGroup objects is wrong.

objects before activeGroup selection:

objects in activeGroup selection:

[kangax]

Fixed in 26c2590

[Kienz]

If you add objects with shift key to activeGroup the bug still exists.
I think the following two code sections are the problem:

1. Creating new activeGroup with mouseClick and shiftKey:
   var group = new fabric.Group([ this._activeObject, target ]);
   https://github.com/kangax/fabric.js/blob/master/src/canvas.class.js#L448
2. Adding object to existing activeGroup with mouseClick and shiftKey:
   https://github.com/kangax/fabric.js/blob/master/src/canvas.class.js#L436
   https://github.com/kangax/fabric.js/blob/master/src/group.class.js#L125

In both cases the zIndex of the objects are not considered. I think if the objects are intersect we have to compare the zIndex and inserting them by that order.

[kangax]

1. should be fixed now (4d4a129)

[kangax]

2. is a bit more complicated. We should first add optional 2nd argument to fabric.Group#addWithUpdate specifying index where to insert an object. Then use that index when injecting object in a group.

[kangax]

@Kienz is #2 still an issue?

[Kienz]

@kangax 2nd issue still exists.
See jsfiddle: http://jsfiddle.net/Kienz/EGYFX/

Select objects (with shift key) in the following direction:

1. blue
2. brown
3. red
   Now you can see that red rectangle is above brown rectangle.

[lukejagodzinski]

Issue still exists

The problem is in rendering order not placing group into canvas._objects.
Here are fixes to code that will make it work properly:

fabric.Canvas.prototype.renderAll = function(allOnTop) {
  var canvasToDrawOn = this[(allOnTop === true && this.interactive) ? 'contextTop' : 'contextContainer'],
    activeGroup = this.getActiveGroup();

  if (this.contextTop && this.selection && !this._groupSelector) {
    this.clearContext(this.contextTop);
  }

  if (!allOnTop) {
    this.clearContext(canvasToDrawOn);
  }

  this.fire('before:render');

  if (this.clipTo) {
    fabric.util.clipContext(this, canvasToDrawOn);
  }

  this._renderBackground(canvasToDrawOn);
  this._renderObjects(canvasToDrawOn, activeGroup);
  // this._renderActiveGroup(canvasToDrawOn, activeGroup);

  if (this.clipTo) {
    canvasToDrawOn.restore();
  }

  this._renderOverlay(canvasToDrawOn);

  if (this.controlsAboveOverlay && this.interactive) {
    this.drawControls(canvasToDrawOn);
  }

  this.fire('after:render');

  return this;
};

fabric.Canvas.prototype._renderObjects = function(ctx, activeGroup) {
  var i, length;

  if (!activeGroup || this.preserveObjectStacking) {
    for (i = 0, length = this._objects.length; i < length; ++i) {
      this._draw(ctx, this._objects[i]);
    }
  } else {
    for (i = 0, length = this._objects.length; i < length; ++i) {
      if (this._objects[i] && !activeGroup.contains(this._objects[i])) {
        this._draw(ctx, this._objects[i]);
      } else { // ADD THIS...
        this._renderActiveGroup(ctx, activeGroup);
      }
    }
  }
};

I've checked it and it works. Here is working example: http://jsbin.com/dejiwu/2/edit

I can create pull request

[asturur]

The screen you capture is not really a fix, we do not want the controls to be under any object. Controls are always over.

[lukejagodzinski]

So, there is one more error because right now it looks like this

I'm working on the fix

[lukejagodzinski]

Ok I've made another pull request #1963. Now code got much simpler.

http://jsbin.com/dejiwu/3/edit?js,output

I've noticed another bug but not related with rendering controls. The error occurs when hovering mouse on the control point of the object that is behind another object. Try here selecting green circle and clicking right bottom control point.

[asturur]

maybe o was wrong? i will check current situation

[asturur]

I think main problem here is this:

https://github.com/kangax/fabric.js/blob/master/src/static_canvas.class.js#L888

    /**
     * @private
     * @param {CanvasRenderingContext2D} ctx Context to render on
     * @param {fabric.Group} activeGroup
     */
    _renderActiveGroup: function(ctx, activeGroup) {

      // delegate rendering to group selection (if one exists)
      if (activeGroup) {

        //Store objects in group preserving order, then replace
        var sortedObjects = [];
        this.forEachObject(function (object) {
          if (activeGroup.contains(object)) {
            sortedObjects.push(object);
          }
        });
-       activeGroup._set('objects', sortedObjects);
+       activeGroup._set('_objects', sortedObjects);
        this._draw(ctx, activeGroup);
      }
    },

We are ordering the activeGroup objects but then we are putting them in object array , while rendering uses just the _objects.
@kangax @Kienz, is this possible?