Closed Bug 367031 Opened 13 years ago Closed 13 years ago

Implement nsSVGOuterSVGFrame::Get[Pref|Min]Width


(Core :: SVG, defect, P3)






(Reporter: jwatt, Assigned: jwatt)




(2 files)

Attached patch patchSplinter Review
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.
Attachment #251519 - Flags: review?(dbaron)
Blocks: 275254
No longer blocks: 275254
Blocks: 275254
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".
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 on attachment 251519 [details] [diff] [review]

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
Attachment #251519 - Flags: review?(dbaron) → review+
Done. Additionally I removed the SetParentCoordCtxProvider calls since that call happens in nsSVGOuterSVGFrame::Init.
Attachment #251743 - Flags: superreview?(tor)
Neil, I'll look into that as soon as I can. Thanks for letting me know.
Attachment #251743 - Flags: superreview?(tor) → superreview+
Checked in.
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Depends on: 367366
Backed out due to regressions.
Depends on: 367368
Resolution: FIXED → ---
Depends on: 367352
So I wonder... When width is not specified for an <svg>, what is svg->mLengthAttributes[nsSVGSVGElement::WIDTH].GetAnimValue(this) ?
Blocks: 288276
Flags: blocking1.9?
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.
No longer depends on: 367366
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:

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.
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.
Out of interest does SVG as a child of <xul:label> have the same problems?
> 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?
Depends on: 367366
No longer blocks: 275254, 288276
Depends on: 294086
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
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.
Closed: 13 years ago13 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.