Last Comment Bug 611099 - remove handling of percentages as intrinsic widths/heights (SVG height="100%" width="100%" defaults)
: remove handling of percentages as intrinsic widths/heights (SVG height="100%"...
Status: RESOLVED FIXED
[DocArea=SVG]
: dev-doc-needed
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla11
Assigned To: David Baron :dbaron: ⌚️UTC-8
:
: Jet Villegas (:jet)
Mentors:
Depends on: 668163 694863 736431 738862 773215 961537
Blocks: css2.1-tests
  Show dependency treegraph
 
Reported: 2010-11-10 12:49 PST by David Baron :dbaron: ⌚️UTC-8
Modified: 2014-01-24 18:54 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix
wontfix


Attachments
patch 1: fix IsPercentageAware (2.27 KB, patch)
2011-05-25 12:14 PDT, David Baron :dbaron: ⌚️UTC-8
dholbert: review+
Details | Diff | Splinter Review
patch 2: don't report percentage intrinsic sizes anymore, and fix tests (29.83 KB, patch)
2011-05-25 12:17 PDT, David Baron :dbaron: ⌚️UTC-8
dholbert: review+
Details | Diff | Splinter Review
patch 3: remove code that handles percentage intrinsic sizes, and remove the concept (4.45 KB, patch)
2011-05-25 12:18 PDT, David Baron :dbaron: ⌚️UTC-8
dholbert: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-8 2010-11-10 12:49:52 PST
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).
Comment 1 David Baron :dbaron: ⌚️UTC-8 2010-11-10 12:54:03 PST
This handling appears to date back to bug 294086.
Comment 2 David Baron :dbaron: ⌚️UTC-8 2010-11-10 12:54:59 PST
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
Comment 3 David Baron :dbaron: ⌚️UTC-8 2011-01-25 10:48:34 PST
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!).
Comment 4 David Baron :dbaron: ⌚️UTC-8 2011-05-25 12:14:43 PDT
Created attachment 535142 [details] [diff] [review]
patch 1: fix IsPercentageAware

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 5 David Baron :dbaron: ⌚️UTC-8 2011-05-25 12:17:13 PDT
Created attachment 535144 [details] [diff] [review]
patch 2: don't report percentage intrinsic sizes anymore, and fix tests
Comment 6 David Baron :dbaron: ⌚️UTC-8 2011-05-25 12:18:14 PDT
Created attachment 535145 [details] [diff] [review]
patch 3: remove code that handles percentage intrinsic sizes, and remove the concept
Comment 7 Daniel Holbert [:dholbert] 2011-05-25 15:22:57 PDT
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 8 Daniel Holbert [:dholbert] 2011-05-27 11:44:58 PDT
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.
Comment 10 Eric Shepherd [:sheppy] 2011-08-17 07:31:12 PDT
Can someone explain to me what this means for documentation purposes? I'm not clear on what this actually means.
Comment 11 David Baron :dbaron: ⌚️UTC-8 2011-08-17 15:19:54 PDT
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)
Comment 12 David Baron :dbaron: ⌚️UTC-8 2011-09-07 09:24:19 PDT
Backed out of mozilla-beta because of bug 668163.
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=9b664712d5f4
Comment 13 christian 2011-09-27 10:03:23 PDT
Backed it out on mozilla-beta8 as well during the source migration.
Comment 14 David Baron :dbaron: ⌚️UTC-8 2011-09-27 11:05:32 PDT
(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
Comment 15 David Baron :dbaron: ⌚️UTC-8 2011-09-27 16:20:12 PDT
backed out on mozilla-aurora for firefox 9:
https://hg.mozilla.org/releases/mozilla-aurora/rev/91108b393572
Comment 16 Eric Shepherd [:sheppy] 2011-10-18 09:35:50 PDT
Is this still on mozilla-central for Firefox 10?
Comment 17 christian 2011-11-08 16:40:46 PST
It looks like it. I'm porting it over the backout to aurora10
Comment 18 christian 2011-11-08 16:47:46 PST
Actually, the transplants aren't applying cleanly so I am going to keep it as-is and write a note to loop back around.
Comment 19 Daniel Holbert [:dholbert] 2011-11-28 14:13:39 PST
(Any updates on this, LegNeato?  IIUC aurora10 still needs the backout.)
Comment 20 David Baron :dbaron: ⌚️UTC-8 2011-12-02 14:35:06 PST
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)
Comment 21 Eric Shepherd [:sheppy] 2011-12-05 05:09:41 PST
So this change has been backed out; the flurry of flags makes it unclear which Firefox release the backout applies to. Can anyone clarify?
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-12-05 06:29:36 PST
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.
Comment 23 Jonathan Watt [:jwatt] 2011-12-20 15:34:22 PST
There's a patch in bug 668163 that (once it has review) is worth considering landing on aurora rather than backing this out again.
Comment 24 David Baron :dbaron: ⌚️UTC-8 2012-02-15 07:39:18 PST
It looks like this will make it into Firefox 11.
Comment 25 Jonathan Watt [:jwatt] 2012-02-15 08:04:26 PST
I was removing the stale review request since dholbert's review is sufficient I think. With bug 668163 fixed, I'm fine with this.
Comment 26 Robert Longson 2014-01-20 02:17:58 PST
*** Bug 961537 has been marked as a duplicate of this bug. ***

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