Closed Bug 611099 Opened 14 years ago Closed 13 years ago

remove handling of percentages as intrinsic widths/heights (SVG height="100%" width="100%" defaults)


(Core :: Layout, defect, P3)




Tracking Status
firefox7 --- wontfix
firefox8 --- wontfix
firefox9 --- wontfix
firefox10 --- wontfix


(Reporter: dbaron, Assigned: dbaron)


(Blocks 1 open bug)


(Keywords: dev-doc-needed, Whiteboard: [DocArea=SVG])


(3 files)

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

This fix should mean that we pass these tests (except for the table case in the first):
(although we need to check that the tests are actually correct; they may have errors).
This handling appears to date back to bug 294086.
Summary: removing handling of percentages as intrinsic widths/heights → remove handling of percentages as intrinsic widths/heights (SVG height="100%" width="100%" defaults) 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.
Assignee: nobody → dbaron
Attachment #535142 - Flags: review?(dholbert)
Attachment #535142 - Flags: review?(dholbert) → review+
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.
Attachment #535145 - Flags: review?(dholbert) → review+
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.
Attachment #535144 - Flags: review?(dholbert) → review+
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 it out on mozilla-beta8 as well during the source migration.
Target Milestone: mozilla8 → mozilla10
Depends on: 694863
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:

The net merging was pretty straightforward:
(the red and green there should help with the reading diffs-of-diffs, too)
Target Milestone: mozilla10 → mozilla11
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.
Attachment #535144 - Flags: review?(jwatt)
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.
Depends on: 736431
Depends on: 738862
Depends on: 773215
Depends on: 961537
Whiteboard: [DocArea=SVG]
You need to log in before you can comment on or make changes to this bug.