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
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.
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.
I forgot <mask> as well, that should also be part of the <symbol>, <pattern>, <marker> group, IMO.
Now that bug 12210, I should note that only the new RenderSVGRoot class will need to have the changes introduced in 12210.
Comment on attachment 12397 [details] First attempt r- for now based on Eric's comments
(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.
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.
(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.
Created attachment 14987 [details] Work in progress This is just work in progress, a first split to RenderSVGRoot, no regressions. Cheers, Rob.
Created attachment 14988 [details] Split out RenderSVGRoot A good first start. Cheers, Rob.
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.
Landed in r23509.