User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:188.8.131.52) Gecko/2009033100 Ubuntu/9.04 (jaunty) Firefox/3.0.8 Build Identifier: Embedded svg image inside a table is treated as if non-existent and breaks word-wrapping of text in the neighbour column when the <td> element containing the svg image is used for determining the width of the column. Well, I'll attach a test case. If that explanation doesn't make sense, view it. I'll recommend. Seeing is believing. Can this bug be a suitable one for me who wants to learn the inner workings of layout, while contributing to Mozilla for integrating svg into (x)html? Or, this elaborates into a fundamental problem in Gecko? Currently, I suspects that the communication between table and svg for querying minimum width isn't just functioning as this does for <img> elements. Reproducible: Always Steps to Reproduce: 1. Open following xhtml page. 2. Resize window narrower so that text wrapping happens. 3. Actual Results: Broken layout Expected Results: Shouldn't overlap.
Created attachment 374432 [details] [diff] [review] Patch v1 When we calculate minimum width, nsSVGOuterSVGFrame::GetMinWidth() is called from nsLayoutUtils::IntrinsicForContainer(), and it unconditionally returns nscoord(0) to that. I'm not confident about rather complicated calculation of intrinsic values for replaced elements specified by CSS, which is mentioned in this patch. But at least, we should behave better than now to fix this bug. Yet changing nsLayoutUtils::IntrinsicForContainer() is too risky for me, so I just copied and pasted the code from nsSVGOuterSVGFrame::GetPrefWidth(). Because this code is introduced by Jonathan Watt by https://bugzilla.mozilla.org/show_bug.cgi?id=294086, I ask you to review this. I know this is little mentioned in the bug report, but I don't have time to read it all, if you're unsure about this, please refer me to the appropriate person. Attached test cases are correctly shown now. And reftests in reftest/svg are OK. From now, I research exactly what other part of code in Mozilla calls nsSVGOuterSVGFrame::GetMinWidth. If I'm lucky, this patch will be found to fix other bugs.
Hmm, I'm not sure about this. Boris, David, shouldn't the table code be using the computed width for the <svg> here?
This reftest calls nsSVGOuterSVGFrame::GetMinWidth(): layout/reftests/svg/inline-in-xul-basic-01.xul This has been-known-fail.
https://bug373960.bugzilla.mozilla.org/attachment.cgi?id=258573 Another test case which calls nsSVGOuterSVGFrame::GetMinWidth()
It seems that this problem reaches to xul let alone html. The following test cases call nsSVGOuterSVGFrame::GetMinWidth(). I haven't checked this is the exact cause for each bug, though. Just hurry spotting. https://bug394078.bugzilla.mozilla.org/attachment.cgi?id=278734 http://www.croczilla.com/svg/samples/xulsvg1/xulsvg1.xul (from https://bugzilla.mozilla.org/show_bug.cgi?id=341446) https://bug422890.bugzilla.mozilla.org/attachment.cgi?id=309373 https://bug452031.bugzilla.mozilla.org/attachment.cgi?id=335325 https://bug244061.bugzilla.mozilla.org/attachment.cgi?id=148882
Jonathan, the table cell size would be based on the min width and pref width both. Since no column widths are specified, each column gets assigned its min width, and then any remaining available width is distributed amongst the columns proportionately to their preferred widths. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/BasicTableLayoutStrategy.cpp&rev=3.269&mark=648-694#647 That said, it does look to me like making GetMinWidth on SVG return the same thing that GetPrefWidth does would make the most sense. Is there a reason that wasn't done in bug 294086? I'd prefer a helper method instead of duplicating the code, though.
I think my only reason for making GetMinWidth return zero was because David said he thought that's what it should return in bug 294086 comment 21. Then again, I do remember changing GetMinWidth/GetPrefWidth many times as I realized different issues with making them return different values. I think it was GetPrefWidth mostly, but I can't remember for sure. So what should happen if the 'width' property is set instead of the 'width' attribute, remembering that the attribute defines an intrinsic width and is not simply mapped into style? Ryo: The comment should be changed to: // Our containing block's width depends on our width, so our behavior is // undefined according to CSS 2.1 section 10.3.2. We choose to return 0. If we're in GetMinWidth there's no "maybe" about it I'm told.
> So what should happen if the 'width' property is set instead of the 'width' > attribute What I mean is, in GetPrefWidth we use the 'width' attribute if it's not a percentage. Should we be returning the 'width' property it it's set, or does the layout system do the right thing in that case?
> bug 294086 comment 21. That seems to be talking about the |width.GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE case|, no? > does the layout system do the right thing in that case? It does. If your style information has a width, that'll get used. All this GetPref* code is needed for the case when the style width is auto.
Yes, you're right, it's about SVG_LENGTHTYPE_PERCENTAGE. Okay, Ryo, can you make a helper function that GetMinWidth and GetPrefWidth can both call? Also, in the 'width.IsPercentage()' check, can you check if the viewBox attribute is set, and if so, try and use its width (if it's something sane). That done, some reftests would be nice. :-)
OK. If we fall back to viewBox. how about even we fall back to root svg element's bounding box, if width attribute, viewBox attribute fails? Also, we must consider about transform attribute too?
No, we should only do viewBox, since the behavior here is defined by the SVG and CSS specs. Only the 'width' attribute and the width component of the 'viewBox' attribute is defined to provide an intrinsic width. I don't think the 'transform' attribute has anything to do with this, so I'm not sure what prompted your question there.
Created attachment 374760 [details] [diff] [review] Patch v2 I was wrong. Bounding box and 'transform' attribute don't matter. According to http://www.w3.org/TR/SVG11/attindex.html, clearly 'svg' element can't have 'transform'. I didn't know that. Bounding box is same, too. I've read appropriate parts of specs. Also, even if we decided to be kind to laze people, some bad side-effect would happen akin to HTML mess. Practically, many svg files exploit the implicit clipping effect by window frame. When I getBBox() the rootmost svg under such situation, I'll get unintended rect including originally-hidden-yet-now-poking paths and shapes. ;) Considering that the fallback of BBox only happens when authors don't specify even viewBox or width, it's very likely those files have such nature. Anyway, I've updated my patch as requested. Please review again.
Can you put numbers in the file names of the reftests please. Also we already fixed the viewBox code so the XXX needs redoing to call isValid on the viewBox.
Created attachment 374802 [details] [diff] [review] Patch v3 Fixed the two problem. According to https://bugzilla.mozilla.org/show_bug.cgi?id=435525#c8, IsValid() doesn't cover the possibility of negative values. So I still need to check? I'm not sure.