Second selection box rendered on canvas

[antucg]

Version

2.7.0

Information about environment

Chrome/Firefox

Steps to reproduce

Add text (Textbox) to canvas and select part of it.

Expected Behavior

Only selected text is "highlighted".

Actual Behavior

A selection "box" is rendered over current selection.

I have just upgraded fabric to 2.7.0. Previous version I had was 2.3.6. The issue only started happening with latest version. Any idea of why this might be happening?

[asturur]

can we replicate with a fiddle please? i never noticed this.

[antucg]

I guess it must be happening because of some configuration I have. I was just asking in case someone had an idea. The code is pretty standard, create canvas and add text to canvas. The thing is that the previous version I was using (2.3.6) doesn't have the problem.
Thanks.

[asturur]

please try to replicate

…

On Sun, 12 May 2019, 22:00 antucg, ***@***.***> wrote: I guess it must be happening because of some configuration I have. I was just asking in case someone had an idea. The code is pretty standard, create canvas and add text to canvas. The thing is that the previous version I was using (2.3.6) doesn't have the problem. Thanks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5682 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJDQQDQ36GUQFMYLTKSGT3PVBZNDANCNFSM4HMEGOFQ> .

[antucg]

I couldn't replicate it. I was expecting this to be honest. I guess it must be some configuration issue. I spotted where it happened. It was after calling canvas.toDataURL(). If I don't call the method then the issue doesn't happen.
The idea behind this post was to see if anybody would think of something that would guide me through the issue.
I couldn't solve it. Instead what I did is clone the canvas and call toDataURL in the cloned instance.

[asturur]

well if calling toDataURL causes it, it has to cause it in a fiddle too.
Or you have some override around that changes fabric inner functionalities?

[asturur]

Let me know if you can reproduce it or not

[antucg]

I have some overrides for different parts of the framework. I tried disabling them all, but it still happened. I don't know what is the problem to be honest. I did the clone and worked on it, so I didn't continue with the investigation.

Thanks anyway.

[asturur]

ok!

Remember that me as a developer i m always trying to understand needs for changing the lib, and where official support for hooks or injectable code makes sense.

So if you want to share why you had to modify the lib, you are welcome.

[antucg]

Hello, I am back with the same issue, my "clone" solution is no longer valid because of different issues. I was debugging what's happening when toDataURL is called. I have narrowed it down to this:

function toCanvasElement(multiplier, cropping) {
      multiplier = multiplier || 1;
      cropping = cropping || {};
      var scaledWidth = (cropping.width || this.width) * multiplier,
        scaledHeight = (cropping.height || this.height) * multiplier,
        zoom = this.getZoom(),
        originalWidth = this.width,
        originalHeight = this.height,
        newZoom = zoom * multiplier,
        vp = this.viewportTransform,
        translateX = (vp[4] - (cropping.left || 0)) * multiplier,
        translateY = (vp[5] - (cropping.top || 0)) * multiplier,
        originalInteractive = this.interactive,
        originalContext = this.contextContainer,
        newVp = [newZoom, 0, 0, newZoom, translateX, translateY],
        canvasEl = fabric.util.createCanvasElement();
      canvasEl.width = scaledWidth;
      canvasEl.height = scaledHeight;
      // this.interactive = false;
      this.viewportTransform = newVp;
      // this.width = scaledWidth;
      // this.height = scaledHeight;
      this.calcViewportBoundaries();
      // this.contextContainer = canvasEl.getContext('2d');
      // // will be renderAllExport();
      this.renderAll();
      this.viewportTransform = vp;
      // this.width = originalWidth;
      // this.height = originalHeight;
      this.calcViewportBoundaries();
      // this.contextContainer = originalContext;
      // this.interactive = originalInteractive;
      // return canvasEl;
    }

This is the code from: toCanvasElement of StaticCanvas class. I have commented out a lot of the things. So now the only lines that are doing something are:

this.viewportTransform = newVp;
this.calcViewportBoundaries();
this.renderAll();
this.viewportTransform = vp;
this.calcViewportBoundaries();

