Bug 12207 – RenderSVGContainer should be split into multiple classes

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

RenderSVGContainer should be split into two classes, one for SVG and one for G

The current implementation is confusing at times and has lead to bugs like
http://bugs.webkit.org/show_bug.cgi?id=12201

[Rob Buis]

Created attachment 12397 [details]
First  attempt

Thi is not the final patch, and lots of test results will change (I guess all SVG results). I hope I get some feedback on the code and names I picked.
Cheers,

Rob.

[Eric Seidel (no email)]

We may need more than 2.

Here are all the SVG containers I can think of, grouped by behavior:

Have a viewBox and establish a viewPort:
<svg> - new SVGRenderRoot
<image> - RenderSVGImage
<foreignObject> - RenderSVGForeignObject

No viewbox, sometimes establishes a viewport:
<symbol> - only when referenced by <use>, the logic should probably be in the <use> renderer(s)

Has viewbox, but does not establish a viewport:
<pattern>
<marker>
Neither of these draw their content directly.

Has no viewport:
<defs> - does not draw its content directly, but probably should have a renderer to allow proper style resolution of its kids (right now it has no renderer)
<g> - should probably be RenderSVGContainer

<view> also has a viewBox, but it's never drawn, so we don't need a renderer for it.

I propose adding 3 new container types in this bug:

RenderSVGRoot - for <svg>, is a RenderBlock, only trickiness occurs when inside another <svg>
RenderSVGGroup - for <g>, only supports localTransform
RenderSVGUseable - for <symbol>, <pattern>, <marker>, viewbox is simply ignored for <symbol>, does not draw content via normal paint() method.

Optionally, you could also make <defs> a RenderSVGUsable, although then maybe it should be called RenderSVGHidden, not sure.

[Eric Seidel (no email)]

I forgot <mask> as well, that should also be part of the <symbol>, <pattern>, <marker> group, IMO.

[Eric Seidel (no email)]

Now that bug 12210, I should note that only the new RenderSVGRoot class will need to have the changes introduced in 12210.

[Maciej Stachowiak]

Comment on attachment 12397 [details]
First  attempt

r- for now based on Eric's comments

[Rob Buis]

(In reply to comment #4)
> Now that bug 12210, I should note that only the new RenderSVGRoot class will
> need to have the changes introduced in 12210.

I'll give this another go, on the feature_branch. If anybody was working on it already, let me know, we should coordinate it.
Cheers,

Rob.

[Eric Seidel (no email)]

Anyone working on this bug (i.e. rob) should be aware of the patch attached to:
http://bugs.webkit.org/show_bug.cgi?id=14012

That patch includes some small changes which try to make it obvious what sections of RenderSVGContainer already have special-case code for the <svg> root element.  The goal of splitting out a RenderSVGRoot class would be to remove those special cases from RenderSVGGroup (or RenderSVGContainer) so that RenderSVGContainer can only worry about being inside other SVG content, and RenderSVGRoot can worry about all the issues of mixing with other non-SVG content.

[Rob Buis]

(In reply to comment #7)
> Anyone working on this bug (i.e. rob) should be aware of the patch attached to:
> http://bugs.webkit.org/show_bug.cgi?id=14012

I am assuming your remark is about trunk. The work will go to feature_branch and there the patch seems to be applied, so no problem.
Cheers,

Rob.

[Rob Buis]

Created attachment 14987 [details]
Work in progress

This is just work in progress, a first split to RenderSVGRoot, no regressions.
Cheers,

Rob.

[Rob Buis]

Created attachment 14988 [details]
Split out RenderSVGRoot

A good first start.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 14988 [details]
Split out RenderSVGRoot

Eventually we'll need a better solution to this:

+        // FIXME: All this setup should be done after attributesChanged, not here.
+        rootContainer->setViewBox(viewBox());
+        rootContainer->setAlign(SVGPreserveAspectRatio::SVGPreserveAspectRatioType(preserveAspectRatio()->align()));
+        rootContainer->setSlice(preserveAspectRatio()->meetOrSlice() == SVGPreserveAspectRatio::SVG_MEETORSLICE_SLICE);

We'll need to find a way to abstract a bunch of this code so it's not so copy/paste.

It looks fine for a start.  We'll need follow up patches to clean up these newly split classes.

[Rob Buis]

Landed in r23509.