Closed Bug 1283539 Opened 4 years ago Closed 3 years ago
To Specified Units() incorrectly works with percent units
Component: Untriaged → SVG
Product: Firefox → Core
Assignee: nobody → longsonr
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8771720 - Flags: review?(dholbert)
The disabled test comes from bug 342513.
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).)
3 years ago
(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.)
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.
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.
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.
(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 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.
Attachment #8771720 - Flags: review?(dholbert) → review+
3 years ago
See Also: → 1288228
(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. :))
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/51b388f16a61 Make SVGLength.convertToSpecifiedUnits work for percentage units on outer svg elements. r=dholbert
You need to log in before you can comment on or make changes to this bug.