Closed Bug 414112 Opened 17 years ago Closed 17 years ago

CSS 2.1 300px fallback width doesn't play nice with percentages other than 100%

Categories

(Core :: SVG, defect, P2)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

testcase
(deleted), application/xhtml+xml
Details
patch
(deleted), patch
bzbarsky
: review+
tor
: superreview+
Details | Diff | Splinter Review
Attached file testcase (deleted) —
Whenever we have <svg width="X%"> inside a containing block (CB) that depends on the <svg>'s width, the <svg> should fall back to a width of 300px. Instead, the <svg> falls back to a width of X% of 300px. This bug is caused because the layout code sizes the CB by calling GetPrefWidth on it's children. When the <svg> returns 300px in the testcase, the CB takes that width, and this is the width passed into nsLayoutUtils::ComputeSizeWithIntrinsicDimensions as aCBSize.width. Unfortunately the intrinsic width of X% is then applied to aCBSize.width to calculate the <svg>'s used width, and hence the bug. I'm not really sure how best to fix this. We can't pre-multiply the value nsSVGOuterSVGFrame::GetPrefWidth returns in preparation for ComputeSizeWithIntrinsicDimensions, since then the CB would take the wrong size. Perhaps there's a way to tell in ComputeSizeWithIntrinsicDimensions whether the CB's width depends on us, but we only get given the CB's size, not the CB, so I don't see how.
> Whenever we have <svg width="X%"> inside a containing block (CB) that depends > on the <svg>'s width, the <svg> should fall back to a width of 300px. I assume the issue is this spec text? Percentage intrinsic widths are first evaluated with respect to the containing block's width, if that width doesn't itself depend on the replaced element's width. If it does, then a percentage intrinsic width on that element can't be resolved and the element is assumed to have no intrinsic width. Telling whether the CB width depends on us is actually pretty nontrivial... It might make more sense to change the spec here or something. I'll post to www-style about this....
That's correct.
Attached patch patch-ish (obsolete) (deleted) — — Splinter Review
So I guess something like this doesn't cover all the bases? (It fixes the testcase.)
Come to think about it, we can probably set a bit on the nsSVGOuterSVGFrame in GetPrefWidth so we know further on in reflow that our CB's width depends on us. We'd need to clear the bit in MarkIntrinsicWidthsDirty since GetPrefWidth can be called for reasons other than reflow, but this trick should allow us to cover all the bases I think. Not elegant, but effective. Thoughts?
> So I guess something like this doesn't cover all the bases? It'll treat a basic <div> as depending on the width of the SVG, whereas in reality it does not in this testcase: <html><div><svg/></div></html> As for comment 4, there's no guarantee that GetPrefWidth() being called means that the ancestor in fact depends on your width.
Flags: blocking1.9?
Comment on attachment 299419 [details] [diff] [review] patch-ish Actually this breaks a whole bunch of the SVG sizing reftests.
Attachment #299419 - Attachment is obsolete: true
So Boris convinced me and www-style that the 300px thing doesn't work. That section has been removed from the CSS 2.1 draft: http://lists.w3.org/Archives/Public/www-style/2008Feb/0029.html I guess percentage intrinsic widths on elements in a CB with dependent width should resolve against the width the CB would have if all the CB's children with percentage widths were given a width of zero. In other words nsSVGOuterSVGFrame::GetPrefWidth should return zero.
Attached patch patch (deleted) — — Splinter Review
Assignee: nobody → jwatt
Status: NEW → ASSIGNED
Attachment #301493 - Flags: superreview?(tor)
Attachment #301493 - Flags: review?(bzbarsky)
Attachment #301493 - Flags: review?(bzbarsky) → review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Attachment #301493 - Flags: superreview?(tor) → superreview+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
This bug reverted the change introduced in bug 382325 BTW.
Summary: CSS 2.1 300px fallback width doesn't play nice with percentages → CSS 2.1 300px fallback width doesn't play nice with percentages other than 100%
Flags: in-testsuite?
I just looked at this using the testcase in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050706 Minefield/3.0pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre and I'm getting a blank window with this test case. I suspect that another bug is masking this but if I look the testcase in current branch, I get a red square and a blue square drawn on the screen (which is better than nothing at all).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
A blank window is exactly what's expected. The floated div has no explicit width or height, so its dimensions depend on its content. If it has no content, or if its content takes up no room, the width and height of the div will be zero. As it happens the div does have some content, but this content has a percentage width and height. These percentages need to be resolved, but how do you resolve them with the cyclic dependency of the parent div depending on its content? We decided that the best thing we could do was to resolve the percentages against the dimensions that the parent div would have if it didn't include the children with the percentage dimensions. In other words the percentages in this case are resolved as percentages of zero. I.e., zero.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Oh, and this comes down to Boris saying it's pretty much impossible in general to know when the containing block's dimensions depend on us, so it's not possible to implement the 300 x 150 px fallback. If I remember correctly, Boris brought that up on www-style, and that section has now been removed from CSS 2.1 draft.
All right. Thanks, Jonathan. I'll mark this as verified then. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Could we add the test as a reftest, then?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: