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

RESOLVED FIXED in mozilla11

Status

()

P3
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla11
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox7 wontfix, firefox8 wontfix, firefox9 wontfix, firefox10 wontfix)

Details

(Whiteboard: [DocArea=SVG])

Attachments

(3 attachments)

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).
Summary: removing handling of percentages as intrinsic widths/heights → remove handling of percentages as intrinsic widths/heights (SVG height="100%" width="100%" defaults)
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.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #535142 - Flags: review?(dholbert)
Created attachment 535144 [details] [diff] [review]
patch 2: don't report percentage intrinsic sizes anymore, and fix tests
Attachment #535144 - Flags: review?(dholbert)
Created attachment 535145 [details] [diff] [review]
patch 3: remove code that handles percentage intrinsic sizes, and remove the concept
Attachment #535145 - 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+
Depends on: 668163
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)

Comment 13

7 years ago
Backed it out on mozilla-beta8 as well during the source migration.
status-firefox8: --- → wontfix

Updated

7 years ago
Depends on: 694863
Is this still on mozilla-central for Firefox 10?

Comment 17

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

Comment 18

7 years ago
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)
status-firefox10: --- → wontfix
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.

Updated

7 years ago
Attachment #535144 - Flags: review?(jwatt)
I was removing the stale review request since dholbert's review is sufficient I think. With bug 668163 fixed, I'm fine with this.
Keywords: dev-doc-needed
Keywords: dev-doc-needed

Updated

7 years ago
Depends on: 736431

Updated

7 years ago
Depends on: 738862

Updated

6 years ago
Depends on: 773215

Updated

5 years ago
Depends on: 961537
Duplicate of this bug: 961537
Whiteboard: [DocArea=SVG]
You need to log in before you can comment on or make changes to this bug.