1283539 - SVGLength#convertToSpecifiedUnits() incorrectly works with percent units

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[max]

Attachment: HTML with inline SVG and JS. SVG image squeezes after convertToSpecifiedUnits()

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606114208

Steps to reproduce:

I am experimenting with inline SVG with content generated by JavaScript.
I specified <svg> element width in percents using element attribute
expecting to place other elements on the base of user units (or px).
I tried to call length.convertToSpecifiedUnits(SVGLength.SVG_LENGTHTYPE_PX)
to obtain value in pixels and got a surprising result.




Actual results:

If conversion involves percents (to or from) then
length.value is incorrect, e.g. if original width is specified
in percents (50%) the value is fraction (0.5) instead of user units.
Conversion to px preserves value, changes units so on the screen 
element becomes extremely narrow. If original value is in px then the element
becomes extremely wide after conversion to percents.

The behavior is similar in stable 47 and nightly 50 linux versions.
I attach an example. convertToSpecifiedUnits() call is delayed for 3 seconds.
value and valueInSpecifiedUnits are reported for some elements before
and after conversion. Look at the "outerSvg" lines

outerSvg [object SVGLength] value 0.5 50 unit 2
...
outerSvg [object SVGLength] value 0.5 0.5 unit 5


Expected results:

I expect that invocation of convertToSpecifiedUnits()
should not change actual size of the rendered element.

Example of expected values

outerSvg [object SVGLength] value 400 50 unit 2
...
outerSvg [object SVGLength] value 400 400 unit 5

Even if convertToSpecifiedUnits() is not the best way
to get length in other units, it should not ruine the plot.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

The disabled test comes from bug 342513.

Comment 3

[Daniel Holbert [:dholbert]]

Quoting comment 0:
> I expect that invocation of convertToSpecifiedUnits()
> should not change actual size of the rendered element.

So, with the patch applied, I'm still seeing the actual rendered size (of the outer <svg> element) changing, when I apply the patch & load the testcase.  (as revealed by the black-bordered area getting smaller)  It doesn't collapse to 0 anymore; it just collapses to around half of its original width.

I'm not clear why convertToSpecifiedUnits(...PX) would have that result.  Do you know why that's happening?  (It happens in Chrome, too, but it does not happen in OperaPresto (version 12.16).)

Comment 4

[Daniel Holbert [:dholbert]]

(The black-bordered area has a similar size-change in Edge, too.  So, I suspect / am willing to believe it's correct, since it happens pretty interoperably in modern browsers [including Firefox with the attached patch].  But I don't understand why it's correct for convertToSpecifiedUnits to trigger this visible change in size.)

Comment 5

[Robert Longson [:longsonr]]

everything is consistent in SVG, we get the viewport width/height and return the outer svg width/height as 1/2 of that in pixels in both cases.

The difference in display seems to hinge on the fact that in one case we have percent vs percent and therefore no intrinsic size in either direction (https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGOuterSVGFrame.cpp#205) against value vs percent where we do have an intrinsic width. 

We're then into layout's size calculation of the outer SVG element via nsLayoutUtils::ComputeSizeWithIntrinsicDimensions based on those values in which as you say seems consistent cross-browser and really outside the scope of this bug.

Comment 6

[Daniel Holbert [:dholbert]]

OK, thanks. I can imagine a percent-->px change triggering a different intrinsic sizing behavior, so that makes some sense.

I still don't really understand the patch, though -- could you explain what the code-change in the patch is aiming to do, and why it's correct?  (probably with a code-comment, ultimately, though bugzilla is good for now too, for the purposes of review)

In particular -- it kind of looks like it might be changing to resolve the percent in e.g.
  <svg width="50%">
...against the size of that very same <svg> element itself (using it as its own context). But that seems circular and doesn't seem right.

Comment 7

[Robert Longson [:longsonr]]

Normally we resolve percentage sizes against an <svg> ancestor element. The outer <svg> element doesn't have any ancestor to resolve percentages against so has to do it itself.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #2)
> The disabled test comes from bug 342513.

(Thanks -- it looks like the test itself was left in the tree (though not in the manifest) when bug 342513 was backed out, and was later added to the manifest (commented-out/disabled) to make it clearer that the test exists but is not being run.  Glad that we can run/enable it with this patch! :))

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8771720 [details] [diff] [review]
patch

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

I still feel a bit conflicted about the circular dependency here (resolving a percent-size against a basis which is in fact an already-resolved version of that very same percent size).

However, I guess this is interoperable, and it makes as much sense as what we were doing before (with the benefit of rendering in a more-visible way).  So, r=me.

Comment 10

[Daniel Holbert [:dholbert]]

(BTW: I triggered a crash while playing around with the testcase here -- I filed bug 1288228 with a more targeted testcase for that crash. Robert: if you're feeling up to maybe-rethinking the ownership model in our DOM wrappers, feel free to take a crack at that bug. :))

Comment 11

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=605704cdd972

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b388f16a61
Make SVGLength.convertToSpecifiedUnits work for percentage units on outer svg elements. r=dholbert

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51b388f16a61