search

fabricjs / fabric.js

Javascript Canvas Library, SVG-to-Canvas (& canvas-to-SVG) Parser
http://fabricjs.com
Other
29.2k stars 3.52k forks source link

BUG - Text's path casts shadows #7667

Open EdnaldoNeimeg opened 2 years ago

EdnaldoNeimeg commented 2 years ago

It'd be realy cool if text's path could be shown on contextTop. Eventhough the path is not always a part of the text, it cast text's shadows, which is weird when the path is used as just a "follow" line.

A fiddle just for reference: https://jsfiddle.net/Neimeg/315jt6rg/96/

Example below: image

ShaMan123 commented 2 years ago

add a simple repro to make your issue clear and easy to help

asturur commented 2 years ago

As of now i don't even remember how we trigger the textPath rendering. If we do, definitely shadow should be removed for that. Can you open a fiddle with the code you use for that rendering?

EdnaldoNeimeg commented 2 years ago

As of now i don't even remember how we trigger the textPath rendering. If we do, definitely shadow should be removed for that. Can you open a fiddle with the code you use for that rendering?

Done. Link on first comment.

ShaMan123 commented 2 years ago

I see in the repro that object dimensions are wrong as well and don't include text chars

EdnaldoNeimeg commented 2 years ago

I see in the repro that object dimensions are wrong as well and don't include text chars

What does it have do to with the "path shadow" issue or the title of the issue?

asturur commented 2 years ago

So ok, now i remember, you put path invisible or visible to have it render. So yes, is a bug. The path shouldn't inherit the shadow from text, if someone wants to had the same shadow will add it to both of the objects.

asturur commented 2 years ago

Showing it on canvasTop is not on the table for now, i m not sure what would solve. We want to move controls on the topCanvas, but the text path is not a decoration of the object in that sense.

ShaMan123 commented 2 years ago

I see in the repro that object dimensions are wrong as well and don't include text chars

What does it have do to with the "path shadow" issue or the title of the issue?

Absolutely nothing But it's a bug non the less

melchiar commented 2 years ago

If you're drawing the path to use it a guide, a better way is to render the path in the after:render callback. This fixes the shadow issue, while also making sure that the guide renders above any other objects in the canvas (as guides should)

https://jsfiddle.net/melchiar/pLy2au0s/

EdnaldoNeimeg commented 2 years ago

If you're drawing the path to use it a guide, a better way is to render the path in the after:render callback. This fixes the shadow issue, while also making sure that the guide renders above any other objects in the canvas (as guides should)

https://jsfiddle.net/melchiar/pLy2au0s/

Well, it is still a bug, although your sulution solves the problem for now.

EdnaldoNeimeg commented 2 years ago

If you're drawing the path to use it a guide, a better way is to render the path in the after:render callback. This fixes the shadow issue, while also making sure that the guide renders above any other objects in the canvas (as guides should)

https://jsfiddle.net/melchiar/pLy2au0s/

Looks VERY promissing, but there's a problem when zooming in/out image

asturur commented 2 years ago

yes because you should set the zoom back on after render.

The fix should be easy, an additional wrap in ctx.save() and ctx.restore() before drawing the path, and inside the ctx.save() block we set the shadow as the path shadow ( so that you can have it or not have it ). That should make the trick

melchiar commented 2 years ago

@EdnaldoNeimeg I've updated the fiddle to correct for zoom. But yes, proper control for separate shadows between text and path should be added.

https://jsfiddle.net/melchiar/pLy2au0s/

upzhujie commented 2 years ago

Showing it on canvasTop is not on the table for now, i m not sure what would solve. We want to move controls on the topCanvas, but the text path is not a decoration of the object in that sense.

hey, is there any plan for this case 'We want to move controls on the topCanvas'?

asturur commented 2 years ago

There was an old PR open for it, but it never got finished/merged. The top canvas seems the natural destination for controls, since you don't want to re-render all the canvas if you have an hover effect on controls, or if your controls do not cause any canvas change ( imagine a control to copy the text content of a textbox or simply selecting an item shouldn't require to redraw all the canvas )

Is not hard at all, is just really about moving the drawing calls for controls on the contextTop, it could be as simple as that. The option controlsAboveOverlay would go away and the ability to put an overlay on top of controls would go away.