489924 - Embedded svg image inside a table overlaps with text in the neighbour column in some cases.

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Ryo Onodera]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.8) 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.

Comment 1

[Ryo Onodera]

Attachment: Test case

Comment 2

[Ryo Onodera]

Attachment: test case2

Comment 3

[Ryo Onodera]

Attachment: 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.

Comment 4

[Jonathan Watt [:jwatt]]

Hmm, I'm not sure about this.

Boris, David, shouldn't the table code be using the computed width for the <svg> here?

Comment 5

[Ryo Onodera]

This reftest calls nsSVGOuterSVGFrame::GetMinWidth():
layout/reftests/svg/inline-in-xul-basic-01.xul
This has been-known-fail.

Comment 6

[Ryo Onodera]

https://bug373960.bugzilla.mozilla.org/attachment.cgi?id=258573
Another test case which calls nsSVGOuterSVGFrame::GetMinWidth()

Comment 7

[Ryo Onodera]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Jonathan Watt [:jwatt]]

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.

Comment 10

[Jonathan Watt [:jwatt]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Jonathan Watt [:jwatt]]

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. :-)

Comment 13

[Ryo Onodera]

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?

Comment 14

[Jonathan Watt [:jwatt]]

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.

Comment 15

[Ryo Onodera]

Attachment: 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.

Comment 16

[Robert Longson [:longsonr]]

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.

Comment 17

[Ryo Onodera]

Attachment: 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.