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?
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

6 years ago
Depends on: 736431

Updated

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