Just by doing this it happens. As I mentioned before, it is something related with my code. We modify some things to adjust them to our needs. For example I have disabled object caching for some types of objects. I can't put a fiddle with the code to reproduce it, because I don't know what it is causing it, otherwise I wouldn't need to post here!!

Does anyone have a remote idea of what could be causing this problem?

Thanks.

[UPDATE]

I have tried all version until I found which one my issue started to happen, it is 2.5.0. Based on the changelogs, I think this change is causing it: #5452.

Any idea how that might impact my app?

[asturur]

well i may imagine that renderAll detect a dirty upperCanvas. and redraw it.
But since you applied some zoom or cropping ( i assume ) is going to clean a part of it only.

Probably we need a fix for this strange use case, like we need to mark the canvas as interactive = false during the process export. We need renderAll to skip touching the upper canvas.

[asturur]

Still you cannot reproduce? i belive you should, this could be a legit bug.

[antucg]

I am literally just adding a textbox object into canvas, and then calling canvas.toDataURL(). I tried a jsfiddle and it was ok, which it is expected. As I mentioned before, I have some modifications like this:

fabric.Line.prototype.noScaleCache = false;
fabric.Rect.prototype.noScaleCache = false;
fabric.Ellipse.prototype.noScaleCache = false;
fabric.Triangle.prototype.noScaleCache = false;
fabric.Textbox.prototype.noScaleCache = false;
fabric.IText.prototype.noScaleCache = false;
fabric.Line.prototype.objectCaching = false;
fabric.Rect.prototype.objectCaching = false;
fabric.Ellipse.prototype.objectCaching = false;
fabric.Triangle.prototype.objectCaching = false;
fabric.Textbox.prototype.objectCaching = false;
fabric.Group.prototype.objectCaching = false;
fabric.IText.prototype.objectCaching = false;

or this:

fabric.Object.prototype.perPixelTargetFind = true;
fabric.IText.prototype.perPixelTargetFind = false;
fabric.Group.prototype.perPixelTargetFind = false;
fabric.Textbox.prototype.perPixelTargetFind = false;

or

fabric.Canvas.prototype.selectionColor = 'rgba(9, 168, 129, 0.3)';
fabric.Canvas.prototype.selectionBorderColor = 'rgb(9, 168, 129)';
fabric.Canvas.prototype.selectionLineWidth = 1;
fabric.Canvas.prototype.controlsAboveOverlay = true;

fabric.Canvas.prototype.enableRetinaScaling = false;
// Prevent selected layer from being rendered on top
fabric.Canvas.prototype.preserveObjectStacking = true;
fabric.Group.prototype.lockUniScaling = true;

I tried commenting it all out, but it still happens. I am out of ideas, any suggestion?

Thanks.

[antucg]

Another step:

I am using clipTo:

canvas.clipTo = function(ctx) {
  ctx.rect(x, y, width, height);
};

I have discovered that if I remove these lines, then it doesn't happen. However I can't still reproduce it in a jsfiddle.
I know that clipTo is deprecated, but I never found what is the alternative. Any suggestion?

Thanks.

[asturur]

clipTo is going to be removed in fabric 4
the alternative to clipTo is an absolute positioned clipPath but the performances are probably lower.

[antucg]

I continued my research and this is the issue, I am using fabric 3.0.0.

In toCanvasElement method this is more or less the relevant code:

newVp = [newZoom, 0, 0, newZoom, translateX, translateY],
canvasEl = fabric.util.createCanvasElement();
this.viewportTransform = newVp;
this.contextContainer = canvasEl.getContext('2d');
this.renderAll();
this.viewportTransform = vp;
this.contextContainer = originalContext;

So basically, a new canvas is created. The 2D context of that new canvas, is assigned to current canvas. RenderAll is called, so new context will have the same content as current context. Once this is done, original context is reset.

My issue is with the newVp, the second cursor and selection coordinates are the ones from this secondary context. vp and newVp aren't the same. I am only need the dataUrl of a portion of the canvas. When original context is reset, cursor and selection from secondary context are also rendered, why? I have no clue. If anyone thinks of something, please let me know. I tried to reproduce it in a jsfiddle, but again, I haven't been successful.

