Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rect.class.js - fix issue with incorrect <rect> tags in SVG #5582

Merged
merged 2 commits into from
Mar 24, 2019

Conversation

amitchell0560
Copy link
Contributor

@amitchell0560 amitchell0560 commented Mar 18, 2019

issue where svg containing <rect> tag missing height or width will have incorrect height or width (inherits from canvas height/width)

May also be fixable by changing the line where it extends from options, but this seems less dangerous.

image of issue.
image

malformed tag (note missing height)
<rect x="171.5" y="289.63" width="94.59" transform="translate(-140.68 239.31) rotate(-44.96)" fill="#F8485E" stroke="none"></rect>

issue where <rect> tag missing height or width will have incorrect height or width.
@asturur
Copy link
Member

asturur commented Mar 20, 2019

thanks for this fix.
Effectively getting width/height from the document itself is completely wrong.
Merging object options with that other generic options is completely wrong.

This fix does not have side effects and since i want to rewrite svg parsing i prefer this over a more dangerous fix now.

We need to add a visual test and a unit test for this. Can you do it?

Your SVG seems like a good use case and easy to extend in the svg_import.js test file.

@asturur
Copy link
Member

asturur commented Mar 24, 2019

@amitchell0560 can you paste here a copy of that SVG? i would like to add a test for it.

@asturur asturur merged commit b538562 into fabricjs:master Mar 24, 2019
@amitchell0560
Copy link
Contributor Author

amitchell0560 commented Mar 25, 2019

Apologies for the slow reply - had some priority issues come up.

The SVG I used isn't free use, however you can use the below that will exhibit the same issue, OR add any <rect> with a missing height/width to an SVG you use for tests now.

<svg viewBox="0 0 256 256" xmlns="http://www.w3.org/2000/svg"> <defs> <style>.cls-1{fill:none;stroke:#000;stroke-linecap:round;stroke-linejoin:round;stroke-width:2px;}</style> </defs> <title/> <g data-name="93-bin" id="_93-bin"> <rect class="cls-1" height="176" width="144" x="56" y="72"/> <rect class="cls-1" height="32" width="176" x="40" y="40"/> <polyline class="cls-1" points="96 40 96 8 160 8 160 40"/> <line class="cls-1" x1="104" x2="104" y1="112" y2="248"/> <rect class=cls-1" height="88" x="128" y="160" /> <rect class=cls-1" width="48" x=104" y=8" /> <line class="cls-1" x1="152" x2="152" y1="112" y2="248"/> </g> </svg>

@asturur
Copy link
Member

asturur commented Apr 27, 2019

Just for reference, today i added the test.
#5653
In the PR you can see how a svg import test is added.

@asturur asturur mentioned this pull request May 19, 2019
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants