Bug 15391 – SVGRenderContainer forces too many kids to relayout

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVGRenderContainer forces too many kids to relayout

SVGRenderContainer currently forces all kids to relayout whenever it has selfNeedsLayout() true.  This is set to true when the corresponding <g> element had its transform change OR when the nearest viewport changed bounds or a parent had a transform change.

SVGRenderContainer used to try and be smarter using SVGStyledElement::hasRelativeValues(), but when it became clear that changing the transform would also require forcing kids to relayout,  this "trick" no longer worked (since we can't differentiate currently between a "transform" inspired relayout or a "bounds" inspired relayout.

This bug is requesting that we make SVG renderers layout() methods smarter to avoid these unnecessary layouts of non-relative value elements.

[Eric Seidel (no email)]

FYI, the "hasRelativeValues()" (broken) optimization was removed as part of fixing bug 15388.

[Eric Seidel (no email)]

Because all renderers cache their absolute bounds, any time your bounds origin, or transform (or a parent's bounds origin or transform) changes *all* kids need to relayout.

However, any time just bounds size changes (or parents bounds size changes) only relative-sized kids need to relayout.  (The parent who's bounds size changed will invalidate the proper region for repaint.)

After fixing our localTransform handling to make the renders pull (soon to land), we now know when the local transform has changed, but that information isn't actually very useful unless we stash it away in the LayoutState so that our container kids can use it to decide what type of layout to do.

[Nikolas Zimmermann]

Finally working on this issue! First results are very promising, saves a lot of unneeded layouts.

[Nikolas Zimmermann]

Created attachment 60535 [details]
Initial patch

Not marked for review yet, missing a changelog so far.

[Nikolas Zimmermann]

Created attachment 60547 [details]
Updated patch

This time including a ChangeLog, have fun! :-) Be sure to try Lively Kernel / animation examples once this lands.

[Dirk Schulze]

(In reply to comment #5)
> Created an attachment (id=60547) [details]
> Updated patch
> 
> This time including a ChangeLog, have fun! :-) Be sure to try Lively Kernel / animation examples once this lands.

Great Patch Niko. Just a question about the LayoutTest. What causes the change? SVGRoot becomes 15px bigger, but I don't see the reason?!?

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=60547) [details] [details]
> > Updated patch
> > 
> > This time including a ChangeLog, have fun! :-) Be sure to try Lively Kernel / animation examples once this lands.
> 
> Great Patch Niko. Just a question about the LayoutTest. What causes the change? SVGRoot becomes 15px bigger, but I don't see the reason?!?

I don't know why it had the wrong size before. If you look at the enclosing box, the new width is the right one. Not sure why it was wrong before - must be related to some premature optimizations, that calcViewport() was not called, when selfNeedsLayout was false (when not using relative lengths). Something like that, anyhow it's a progression.

[Dirk Schulze]

Comment on attachment 60547 [details]
Updated patch

Great patch! We discussed the failing test on IRC. Please add the explaination to the changelog as well. r=me.

[Nikolas Zimmermann]

Landed in r62531.