Note to self: I had to remove 294120 from the depends list because it was causing a circular dependency.

In fact lets put it in the blocks list so we're at least linked.

So here's the current incarnation of my attempts to fix SVG sizing. It's incomplete, but worth getting out so interested parties can take a look. A few things to note: * nsLayoutUtils::ComputeSizeWithIntrinsicDimensions still needs a fair bit of work. * nsSVGOuterSVGFrame::GetMinWidth/GetPrefWidth - I'm not sure what to return when the intrinsic size is a percentage value. * nsSubDocumentFrame::ObtainIntrinsicSizeFrame is called more than I'd like during reflow. Maybe there's a way we can cache the result, but care would need to be taken to make sure nobody de-refs a dangling pointer. * Bunch of other smaller stuff to resolve. On the plus side, this fixes all the testcases on this bugs deps.

> * nsSVGOuterSVGFrame::GetMinWidth/GetPrefWidth - I'm not sure what to return > when the intrinsic size is a percentage value. What is the expected layout of such an intrinsic size when placed inside a <div style="float: left">? The possibilities I see are to either have a min-size of 0 and unconstrained pref or to set them both to 0 or to try to do what a <div style="width: 100%"> would end up doing for IntrinsicForContainer.... but maybe we don't really need that. In any case, this is a spec question.

CSS 2.1 defines the behavior doesn't it? In the case where the containing block's width relies on a replaced element child that has a percentage intrinsic width, the child is assumed to have no intrinsic width. It will then default to 300 px unless the height is specified and the element has an intrinsic ratio. GetMinWidth/GetPrefWidth can't know about whether the parent will rely on it though.

Sure they can. If they're being called, the parent is relying on it.

This is virtually ready for review I think. I'm finished with the replaced element algorithm (nsLayoutUtils::ComputeSizeWithIntrinsicDimensions). One issue I'm unsure of is the GetPrevInFlow stuff I've added to nsSVGOuterSVGFrame::Reflow (commented out). I'm guessing I need it for something, but I can't quite tell what it's for, or why the versions in nsImageFrame and nsHTMLCanvasFrame are different. I'm in the midsts of writing extra (ref)tests, and I'd like to resolve the issue above before requesting review. Having said that, I'll have very few cycles and limited access to my computer between now and the code freeze, so if someone wants to jump in early and start looking at this patch that'd be great. (Two blocking1.9+ and two blocking1.9? bugs depend on this.)

You need the prev-in-flow thing if you can actually be paginated (that is, have part of the SVG outer frame on one page, and part on another). Images can (because they return aStatus != NS_FRAME_COMPLETE as needed). Your code can't as far as I can see. The GetPrevInFlow() stuff in nsHTMLCanvasFrame::Reflow looks bogus to me, as does yours; it might be better to assert that there isn't one.

Thanks Boris, I'll do that.

Comment on attachment 279083 [details] [diff] [review] patch v3 >+ svgElem->mViewportWidth = >+ nsPresContext::AppUnitsToFloatCSSPixels(aReflowMetrics.width); >+ svgElem->mViewportHeight = >+ nsPresContext::AppUnitsToFloatCSSPixels(aReflowMetrics.height); I should get the computed values from aReflowState there. Borders are still not being painted into the space that's made available for them though for some reason. I don't think that's a blocker for this, but it would be nice to have that working too.

I don't think I can get to this in time for the M8 freeze. I should be able to review towards the end of Sept, though.

Comment on attachment 279131 [details] [diff] [review] patch v4 - now supporting borders and padding I'm working on testing this to death and I have a good grasp of the changes I'm making. However, I think this is still high enough risk that we want to get it in for M8 if at all possible and get other people testing their SVG on it. I'd rather not end up having to land it later if at all possible, so I'll move the review request to dbaron for now. Thanks bz.

