Closed Bug 369850 Opened 13 years ago Closed 12 years ago

SVG sometimes fails to render when vertically resized

Categories

(Core :: SVG, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files)

Attached image testcase
When SVG content is resized it flickers badly and then frequently fails to render. Loading the attached testcase I noticed that there are no flickering or rendering problems when only the width of the browser window is changed. The problem always shows when the bottom edge of the browser window is dragged to resize the height of the window.

Looking back through the archives I found the build directly before the landing of the reflow branch works fine, but the build directly after it has this regression.
Flags: blocking1.9?
On the problem resizes, nsSVGOuterSVGFrame::Reflow is doing an early return (!ShouldReflowAllKids).
Blocks: 374293
See bug 374293 comment 5 for some more interesting info.
Flags: blocking1.9? → blocking1.9+
In the ShouldReflowAllKids() call, what are the various flags?  In particular, what is mFlags.mVResize?  What's the computed height in the reflow state?

Basically, is this a consequence of not actually mapping SVG height/width properties into style, such that we have an unconstrained computed height in the reflow state when we get into this method?
In the resize that doesn't work, the mHResize and mVResize flags are zero.  In the working resize, they are both set.  In both cases mComputedHeight is unconstrained.
> In the resize that doesn't work, the mHResize and mVResize flags are zero.

Right.  That would do it.

> In both cases mComputedHeight is unconstrained.

Sounds like someone should be implementing ComputeSize...  Then again, that's come up before.
Not sure if this is a legitimate step forward, or just hacking around the problem.
Attachment #276188 - Flags: review?(bzbarsky)
I'm not sure I follow this patch.  I seem to recall jwatt giving reasons this couldn't be immediately done (things like percentages are not actually percentages of the ancestor box, as in CSS), no?
Hmm, there is the aspect ratio of the viewBox that we consider when calculating the viewport matrix and axis lengths, but for space allocation of the outer svg I think we can ignore this.  jwatt?
Space allocation of the outer SVG should use the aspect ratio if it's got a single specified dimension and auto other one.  Your svg.css patch guarantees that that can't work, since both are always specified.
So I have to be honest.  I have no idea when I'll have time to sit down and try to figure out whether this patch in fact makes things better or not.  My gut instinct is that this is not what we want to do, based on past conversations with jwatt but I simply don't remember the details (either of what the spec calls for or how the svg reflow works).
Attachment #276188 - Flags: review?(bzbarsky)
As I understand it, when the SVG is viewed standalone the 'width' and 'height' attributes are used to size it, so yeah, mapping into style would be fine in this scenario. The problem is SVG embedded by reference or embedded inline. In these scenarios the SVG should be treated as a "replaced element" and the 'width' and 'height' attributes specify "intrinsic" dimensions (or the viewBox width/height does if one of 'width' or 'height' has a percentage value), and so the attributes cannot be mapped into style (as Boris says). I'm actually working on this right now and hope to have wiki docs, a bunch of reftests and a patch by Friday that will fix at least:

https://bugzilla.mozilla.org/buglist.cgi?bug_id=280923,288276,294086,342532,367031
No longer blocks: 374293
Depends on: 294086
So the behavior on trunk builds has changed somehow. Now it's no longer the case that the entire SVG fails to render on every second reflow caused by vertical resizing of the window. The problem now is that when the window is resized vertically to make it _taller_, nsSVGOuterSVGFrame::Reflow returns early because |!aReflowState.ShouldReflowAllKids()| is true. When the window is resized horizontally or resized vertically to make the window _shorter_, this condition is not true, so nsSVGOuterSVGFrame::Reflow completes and we repaint correctly.
It seems the same sort of symptoms can be seen for some SVG when the window is resized _horizontally_. In this case though the reflow methods on nsSVGOuterSVGFrame aren't being called _at all_.

