Implement nsSVGOuterSVGFrame::Get[Pref|Min]Width

RESOLVED FIXED

Status

()

Core
SVG
P3
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 251519 [details] [diff] [review]
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.
Attachment #251519 - Flags: review?(dbaron)
(Assignee)

Updated

11 years ago
Blocks: 275254

Updated

11 years ago
No longer blocks: 275254
(Assignee)

Updated

11 years ago
Blocks: 275254
(Assignee)

Comment 1

11 years ago
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".
Blocks: 300030

Comment 2

11 years ago
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]
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
Attachment #251519 - Flags: review?(dbaron) → review+
(Assignee)

Comment 4

11 years ago
Created attachment 251743 [details] [diff] [review]
patch with dbaron's comments addressed and SetParentCoordCtxProvider removed

Done. Additionally I removed the SetParentCoordCtxProvider calls since that call happens in nsSVGOuterSVGFrame::Init.
Attachment #251743 - Flags: superreview?(tor)
(Assignee)

Comment 5

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

Updated

11 years ago
Attachment #251743 - Flags: superreview?(tor) → superreview+
(Assignee)

Comment 6

11 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite?

Updated

11 years ago
Depends on: 367366

Comment 7

11 years ago
Backed out due to regressions.
Status: RESOLVED → REOPENED
Depends on: 367368
Resolution: FIXED → ---

Updated

11 years ago
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.

Updated

11 years ago
No longer depends on: 367366
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
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

11 years ago
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?
(Assignee)

Updated

11 years ago
Depends on: 367366
(Assignee)

Updated

11 years ago
No longer blocks: 275254, 288276
(Assignee)

Updated

11 years ago
Depends on: 294086

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Assignee)

Comment 14

10 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 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.