Keep in mind that M8 freeze is on Tuesday and this is a holiday weekend. I suspect getting reviews will be difficult in general.... :(

I thought it was Thursday for some reason, and I didn't realize it was a holiday weekend. :-( We'll see I guess.

+ // Get tentitive values - CSS 2.1 sections 10.3.2 and 10.6.2: It's tentative

Thanks, I'll fix that in the next patch.

Here are the comments I have so far. I still need to look at: * the nsSVGOuterSVGFrame changes * CSS2.1 section 10.3.2 * handling of intrinsic widths for things with specified height and intrinsic ratio and perhaps some other things. But I want a backup of my comments so far: In nsSVGSVGElement, the removal of SetCoordCtxRect means you'll never initialize mViewportWidth and mViewportHeight to anything nonzero. But they're still used in a bunch of places, so that seems like a problem Seems like you should rename ObtainIntrinsicSizeFrame to GetIntrinsicSizeFrame, unless there's a good reason to use Obtain rather than Get. I'm not entirely comfortable with calling the percentages an intrinsic size rather than a specified size, but I suppose it works, and we probably do need it at this level. How do you handle when the document inside such an <object> changes, e.g., due to link navigation? nsSubDocumentFrame::ObtainIntrinsicSizeFrame seems way too complicated. You can just get the document from the pres shell and the root element from the document, no? Why are you checking GetIntrinsicRatio in IsPercentageAware in nsLineLayout? That seems like it will include a good bit of stuff that shouldn't be included. Why are you adding display:inline to svg.css? display:inline is the default. In nsLayoutUtils.cpp: Where you set hasIntrinsicWidth and hasIntrinsicHeight, could you make the width and the height if/else the same way around rather than the opposite of each other? I'm a little skeptical that condensing the intrinsic ratio's height and width both being nonzero into hasIntrinsicRatio -- it's defined such that if one side is nonzero than there's a ratio, although I'm not sure whether that can actually happen. Maybe that's what the spec says to do -- but at least add a comment that it's possible to handle 0xN and Nx0 better in some cases. Though there are actually a bunch of cases that you're changing from the better handling to the worse handling, although maybe it doesn't really matter. What was the motivation for the change? I also don't like the reuse of the |width| and |height| variables for two different things. Why not call your new one |intrinsicWidth| and |intrinsicHeight| or |intrinsicWidthCoord| and |intrinsicHeightCoord| and keep them separate? (You'd need to readd the very last clause in the if/else chain, where you'd do width=intrinsicWidth and height=intrinsicHeight.) Have you tested fantasai's tests for the min/max-height handling rules on images? They should really be part of reftests if they're not already. + } else if (height < minHeight) { + width = maxWidth; + height = minHeight; + } else if (height > maxHeight) { width = minWidth; height = maxHeight; These two three-line insertions aren't needed, but you might want to instead add a comment around the else following each of them that that else covers two rows of the table. We know the extras aren't needed since if (w < min-width) and (h > max-height), then we know that: * max(max-height * w/h, min-width) is equal to min-width since the two sides of the max() are the product of the two smaller terms in the inequalities (w and max-height) divided by h, and the product of the two larger terms in the inequalities (h and min-width) divided by h. * min(min-width * h/w, max-height) is equal to max-height since the two sides of the min() are the product of the two larger terms in the inequalities (min-width and h) divided by w, and the product of the two smaller terms in the inequalities (w and max-height) divided by w. Thus the last two rows of the table really aren't needed -- they're just clarifying that in that case the two out of the first four rows of the table that apply both yield the same result -- so we can just include the code for whichever of those two rows is easier. It's worth a comment to explain this, though. I don't like the unindenting (removal of else{}) surrounding the height conditions around the end of that if/else chain. It was nice to have all the width conditions at one level of indentation and all the height conditions at the next.

I'm having trouble making sense of some of the code in nsSVGOuterFrame because I'm not sure what svg->mLengthAttributes[nsSVGSVGElement::WIDTH] gives in the case where nothing is specified. The 1.1 spec suggests 100% (which I think gives bad results, since it avoids a bunch of the nice things in CSS2.1, although that may have been changed in errata and/or 1.2), but it looks like our code gives 0, unless I'm missing something.

Anyway, may as well post the next chunk of review comments, although I'm expecting to have some pretty substantial comments on the GetPrefWidth / GetIntrinsicRatio / GetIntrinsicSize / Reflow code in nsSVGOuterFrame once I understand it: I realized that some of the nsLayoutUtils.cpp changes are wrong if box-sizing has a non-default value. Your added calls to ComputeWidthValue and ComputeHeightDependentValue when you're passing in aIntrinsicSize.width and aIntrinsicSize.height need to be computed using bogus box sizing values -- you need to replace (when and around making those calls) boxSizingAdjust.width and .height with 0 and boxSizingToMarginEdgeWidth with (boxSizingToMarginEdgeWidth + boxSizingAdjust.width) since the intrinsic sizes always use content-box widths and heights. And the start of my nsSVGOuterFrame.cpp comments: In nsSVGOuterSVGFrame::AttributeChanged: + if (embeddingFrame && !HasFixedSize(embeddingFrame)) { ... + } else if (!embeddingFrame) { This pattern is probably better written as: if (embeddingFrame) { if (!HasFixedSize(embeddingFrame)) { ... } } else { ... } or maybe better as: if (!embeddingFrame) { ... } else if (!HasFixedSize(embeddingFrame)) { ... } In nsSVGOuterSVGFrame::GetPrefWidth, I think using 300 is wrong when you have an intrinsic ratio -- at least for min-width, if not for pref-width as well. I think min-width should be 0, and probably pref-width too, since the SVG will fit in its container. You have some occurrences of NS_STATIC_CAST that need to be static_cast now.

(In reply to comment #19) > In nsSVGSVGElement, the removal of SetCoordCtxRect means you'll never > initialize mViewportWidth and mViewportHeight to anything nonzero. But > they're still used in a bunch of places, so that seems like a problem The only caller of SetCoordCtxRect used to be nsSVGOuterSVGFrame::Reflow, and now I just set mViewportWidth and mViewportHeight directly in that method. > Seems like you should rename ObtainIntrinsicSizeFrame to > GetIntrinsicSizeFrame, unless there's a good reason to use Obtain rather > than Get. My rational was that nsSubDocumentFrame::ObtainIntrinsicSizeFrame calls nsSubDocumentFrame::GetDocShell, and nsSubDocumentFrame::GetDocShell has the following comment: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrameFrame.cpp&rev=3.317&mark=642,643#641 > How do you handle when the document inside such an <object> changes, > e.g., due to link navigation? Ah, I covered the case where the document inside an <object> changes from a document without intrinsic size to SVG with intrinsic size, but I hadn't considered the converse. I'll look into that. > nsSubDocumentFrame::ObtainIntrinsicSizeFrame seems way too complicated. > You can just get the document from the pres shell and the root element > from the document, no? It's the frame for the root element I want though. I can perhaps change the code to: nsCOMPtr<nsIDocShell> docShell; GetDocShell(getter_AddRefs(docShell)); if (docShell) { nsCOMPtr<nsIPresShell> presShell; docShell->GetPresShell(getter_AddRefs(presShell)); if (presShell) { nsIDocument* subDoc = presShell->GetDocument(); if (subDoc) { nsIContent* rootContent = subdoc->GetRootContent(); if (rootContent) { subDocRoot = presShell->GetPrimaryFrameFor(rootContent); } } } } if you like, but I was trying to avoid the GetPrimaryFrameFor overhead. What do you think? > Why are you checking GetIntrinsicRatio in IsPercentageAware in > nsLineLayout? That seems like it will include a good bit of stuff that > shouldn't be included. I really can't recall now, but it was something to do with bug 369850 comment 16. From my tests it doesn't appear to be necessary any more though, so I can remove it. > Why are you adding display:inline to svg.css? display:inline is the > default. Just to make it explicit that the SVG spec requires it to default to inline.

(In reply to comment #19) > I also don't like the reuse of the |width| and |height| variables for > two different things. Why not call your new one |intrinsicWidth| and > |intrinsicHeight| or |intrinsicWidthCoord| and |intrinsicHeightCoord| > and keep them separate? (You'd need to readd the very last clause in > the if/else chain, where you'd do width=intrinsicWidth and > height=intrinsicHeight.) I don't really think "intrinsic" should be in the name. The intrinsic values are fixed values (during the lifetime of the function call), which are then modified according to the section 10 algorithm, on their way to becoming the used values. I'll add two variables tmpWidth and tmpHeight to indicate they contain intermediary values. > Have you tested fantasai's tests for the min/max-height handling rules > on images? They should really be part of reftests if they're not > already. I've been running our entire reftest suite, and all 19 tests on http://fantasai.inkedblade.net/style/specs/css2.1/tests/min-max-replaced pass. I also have the ~180 reftests I wrote to cover this bug. I'll upload them to my site and include the reftest.list for them in my next patch. > These two three-line insertions aren't needed, but you might want to > instead add a comment... Ah, I see, will do. > I don't like the unindenting (removal of else{}) surrounding the height > conditions around the end of that if/else chain. It was nice to have > all the width conditions at one level of indentation and all the height > conditions at the next. The problem I had was that it really confused me the first time I was trying to correlate the code with the table in the spec. I found my changes made it a lot easier for me, but if you want I can revert that part.

(In reply to comment #20) > I'm having trouble making sense of some of the code in nsSVGOuterFrame because > I'm not sure what svg->mLengthAttributes[nsSVGSVGElement::WIDTH] gives in the > case where nothing is specified. Yeah, our code to get the defaults is very "clever" in how it saves space, but it sure isn't easy to figure out. So what happens in the case of no width/height attributes is that when we end up here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGElement.cpp&rev=1.140&mark=112-117#102 the lengthInfo.mLengthInfo is this static object here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGSVGElement.cpp&rev=1.105&mark=67-73#66 As you can see that gives the default for width and height as 100%.

(In reply to comment #21) > I realized that some of the nsLayoutUtils.cpp changes are wrong if > box-sizing has a non-default value...since the intrinsic sizes always use > content-box widths and heights. Actually I did that intentionally since I thought it was best for box-sizing to be taken into account for intrinsic dimensions. I was thinking in the context of SVG at the time, specifically inline SVG with width/height attributes. In retrospect those changes don't make as much sense for other image types, so I'll fix that up as you suggest and perhaps revisit for SVG in future. > In nsSVGOuterSVGFrame::AttributeChanged: > + if (embeddingFrame && !HasFixedSize(embeddingFrame)) { > ... > + } else if (!embeddingFrame) { > This pattern is probably better written as: > if (embeddingFrame) { > if (!HasFixedSize(embeddingFrame)) { > ... > } > } else { > ... > } Then we'd never request any reflow at all if there was an embeddingFrame which doesn't rely on our intrinsic size. Actually come to think of it, in that case the SVG's width and height is overridden, so that's exactly what we want. I'll fix that in the next patch. > In nsSVGOuterSVGFrame::GetPrefWidth, I think using 300 is wrong when you > have an intrinsic ratio -- at least for min-width, if not for pref-width > as well. I think min-width should be 0, and probably pref-width too, > since the SVG will fit in its container. Okay.

(In reply to comment #25) > Then we'd never request any reflow at all if there was an embeddingFrame which > doesn't rely on our intrinsic size. Actually come to think of it, in that case I wasn't proposing any logic change -- just not checking embeddingFrame twice.

(In reply to comment #22) > The only caller of SetCoordCtxRect used to be nsSVGOuterSVGFrame::Reflow, and > now I just set mViewportWidth and mViewportHeight directly in that method. Could you use a setter and make the members private? It seems highly confusing to have member variables set in another *module*.

Glad to hear you have so many reftests. I'm actually a bit hesitant about supporting borders and padding on the svg element given that 100% default, since it means the borders and padding will overflow by default. Do you know if other browsers support them? I notice you're removing SuspendRedraw/UnsuspendRedraw from nsSVGOuterSVGFrame::Reflow -- why were they there, and why aren't they needed anymore? In nsSVGOuterSVGFrame::Reflow, could you leave the nsHTMLReflowMetrics parameter called aDesiredSize, for consistency with other reflow methods? + aReflowMetrics.width = aReflowState.ComputedWidth(); + aReflowMetrics.height = aReflowState.ComputedHeight(); + + // add borders and padding + aReflowMetrics.width += aReflowState.mComputedBorderPadding.LeftRight(); + aReflowMetrics.height += aReflowState.mComputedBorderPadding.TopBottom(); Seems like you should just assign the sums straight away rather than using two statements. + // XXX what if we have CSS width/height? How can we tell the embedding + // element about changes to those properties? You should probably implement DidSetStyleContext, although it doesn't currently have access to the old style data (but it would be very easy to give it access, although that would probably be best as a separate patch because it'll have a lot of boring signature changes). Given that, I'm ok if you do that as a later patch, but I think it should happen for 1.9. (The CSS width/height affect the SVG's mLengthAttributes, right?) In nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, can't you assert that aCBSize.width != NS_UNCONSTRAINEDSIZE rather than checking it? (You need to check the height, though.) r+sr=dbaron with all these comments addressed, although I think I'd like to look at the patch that addresses them to check some of them.

> Could you use a setter and make the members private? It seems highly > confusing to have member variables set in another *module*. Sure. My problem was that I needed something like an nsSize that has float members, not nscoord or double members. I'll add that I guess. > I'm actually a bit hesitant about supporting borders and padding on the > svg element given that 100% default, since it means the borders and > padding will overflow by default. Do you know if other browsers support > them? Opera and Safari both support border, padding and margin on SVG. The 100% is stupid in this case of course, which is partly why I wanted intrinsic sizing to take box-sizing into account on SVG. Anyway, users can specify the width and height, so I think it's still useful. > I notice you're removing SuspendRedraw/UnsuspendRedraw from > nsSVGOuterSVGFrame::Reflow -- why were they there, and why aren't they > needed anymore? They were there to batch together the changes made by various calls. Actually, they're no longer needed even in the current code, since the only thing that might require them is the NotifyViewportChange() call, and it has it's own SuspendRedraw/UnsuspendRedraw calls. They should have been removed already. > In nsSVGOuterSVGFrame::Reflow, could you leave the nsHTMLReflowMetrics > parameter called aDesiredSize, for consistency with other reflow > methods? If I must. I hate that name though (the word "metrics" is a lot less misleading about what it contains IMHO). > + // XXX what if we have CSS width/height? How can we tell the embedding > + // element about changes to those properties? > > You should probably implement DidSetStyleContext, although it doesn't > currently have access to the old style data (but it would be very easy > to give it access, although that would probably be best as a separate > patch because it'll have a lot of boring signature changes). Given > that, I'm ok if you do that as a later patch, but I think it should > happen for 1.9. Actually I wrote that comment when I was thinking that embedding elements like <object> should use the CSS width/height on the embedded document's root as its intrinsic dimensions if specified. I.e. CSS should be taken over the attributes when asking embedded SVG documents for their intrinsic dimensions. Thinking about that got me into a whole world of pain though - although it would make some sense, it's not part of a spec., and it has implementation complications - so I concluded that for the purposes of intrinsic sizing we should ignore CSS on embedded SVG for now. If someone specifies CSS and the attributes then embeds the SVG by reference they'll get what they get. It's a much bigger issue that should be revisited later I think. > (The CSS width/height affect the SVG's mLengthAttributes, right?) No, mLengthAttributes only contains the values parsed from the attributes. It wouldn't be much use as the source of the intrinsic dimensions for CSS sizing if CSS width/height were mapped to them.

(In reply to comment #29) > > (The CSS width/height affect the SVG's mLengthAttributes, right?) > > No, mLengthAttributes only contains the values parsed from the attributes. It > wouldn't be much use as the source of the intrinsic dimensions for CSS sizing > if CSS width/height were mapped to them. Hrm. In that case, it seems like there are a lot of tests that aren't right, although maybe if they're consistently wrong it's not so horrible. I'll try to look at that more later...

I would really like for this patch to go in for M9 if possible since it fixes so much, and I'd like it to get exposure so I can know about any problems as soon as possible. If you're happy to give it r+sr can you also mark approval? Concerning the resizing after unloading SVG in <object>, it isn't entirely straightforward to fix since although nsObjectLoadingContent inherits from nsImageLoadingContent, the only observers that can currently be registered are imgIDecoderObserver observers. The unload resizing is a minor problem compared to all the other issues this fixes, but I'll certainly work to fix it (and I added a reftest for this case). Oh, and note that the IsPercentageAware change is required for inline SVG to reflow correctly when it has a percentage width attribute and its containing block is resized. (In reply to comment #30) > Hrm. In that case, it seems like there are a lot of tests that aren't right, > although maybe if they're consistently wrong it's not so horrible. I'll try > to look at that more later... Not quite sure what you mean there.

Did you mean to just attach the reftests names or did you mean to attach their contents?

That was deliberate so as not to make the patch too large to attach (adding the file reftest.list has bumped up the size enough as it is). You can find the actual reftests at http://jwatt.org/moz/tmp/sizing/

Just to remind myself, for <svg width="50" style="width: 25px" /> (with some contents), the width attribute is supposed to win in standalone SVG (since the 'width' CSS property doesn't apply in SVG), but the CSS property wins when the svg element is a child element of an HTML element, since the width CSS property applies at that boundary case. So it's correct that IntrinsicSize doesn't consider the CSS 'width' property in any cases. So I think that part of the patch is ok. Although maybe it should be that the 'width' property shouldn't apply at all to svg:svg.

The patch also get's us passing the CDF "Rightsizing" tests: http://www.w3.org/2004/CDF/TestSuite/WICD_CDR_WP1/wicdcore.xhtml#core20 And the "Scalable Child" tests: http://www.w3.org/2004/CDF/TestSuite/WICD_CDR_WP1/wicdcore.xhtml#core-combined01

Comment on attachment 285739 [details] [diff] [review] patch addressing comments In nsLayoutUtils -- can you depend on both sides of the intrinsic ratio being nonnegative, rather than checking them? It seems like it would be better if we guaranteed that intrinsic ratios were always composed of nonnegative numbers. In nsLayoutUtils::ComputeSizeWithIntrinsicDimensions I'd think you could assert that aCBSize.width is not NS_UNCONSTRAINEDSIZE independently of any other conditions. Please get followup bugs filed on: * implementing DidSetStyleContext * any remaining issues about navigation between documents inside the <object> * a better fix for the box-sizing issues for SVG with padding/border/margin And don't forget to land all of the reftests along with the patch! r+sr=dbaron Sorry to take so long to get to the second pass of review here... and thanks for fixing all of this -- this looks like it makes a big improvement.

(In reply to comment #36) > Please get followup bugs filed on: > * implementing DidSetStyleContext > * any remaining issues about navigation between documents inside the <object> > * a better fix for the box-sizing issues for SVG with padding/border/margin ... and anything else I missed.

Checked in. Thanks for the review David. I'll be filing follow up bugs shortly. I'm going to leave the dependent bugs as they are for 24 hours or so until I'm sure the checkin will stick.

I can't recompile trunk after this checkin - http://pastebin.mozilla.org/244826 (including the bustage fix from JWatt)

The second attempt to fix the bustage did the trick. dbaron - the fix was to change nsLayoutUtils::ComputeSizeWithIntrinsicDimensions to have aIntrinsicSize be pass by value instead of pass by reference. I'm not sure why that's necessary on Mac and Linux, but I'm assuming it's okay to make that change.

jag filled me in on temporaries returned from functions being rvalues, meaning that to be passed to a function directly, the function parameter must be const. I've checked that fix in, so here's the code (minus reftests) that landed as a single patch.

The reftest counts on tinderbox didn't go up. Looks like you need to add an include of the reftest.list to the reftest.list in the parent directory?

Oops. I've turned them on now. Thanks.

(In reply to comment #36) > Please get followup bugs filed on: > * implementing DidSetStyleContext bug 404998 > * any remaining issues about navigation between documents inside the <object> bug 404999 > * a better fix for the box-sizing issues for SVG with padding/border/margin bug 400155