367031 - Implement nsSVGOuterSVGFrame::Get[Pref|Min]Width

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Jonathan Watt [:jwatt]]

Attachment: patch

Some SVG in XUL seems to be broken without nsSVGOuterSVGFrame::Get[Pref|Min]Width (my examples are in a private app ATM). Perhaps there are other methods introduced by the reflow branch that SVG should implement? At any rate, here's the code that gets things working for me. Unfortunately it doesn't fix bug 364732 and similar.

Comment 1

[Jonathan Watt [:jwatt]]

Actually this does fix the testcase attached to bug 364732. What it doesn't fix is the testcases linked in the comment, but that seems to be because their root most <svg> element doesn't have flex="1".

Comment 2

[Neil Deakin]

flex="1" shouldn't be necessary. Wherever the preferred (intrinsic) height of the svg content is being calculated seems to be ignored. Something in nsFrame::BoxReflow perhaps.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 251519 [details] [diff] [review]
patch

This seems ok for now, although I'd like to improve this behavior sometime before 1.9.  Could you cast to nscoord instead of int, and use a construction-style cast, though?  r=dbaron with that

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch with dbaron's comments addressed and SetParentCoordCtxProvider removed

Done. Additionally I removed the SetParentCoordCtxProvider calls since that call happens in nsSVGOuterSVGFrame::Init.

Comment 5

[Jonathan Watt [:jwatt]]

Neil, I'll look into that as soon as I can. Thanks for letting me know.

Comment 6

[Jonathan Watt [:jwatt]]

Checked in.

Comment 7

[tor]

Backed out due to regressions.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I wonder... When width is not specified for an <svg>, what is svg->mLengthAttributes[nsSVGSVGElement::WIDTH].GetAnimValue(this) ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Also, I should think we want to implement ComputeSize and fix nsLayoutUtils::ComputeSizeWithIntrinsicDimensions to work with an aspect ratio.  Or have a separate method for the case when all we have is an aspect ratio, more likely.

Comment 10

[Jonathan Watt [:jwatt]]

So the breakage was caused by the change to nsSVGSVGElement::SetParentCoordCtxProvider. When nsSVGOuterSVGFrame::InitSVG calls SetParentCoordCtxProvider and the width or height specified on the outer <svg> is specified as a percentage, then SetParentCoordCtxProvider ends up setting a bogus mViewBox on the nsSVGSVGElement. This is because SetCoordCtxRect hasn't been called yet, and as a result when SetParentCoordCtxProvider calls GetAnimValue(mCoordCtx) to set mViewBox, GetAnimValue returns a tiny value because it ends up here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGLength2.cpp&rev=1.1&mark=167#154

Removing the changes to nsSVGSVGElement::SetParentCoordCtxProvider seems to fix things, but it's really bad to call that every time we reflow. Also adding a SetParentCoordCtxProvider call to GetMinWidth and GetPrefWidth seems wrong since they're called before Reflow, and at least the first time round that means SetCoordCtxRect will not have been called.

Comment 11

[Jonathan Watt [:jwatt]]

When I say "really bad" I mean undesirable. I haven't noticed a perf hit.

Perhaps the way to go is to call it only if NS_FRAME_FIRST_REFLOW is set? And perhaps there is a reflow method that gets called before GetMinWidth, GetPrefWidth and Reflow where we can do it? Gotta catch a flight but I'll come back to this.

Comment 12

[[e-mail]]

Out of interest does SVG as a child of <xul:label> have the same problems?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> And perhaps there is a reflow method that gets called before GetMinWidth

Not really.  GetMinWidth and GetPrefWidth are called before the other reflow methods.

That said, why do you need layout information for GetMinWidth and GetPrefWidth?  If your size is specified in percentages, shouldn't that mean that your specified CSS width/height is in percent and your intrinsic size can be whatever (since it won't matter, given that you have a non-auto width/height specified)?

Or does this get us back to SVG not mapping the width/height into style?

Comment 14

[Jonathan Watt [:jwatt]]

This bug was just fixed by the fix for bug 294086. Many of the reftests checked
in for that bug cover this bug too I guess. Note reftests were also previously checked in for the regressions that the patch on this bug caused as part of the regression bugs.