To see this in the attached testcase, first resize the window to make it narrower. In this scenario the SVG gets its reflow and it resizes correctly. Now make the window wider. The SVG does not get its reflow, even though its width depends on the width of its containing block which is getting wider. Hence it is never repainted and seems stuck at its old size.
The change in behavior was caused by Boris' checkin for bug 380516.
Assignee: general → jwatt
I can fix the vertical resizing issue by putting the following at the top of nsSVGOuterSVGFrame::Reflow:

    AddStateBits(NS_FRAME_CONTAINS_RELATIVE_HEIGHT);

However, for the horizontal problem I need to hack IsPercentageAware to make it aware of SVG.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineLayout.cpp&rev=3.274&mark=718#716

I can see two direct ways of doing this. I can go through the XPCOM DOM APIs to get the SVGSVGElement, get its SVGAnimatedLength and thus get that's SVGLength to see if we have a percentage width. Alternatively I can try and get to our internal class nsSVGSVGElement to save the XPCOM overhead. The only problem with this is that it requires I add a bunch of exports to the SVG Makefiles so the include dependencies can be found by nsLineLayout.cpp. Specifically:

mozilla/content/svg/content/src/Makefile.in:
+       nsSVGSVGElement.h          \
+       nsSVGStylableElement.h     \
+       nsSVGElement.h             \
+       nsSVGClassValue.h          \
+       nsSVGValue.h               \
+       nsSVGLength.h              \
+       nsSVGEnum.h                \

mozilla/layout/svg/base/src/Makefile.in
+       nsSVGOuterSVGFrame.h \
+       nsSVGContainerFrame.h \
+       nsISVGChildFrame.h \
+       nsISVGSVGFrame.h \

And of course as soon as someone adds an include to nsSVGOuterSVGFrame.h or one of it's deps the build likely to break in nsLineLayout.cpp.

Does anyone have a preference/alternative?
On, and of course we need to implement nsSVGOuterSVGFrame::ComputeSize as Boris said, which is in bug 294086.
Create a helper function in nsSVGUtils and use that from nsLineLayout?
Wouldn't the CONTAINS_RELATIVE_HEIGHT need to be on the ancestors of the SVG frame, not the SVG frame itself?

In any case, it seems like the right way to do this would be to teach InitResizeFlags() about replaced element frames with intrincic ratios and possibly percentage intrinsic heights or something, no?
Mmm, I was tired last night. An nsSVGUtils helper would work of course, but as Boris hints I'm already adding methods to nsIFrame that give me the information I need. Doh!

I did try to teach InitResizeFlags about SVG but decided against at the time due to the same header problem.
(In reply to comment #19)
> Wouldn't the CONTAINS_RELATIVE_HEIGHT need to be on the ancestors of the SVG
> frame, not the SVG frame itself?

On fixing up InitResizeFlags, CONTAINS_RELATIVE_HEIGHT is set correctly on the ancestors of the SVG and (with the patch from bug 294086) mHResize and mVResize are also set correctly. However, ShouldReflowAllKids still returns false. If CONTAINS_RELATIVE_HEIGHT is not supposed to be set on the SVG frame itself then it seems someone should be setting NS_FRAME_IS_DIRTY on the SVG frame but isn't.
Actually, in the new patch from bug 294086 I've just removed the ShouldReflowAllKids check since it's redundant. The only expensive operation we need to return early for is the NotifyViewportChange() call, and we can easily and cheaply check for that.
> However, ShouldReflowAllKids still returns false. 

Which is fine.  It should return false.

That said, the optimization in nsSVGOuterSVGFrame::Reflow is just wrong.  If !ShouldReflowAllKids it's ok to skip reflowing the kids, but _we_ should still get reflown.  In particular, mRect may no longer be the right size.  Further, we should reflow any kids that are dirty subtrees, even if aReflowState.ShouldReflowAllKids() is dirty.

Removing that stuff altogether seems fine by me.
OS: Windows XP → All
Hardware: PC → All
Priority: -- → P1
This bug was just fixed by the fix for bug 294086. The layout/reftests/svg/sizing/dynamic--inline-resize-... reftests checked in for bug 294086 cover this bug too.

 
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.