In the meantime I have implemented an alternative implementation to that method:

canvasEl = fabric.util.createCanvasElement();
canvasEl.width = scaledWidth;
canvasEl.height = scaledHeight;
var context = canvasEl.getContext('2d');
context.drawImage(
        this.getElement(),
        cropping.left,
        cropping.top,
        scaledWidth,
        scaledHeight
      );

It works, it seems fast, and the secondary cursor and selection box aren't rendered on my main canvas. @asturur what do you think, is it viable solution?

Thanks.

[asturur]

no because this is not gonna scale the image up.
If you are multiplying by 10X this is gonna be fuzzy

[asturur]

http://jsfiddle.net/kuh1e5ja/

This is a fiddle that reproduce your bug that i wrote reading your description.

I do not understand how you could not reproduce it.

So there is a bug, and the solution is stopping the upperCanvas ( contextTop ) to be modified during toDataURL.

[antucg]

Hi, in my test I was not selecting the text programmatically. In my app it happens when I select the text with the mouse for example.

At the moment this bug is an issue for me. It is preventing me from releasing to production. Could you tell me where it should be fixed, more or less so I can work on it?

[asturur]

http://jsfiddle.net/mg6wy7vn/

This is with manual selection, enter editing and select the text with double click, you will get the bug randomly.

[antucg]

Actually that was the bit I was missing. I am doing that in my app, on mouse up I call toDataURL. I have been working on it for the last 72 hours, I tried so many things that I was out of ideas. A fresh view definitely was going to be helpful.

Can I do something to help with this bug?
Thanks.

[asturur]

the bug is in the choice of reusing the same canvas for execute the toCanvasElement/toDataUrl, but has many advantages, we can avoid somehow to make dirty the contextTop.

Probably i can dereference it and re reference it again after export.

[asturur]

i think i have one line change that avoid the problem, trying.

[asturur]

#5736

You can try this change, i m waiting to see if it passes the tests, since there are tests for dataUrl that have to pass.

[antucg]

I have applied the change, however, it is still happening.

[asturur]

i have to figure out something for the upperCanvas.
When the text in editing mode rerender, it triggers a render of the selection box.
But i would like to avoid to add in the codebase checks to verify the upperCanvas ( and the contextTop) exists or not. I ll get back to this tomorrow or asap.

[antucg]

Cool, thanks for that. Let me know if there is anything I can do.

[asturur]

http://jsfiddle.net/0Lywxzf7/

this 2 extra lines in the fiddle seems to work around the problem somehow, the fix will be based on something like that in the code. After i verified nothing else will assume a contextTop exists.

[antucg]

I have tried that, in my case I got an exception. I guess it is a "race" condition. Basically there are functions that need the contextTop, and given that for a short period of time it is null, I am getting an exception. I guess is something that might happen quite easily. The workaround is this:

var origin = canvas.fabric.contextTop;
var canvasEl = fabric.util.createCanvasElement();
canvas.fabric.contextTop = canvasEl.getContext('2d');
var dataURL = canvas.fabric.toDataURL({
        format: 'jpeg',
        quality: 0.75,
        width: currentCategory.width,
        height: currentCategory.height,
        top: padding,
        left: padding,
      });
canvas.fabric.contextTop = origin;

So I don't assign contexTop to null, instead I assign an empty context that will be discarded.

I just wanted to let you know, but probably is something you are already aware of it.

Thanks

[asturur]

Merging the related PR that seems an improvement to me anyway.
Please let me know if with the latest master built ( so download master as soon as the pr is merged, and build it ) the problem is still there.

[antucg]

Hi @asturur, I have just tested it, it seems to be ok.

Just to confirm, I run the build command from package.json. If it is correct, then the change seems to fix the problem.

Thanks for that, you are awesome.

[antucg]

Hi @asturur,

When are you planning on creating a new release with this fix?

Thanks.