fix relative font-sizes #748 #749
Conversation
src/canvg.js
Outdated
| } else { | ||
| var parentsFontSize; | ||
| if (this.parent) { | ||
| var parent = this.firstParentWithAbsoluteSize(this.parent, 'font-size'); |
There was a problem hiding this comment.
the absolute size is stored on svg.emSize for now ... look at how getEM works. all we have to do is tell the toPixels method whether we care about fontSize for %'s or not (can just be a bool). if we do, it should use the viewport, otherwise it should use the same logic as getEM. so i'd add a bool prop to toPixels and then set it to true in this general area but leave it false/undefined in other areas and that should be it.
There was a problem hiding this comment.
Thank you @gabelerner , I updated the PR according to your feedback. It's, indeed, much cleaner that way. Please let me know if you have more thoughts on it.
| @@ -419,11 +419,22 @@ function build(opts) { | |||
| if (s.match(/cm$/)) return this.numValue() * this.getDPI(viewPort) / 2.54; | |||
| if (s.match(/mm$/)) return this.numValue() * this.getDPI(viewPort) / 25.4; | |||
| if (s.match(/in$/)) return this.numValue() * this.getDPI(viewPort); | |||
| if (s.match(/%$/)) return this.numValue() * svg.ViewPort.ComputeSize(viewPort); | |||
| if (s.match(/%$/)) { | |||
There was a problem hiding this comment.
this part looks fine, you can always do return this.numValue() * (considerParentFontSize ? this.getEM(viewPort) : svg.ViewPort.ComputeSize(viewPort); for brevity. optional for this PR
There was a problem hiding this comment.
I wrote the more verbose version to make it clearer what happens. But feel free to adapt it if you think it's clear anyway.
| @@ -1104,24 +1115,19 @@ function build(opts) { | |||
| if (typeof ctx.font != 'undefined') { | |||
| if (this.style('font').hasValue()) { | |||
| ctx.font = this.style('font').value; | |||
| // store the font-size in case we have to update the current font-size | |||
There was a problem hiding this comment.
was this removed on purpose or just a bad merge?
There was a problem hiding this comment.
Sorry, I assume this was a bad merge. I didn't want to remove it.
|
thanks again - we're currently actively trying to merge the typescript PR 751 and then i'll either merge this into that or if that takes too long i'll push this to master and create a release. will know more in about a weeks time. |
|
i merged your fix into our 3.0 (now in master) #760 will release that to npm fairly soon |
Attempt to fix #748