search

yWorks / svg2pdf.js

A javascript-only SVG to PDF conversion utility that runs in the browser. Brought to you by yWorks - the diagramming experts
MIT License
655 stars 101 forks source link

SVG arcs don't work in Safari and Firefox #22

Closed TorsteinHonsi closed 7 years ago

TorsteinHonsi commented 7 years ago

After fixing #10, arcs display fine in Chrome, but Safari and Firefox simply leave them out. It can be seen with test no 21 in the repo:

skjermbilde 2017-03-21 kl 11 32 46

yGuy commented 7 years ago

True - the reason is that the new code only runs when there is no "pathSegList" JS property, which still exists for Firefox and Safari, but is gone with Chrome. We should probably just move the SvgPath && (d = SvgPath(d).unshort().unarc().abs().toString()); part higher up in front of the return statement in getPathSegList() since the calling code expects all of the svg path oddities to have already been removed when the call returns.

TorsteinHonsi commented 7 years ago

Thanks! I would like to try this and send a PR. In the mean time, can you tell me, or document in readme.md how the workflow is at the moment? The test files refer to svg2pdf.min.js, which is concatenated from source files and minified using Uglify and apparently there is a plugin to preserve the license headers. What commands do I run to build it?

yGuy commented 7 years ago

Actually building should be simple: there are scripts defined in package.json so you can do 

npm run build

and it should build the minified sources along with source maps so that you can conveniently run and debug the tests.

TorsteinHonsi commented 7 years ago

Ah, thanks!

TorsteinHonsi commented 7 years ago

Moving the path definition line up in front of the return statement didn't cut it. However, completely removing the return statement works for both Firefox and Safari. Is that a viable solution, or are you aware of any horrible side effects?

yGuy commented 7 years ago

You need of course set the "d" attribute anew after cleaning up the string. Just moving the line alone will not work. This however modifies the SVG and as such is not a nice solution, anyway.

I guess simply always using the Chrome code (removing the if) should work well. Otherwise we would have the same "horrible side-effects" in Chrome. I am not aware of any such side-effects, other than possibly a loss in performance.

TorsteinHonsi commented 7 years ago

Probably the cleanest solution would be to mimic the arc segment in the segment list, and to the transform to a curve here instead. But your proposed fix works, hence the PR. Would appreciate a pull and a version bump.

yGuy commented 7 years ago

Sorry, but as I said before, I don't like the way we are now modifying the document. I would rather just get rid of the FF/Safari branch and always go with the Chrome branch.