Based on CSS+SVG discussions at the technical plenary week last week, we should remove our handling of percentages on SVG (including the default width/height of 100%) as intrinsic dimensions. (Percentages should only affect the portion of the SVG viewport into which the image draws.) The behavior is described in http://www.w3.org/TR/SVGMobile12/coords.html#IntrinsicSizing , which should make its way into SVG 1.1 second edition (but hasn't done so yet). The CSS spec will likely remove mentions of percentage intrinsic widths and heights as a result of this discussion; see http://lists.w3.org/Archives/Public/www-style/2010Nov/0077.html . This fix should mean that we pass these tests (except for the table case in the first): http://test.csswg.org/suites/css2.1/20101001/xhtml1/replaced-intrinsic-ratio-001.xht http://test.csswg.org/suites/css2.1/20101027/xhtml1/replaced-intrinsic-ratio-001.xht http://test.csswg.org/source/contributors/fantasai/submitted/css2.1/replaced-intrinsic-ratio-001.htm (although we need to check that the tests are actually correct; they may have errors).

This handling appears to date back to bug 294086.

My test suite bug report and retraction were: http://lists.w3.org/Archives/Public/public-css-testsuite/2010Oct/0278.html http://lists.w3.org/Archives/Public/public-css-testsuite/2010Nov/0049.html

http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1ee897e216a5/remove-percent-intrinsic-size fixes the test, though there's a lot of code removal that needs to be added to the patch (and tests!).

This fixes IsPercentageAware to test for the general condition for when replaced elements size to their container's width rather than looking specifically at the conditions for SVG in a way that depends on SVG reporting a percentage intrinsic width.

Comment on attachment 535145 [details] [diff] [review] patch 3: remove code that handles percentage intrinsic sizes, and remove the concept >diff --git a/layout/base/nsLayoutUtils.cpp b/layout/base/nsLayoutUtils.cpp >+ if (aIntrinsicSize.width.GetUnit() == eStyleUnit_Coord) { > hasIntrinsicWidth = PR_TRUE; >- intrinsicWidth = nsLayoutUtils::ComputeWidthValue(aRenderingContext, >- aFrame, aCBSize.width, 0, boxSizingAdjust.width + >- boxSizingToMarginEdgeWidth, aIntrinsicSize.width); >+ intrinsicWidth = aIntrinsicSize.width.GetCoordValue(); >+ if (intrinsicWidth < 0) >+ intrinsicWidth = 0; Nit: If you like, those last 3 lines could be simplified to: intrinsicWidth = NS_MAX(0, aIntrinsicSize.width.GetCoordValue()); >+ if (aIntrinsicSize.height.GetUnit() == eStyleUnit_Coord) { > hasIntrinsicHeight = PR_TRUE; >- intrinsicHeight = nsLayoutUtils:: >- ComputeHeightDependentValue(aCBSize.height, aIntrinsicSize.height); >+ intrinsicHeight = aIntrinsicSize.height.GetCoordValue(); > if (intrinsicHeight < 0) > intrinsicHeight = 0; (Same here) r=dholbert either way.

Comment on attachment 535144 [details] [diff] [review] patch 2: don't report percentage intrinsic sizes anymore, and fix tests >diff --git a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp > nsSVGOuterSVGFrame::ComputeSize(nsRenderingContext *aRenderingContext, So you've got assertions here for "if percent value, then GetIntrinsicSize should have reported *no* intrinsic width". It'd be nice to also assert "Otherwise [non-percent], it should have reported *some* intrinsic width". Perhaps we could check this at the end of the "We're the root of a browsing context," clause with something like: > NS_ABORT_IF_FALSE(intrinsicSize.height.GetUnit() == eStyleUnit_Coord && > intrinsicSize.width.GetUnit() == eStyleUnit_Coord > "We should have just handled the only situation where" > "we lack an intrinsic height or width."); r=me either way.

https://hg.mozilla.org/mozilla-central/rev/c07445f34e92 https://hg.mozilla.org/mozilla-central/rev/58fe3ede72f8 https://hg.mozilla.org/mozilla-central/rev/3af9fed4e33a

Can someone explain to me what this means for documentation purposes? I'm not clear on what this actually means.

It means that the handling of SVG images, in the <img> element or in an <svg> element inside of HTML, is drastically different when the root SVG element either: * doesn't have a height/width attribute (where it now makes much more sense) * has a height/width attribute in % (where it now works the same as the previous case)

Backed out of mozilla-beta because of bug 668163. https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=9b664712d5f4

Backed it out on mozilla-beta8 as well during the source migration.

(In reply to Christian Legnitto [:LegNeato] from comment #13) > Backed it out on mozilla-beta8 as well during the source migration. That backout being: https://hg.mozilla.org/releases/mozilla-beta/rev/8bea838693f3 https://hg.mozilla.org/releases/mozilla-beta/rev/e8378f974c3e https://hg.mozilla.org/releases/mozilla-beta/rev/8cb916f96ffd

backed out on mozilla-aurora for firefox 9: https://hg.mozilla.org/releases/mozilla-aurora/rev/91108b393572

Is this still on mozilla-central for Firefox 10?

It looks like it. I'm porting it over the backout to aurora10

Actually, the transplants aren't applying cleanly so I am going to keep it as-is and write a note to loop back around.

(Any updates on this, LegNeato? IIUC aurora10 still needs the backout.)

I merged the backouts to Aurora (10) and pushed them: https://hg.mozilla.org/releases/mozilla-aurora/rev/379ab4ae0037 https://hg.mozilla.org/releases/mozilla-aurora/rev/9942cb258707 https://hg.mozilla.org/releases/mozilla-aurora/rev/5f445a3e6205 The net merging was pretty straightforward: https://hg.mozilla.org/users/dbaron_mozilla.com/patches-aurora/rev/57b9b6a35ded (the red and green there should help with the reading diffs-of-diffs, too)

So this change has been backed out; the flurry of flags makes it unclear which Firefox release the backout applies to. Can anyone clarify?

Eric, this change has not shipped in any final release yet. It's proved to be not compatible with the web as-is, so we've been backing it out on aurora or beta as needed while waiting for a followup fix that will make it more web-compatible.

There's a patch in bug 668163 that (once it has review) is worth considering landing on aurora rather than backing this out again.

It looks like this will make it into Firefox 11.

I was removing the stale review request since dholbert's review is sufficient I think. With bug 668163 fixed, I'm fine with this.