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)
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: dbaron, Assigned: dbaron)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, Whiteboard: [DocArea=SVG])
Attachments
(3 files)
2.27 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
29.83 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
4.45 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
This handling appears to date back to bug 294086.
Assignee | ||
Comment 2•14 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•14 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•14 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•14 years ago
|
||
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 | ||
Comment 5•14 years ago
|
||
Attachment #535144 -
Flags: review?(dholbert)
Assignee | ||
Updated•14 years ago
|
Attachment #535144 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #535145 -
Flags: review?(dholbert)
Updated•14 years ago
|
Attachment #535142 -
Flags: review?(dholbert) → review+
Comment 7•14 years ago
|
||
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 8•14 years ago
|
||
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•13 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
Closed: 13 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 10•13 years ago
|
||
Can someone explain to me what this means for documentation purposes? I'm not clear on what this actually means.
Assignee | ||
Comment 11•13 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•13 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•13 years ago
|
||
Backed it out on mozilla-beta8 as well during the source migration.
status-firefox8:
--- → wontfix
Assignee | ||
Comment 14•13 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•13 years ago
|
Target Milestone: mozilla8 → mozilla10
Assignee | ||
Comment 15•13 years ago
|
||
backed out on mozilla-aurora for firefox 9:
https://hg.mozilla.org/releases/mozilla-aurora/rev/91108b393572
status-firefox9:
--- → wontfix
Comment 16•13 years ago
|
||
Is this still on mozilla-central for Firefox 10?
Comment 17•13 years ago
|
||
It looks like it. I'm porting it over the backout to aurora10
Comment 18•13 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.
Comment 19•13 years ago
|
||
(Any updates on this, LegNeato? IIUC aurora10 still needs the backout.)
Assignee | ||
Comment 20•13 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
Comment 21•13 years ago
|
||
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•13 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.
Comment 23•13 years ago
|
||
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•13 years ago
|
Attachment #535144 -
Flags: review?(jwatt)
Assignee | ||
Comment 24•13 years ago
|
||
It looks like this will make it into Firefox 11.
Comment 25•13 years ago
|
||
I was removing the stale review request since dholbert's review is sufficient I think. With bug 668163 fixed, I'm fine with this.
Updated•13 years ago
|
Keywords: dev-doc-needed
Updated•13 years ago
|
Keywords: dev-doc-needed
Updated•11 years ago
|
Whiteboard: [DocArea=SVG]
You need to log in
before you can comment on or make changes to this bug.
Description
•