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

RESOLVED FIXED in mozilla11

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
7 years ago
4 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)

(Assignee)

Description

7 years ago
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).
(Assignee)

Comment 1

7 years ago
This handling appears to date back to bug 294086.
(Assignee)

Comment 2

7 years ago
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
(Assignee)

Updated

7 years ago
Summary: removing handling of percentages as intrinsic widths/heights → remove handling of percentages as intrinsic widths/heights (SVG height="100%" width="100%" defaults)
(Assignee)

Comment 3

7 years ago
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!).
(Assignee)

Comment 4

6 years ago
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)
(Assignee)

Comment 5

6 years ago
Created attachment 535144 [details] [diff] [review]
patch 2: don't report percentage intrinsic sizes anymore, and fix tests
Attachment #535144 - Flags: review?(dholbert)
(Assignee)

Updated

6 years ago
Attachment #535144 - Flags: review?(jwatt)
(Assignee)

Comment 6

6 years ago
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+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/c07445f34e92
https://hg.mozilla.org/mozilla-central/rev/58fe3ede72f8
https://hg.mozilla.org/mozilla-central/rev/3af9fed4e33a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
Depends on: 668163
Can someone explain to me what this means for documentation purposes? I'm not clear on what this actually means.
(Assignee)

Comment 11

6 years ago
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)
(Assignee)

Comment 12

6 years ago
Backed out of mozilla-beta because of bug 668163.
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=9b664712d5f4
status-firefox7: --- → wontfix
Target Milestone: mozilla7 → mozilla8

Comment 13

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

Comment 14

6 years ago
(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
(Assignee)

Updated

6 years ago
Target Milestone: mozilla8 → mozilla10
(Assignee)

Comment 15

6 years ago
backed out on mozilla-aurora for firefox 9:
https://hg.mozilla.org/releases/mozilla-aurora/rev/91108b393572
status-firefox9: --- → wontfix

Updated

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

Comment 17

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

Comment 18

6 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.)
(Assignee)

Comment 20

6 years ago
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?

Comment 22

6 years ago
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

6 years ago
Attachment #535144 - Flags: review?(jwatt)
(Assignee)

Comment 24

6 years ago
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.
Keywords: dev-doc-needed
Keywords: dev-doc-needed

Updated

5 years ago
Depends on: 736431

Updated

5 years ago
Depends on: 738862

Updated

5 years ago
Depends on: 773215

Updated

4 years ago
Depends on: 961537

Updated

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