Closed Bug 1396642 Opened 2 years ago Closed Last year

SVG wrong size with viewBox width/height non-integers

Categories

(Core :: SVG, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: simon.lydell, Assigned: longsonr)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file test.html
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170825011442

Steps to reproduce:

Added the following code to an HTML5 document:

<svg viewBox="0 0 2 3.5" style="width: 200px; height: auto; background: black;"></svg>


Actual results:

The size of the SVG element was 200×400 px.


Expected results:

The size of the SVG element should be 200×350 px, like in Chrome, Safari and Edge.



Note that the viewBox height is a non-integer: viewBox="0 0 2 3.5"
If I use only integers, Firefox sizes the SVG correctly.

As soon as the viewBox width and/or height is a non-integer, and the (CSS) width and/or height of the SVG element is auto, Firefox sizes the SVG element incorrectly. Chrome, Safari and Edge all render the SVG with the expected size.

See the attached test.html for tests of different combinations of viewBox widths and heights, as well as CSS widths and heights.
Component: Untriaged → SVG
Product: Firefox → Core
Priority: -- → P3
Duplicate of this bug: 1454818
We're rounding the viewBox width and height to ints here:

https://dxr.mozilla.org/mozilla-central/rev/2a59b432d2bd9b15ceec6b9435f60c785a820ef2/layout/svg/nsSVGOuterSVGFrame.cpp#282

because nsFrame::ComputeSizeWithIntrinsicDimensions only supports an nsSize aIntrinsicRatio (with int width and height).  So to fix I guess we'd need ComputeSizeWithIntrinsicDimensions to take floats for the ratio, which would mean LogicalSize would need to be taught to handle floats instead of ints as well.  I'd welcome any thoughts on whether or not it'd be worth it, or if anyone sees alternative solutions.

(At first blush at least, the simplest solution to me seems to be to just make a new (somewhat pared down) version of LogicalSize that takes floats instead of an nsSize, and then make a new (overload of?) ComputeSizeWithIntrinsicDimensions that takes two float arguments instead of the current aIntrinsicRatio (unless there's a float version of nsSize around).  LogicalSize is used in a lot of other places, so templating doesn't seem like a great option.)
Attached patch 1396642.txt (obsolete) — Splinter Review
Well spotted Tom.

nsSize being an nscoord suggests its supposed to contain values in app units, doesn't it Jonathan?

That seems to be how canvas works: https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLCanvasFrame.cpp?q=%2Bfunction%3A%22%28static+in+layout%2Fgeneric%2FnsHTMLCanvasFrame.cpp%29%3A%3AIntrinsicRatioFromCanvasSize%28const+nsIntSize+%26%29%22&redirect_type=single#60
Assignee: nobody → longsonr
Flags: needinfo?(jwatt)
Attachment #9008951 - Flags: review?(dholbert)
Comment on attachment 9008951 [details] [diff] [review]
1396642.txt

This looks good, though it needs an automated test of some sort.
Flags: needinfo?(longsonr)
Attachment #9008951 - Flags: review?(dholbert) → feedback+
Attached patch with mochitestSplinter Review
Attachment #9008951 - Attachment is obsolete: true
Flags: needinfo?(longsonr)
Attachment #9009257 - Flags: review?(dholbert)
The nsSVGOuterSVGFrame::GetIntrinsicRatio() change here doesn't look right to me: nsPresContext::CSSPixelsToAppUnits takes an int32_t, so when we pass viewboxWidth (a float) to it, the first thing it's going to do is round the float down to an int (if I remember that conversion correctly).  So a viewboxWidth of 2.5 will round to 2, which is different than the current round to 3, but still seems like in general will be wrong.  (Yes, it will get multiplied by 60, but 2.5 * 60 is a ways off from 2 * 60.)  Am I missing something?

We could do viewboxWidth * AppUnitsPerCSSPixel() as a float, and then that will in general get rounded when it gets put into an nsSize, and maybe that will be good enough to fix the original poster's testcases, but there will still be viewboxes with (somewhat) reasonable dimensions that will fail with that fix.  For example, if the viewbox is '0 0 .025 .035', then the nsSize we'd be returning from GetIntrinsicRatio with this patch would be (round(.025 * 60), round(.035 * 60)) = (2, 2), which is quite a bit off from the correct ratio of ~.714.  Maybe those sizes are too small to worry about?
Yes you're missing something. I'm calling this overload https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.h?q=%2Bfunction%3AnsPresContext%3A%3ACSSPixelsToAppUnits%28float%29&redirect_type=single#667, not the int32_t one.

This just makes things somewhat better since most viewBoxes I've seen are not that small. A more invasive fix would be to change from nsSize to something else that holds a float or double pair (the base class for nsSize is a template). I'm not sure whether that would fly or not.
Thanks :)
Comment on attachment 9009257 [details] [diff] [review]
with mochitest

Review of attachment 9009257 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me
Attachment #9009257 - Flags: review?(dholbert) → review+
This still seems weird to me (the viewBox part)... We're multiplying the numerator and denominator of what's supposed to be a ratio by an AppUnitsPerCSSPixel number (which shouldn't actually change anything for a ratio but does here), when (to my mind at least, I'd appreciate correction if I'm wrong here - this is what's bugging me) the viewBox units aren't really CSS pixel units, and we're not really converting them to app units.  This all happens to help because we're trying to force the float viewBox width and height into ints, and so multiplying them up reduces the rounding.  Multiplying by 100, say, would help even more and (again, unless I'm mistaken) be just as correct, and more transparent. My two cents :)

Could we maybe add a note saying that this will still fail for small viewBox coordinates, for future reference?
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6c04a63309
support smaller viewBox coordinates at the expense of larger ones r=dholbert
https://hg.mozilla.org/mozilla-central/rev/dc6c04a63309
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You guys sorted this out while I was away on PTO, but how about this as an alternative: scale the larger value to 10,000, say, and scale the smaller value accordingly. That would eliminate the problems with much larger/smaller viewBox values that still exist after this patch. (This suggestion assumes it's only the ratio of the two values that matters and the actual values don't. But since scaling to app units didn't break anything presumably that is the case.)
Flags: needinfo?(jwatt) → needinfo?(longsonr)
I honestly think the best way is to rewrite the whole lot to return something that contains floats rather than ints
Flags: needinfo?(longsonr)

I have a lot of PATH elements with very small coordinates represented by real numbers. I;ve received them from GIS software and can't scale them easy. Hope that there will be progress on that issue soon.

Regressions: 1571756
You need to log in before you can comment on or make changes to this bug.