1396642 - SVG wrong size with viewBox width/height non-integers

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Simon Lydell]

Attachment: 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.

Comment 2

[Tom Klein]

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.)

Comment 3

[Robert Longson [:longsonr]]

Attachment: 1396642.txt

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

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 9008951 [details] [diff] [review]
1396642.txt

This looks good, though it needs an automated test of some sort.

Comment 5

[Robert Longson [:longsonr]]

Attachment: with mochitest

Comment 6

[Tom Klein]

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?

Comment 7

[Robert Longson [:longsonr]]

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.

Comment 8

[Tom Klein]

Thanks :)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 9009257 [details] [diff] [review]
with mochitest

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

Thanks! r=me

Comment 10

[Tom Klein]

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?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6c04a63309
support smaller viewBox coordinates at the expense of larger ones r=dholbert

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/dc6c04a63309

Comment 13

[Jonathan Watt [:jwatt]]

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.)

Comment 14

[Robert Longson [:longsonr]]

I honestly think the best way is to rewrite the whole lot to return something that contains floats rather than ints

Comment 15

[xentia]

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.