Fix object.to data url

[asturur]

If toDataUrl is method of statiCanvas is better to render everything on the lower canvas and remove checks for upperCanvas and contextTop.

The real bug is somewhere else:

    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);
      }

we were calling renderAll with allOnTop = true on staticCanvas.
So canvasToDrawOn was contextContainer.
But later we were cleaning the context just on allOnTop = false.
So both copies of object, scaled and unscaled were on the canvas.
The unscaled one coming from canvas.add() in the object.toDataURL.

really this allOnTop i think is something from the past, now we are not rendering anything on contextTop other than controls sometimes or selectors. i see no reasons to draw all on top.
But not time to check it now.

closes #2474

[asturur]

I add, after this change nothing in the library is calling renderAll(true)

[kangax]

I add, after this change nothing in the library is calling renderAll(true)

Perhaps it's time to remove it then?

[asturur]

just waiting for you to ask.

[kangax]

:) let's do it! the less old, unnecessary cruft the better

On Sat, Sep 19, 2015 at 6:20 PM, Andrea Bogazzi [email protected]
wrote:

just waiting for you to ask.

—
Reply to this email directly or view it on GitHub
#2487 (comment).

[asturur]

@kangax ready to merge

[kangax]

Gotta update changelog with back-incompat note

[taveras]

It looks like this change has affected rendering of the fabric.Canvas.toDataURL method. The data URL returned by the method is always an empty image.

This line short-circuits the canvas element used to render to the upper canvas, if the upper canvas exists.

It does exist, but it is empty, so is renders the empty canvas as the fabric.Canvas's data URL.

Previously, fabric.Canvas.renderAll was called with the allOnTop flag set to true. This forced all of the content to be rendered onto the upper canvas, before the native toDataURL method was called.

[asturur]

@taveras thank you for noticing. i resubmitted the fix.

[taveras]

@asturur Awesome! I also had another strange observation:

When calling fabric.Canvas.toDataURL, it appears to always output an image that is two-times the size of the canvas, but cropped to the dimensions of the canvas.

If I call fabric.Canvas.setZoom(0.5) just before I call fabric.Canvas.toDataURL, it renders the canvas as expected.

I realize that this may require a new issue be created to be understood and resolved, and I'll do so if you feel it necessary.

[asturur]

What do you mean, if it is cropped, how do you know how large was the image before?
Can you show the bug somehow?

[taveras]

I only meant cropped in the sense that it was clipped to the normal canvas size.
Funnily enough, though, I believe the first issue may have even been with my own local build.
When I grab this fabric.min.js, the original issue I noted about a transparent Data URL image is gone.

It is this commit that seems to make the image in the outputted Data URL be the canvas scaled 2x, but clipped to the original dimensions.

Here is an example of the effect of your most recent commit:

Before
After

again, the original reasoning for this change, was that I was getting a transparent image when using fabric.Canvas.toDataURL, but it turns out that my local build of fabric.js was different than the build in the 'dist' folder on GitHub.