Bug 90738 - Harmonize HTML & SVG rendering
Summary: Harmonize HTML & SVG rendering
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 110911 86022
Blocks: 23113
  Show dependency treegraph
 
Reported: 2012-07-08 13:43 PDT by Nikolas Zimmermann
Modified: 2019-10-02 01:43 PDT (History)
29 users (show)

See Also:


Attachments
Prototype patch (298.85 KB, patch)
2012-07-08 14:33 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Prototype patch v2 (303.55 KB, patch)
2012-07-12 04:55 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Prototype patch v3 (306.45 KB, patch)
2012-07-27 06:10 PDT, Nikolas Zimmermann
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Prototype patch v4 (306.83 KB, patch)
2012-08-23 08:12 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated against ToT. (310.84 KB, patch)
2012-10-03 15:49 PDT, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-07-08 13:43:48 PDT
This is the master bug tracking HTML & SVG harmonization.

In particular:
- RenderLayer & co should work for SVG
- z-index support for SVG
- 3D Transformations on any SVG element
- Move SVG rendering tree to use layers to handle transform/clipping/masking etc.
- SVG masking/clipper/filters on any RenderBox
- clipping via compositing operations (aka. reuse RenderBox code)
- paving the way for h/w acceleration
Comment 1 Nikolas Zimmermann 2012-07-08 14:33:08 PDT
Created attachment 151165 [details]
Prototype patch

This patch is a first prototype which implements the aforementioned features and integrates SVG rendering within HTML in a much more tight way.
All except 56 tests pass in svg/W3C-SVG-1.1-SE svg/W3C-SVG-1.2-Tiny svg/W3C-SVG-1.1 svg/W3C-I18N  svg/animations svg/as-background-image svg/as-border-image svg/as-image svg/as-object  svg/custom, which are the most important ones.

I'll tell more about this patch soon -- for tonight I just want to share it, so the others can have a look.
NOTE: This is still work in progress (with the patch as-is, 3d transforms don't work yet if a transform attr is set as well, stuff like that, but in general it does)
Comment 2 Nikolas Zimmermann 2012-07-08 14:36:13 PDT
(In reply to comment #0)
> In particular:
...
- Remove ALL of SVG specific transform support (aka. localToParentTransform(), custom mapLocalToContainer/pushMappingToAncestor/computeRectForRepaint methods)
- Make all renders inherit from RenderBox -- RenderSVGModelObject is a dead-end, and removed.
- Use CSS relative positioning internally to position shapes within containers (see moveChildren of RenderSVGResourceContainer)
- ... lots more I forgot during the work of the last weeks

The patch is not intended for any review yet, I'm rather looking for general comments if the direction is good at this point.
Comment 3 Rob Buis 2012-07-08 16:10:05 PDT
Comment on attachment 151165 [details]
Prototype patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151165&action=review

Looks great! I posted some questions, and I guess I did do a mini-review after all :)

> Source/WebCore/css/StyleResolver.cpp:2159
> +        bool isOuterSVGElement = e->hasTagName(SVGNames::svgTag) && e->parentNode() && !e->parentNode()->isSVGElement();

There may be an opportunity to share code, aka a free function, since I think SVGSVGElement needs the same thing(likely we have a method for this on SVGSVGElement).

> Source/WebCore/loader/cache/CachedImage.cpp:156
> +        return 0;

Is this a related or separate fix?

> Source/WebCore/rendering/RenderLayer.cpp:547
> +static inline bool isLayerAwareSVGRenderer(RenderObject* renderer)

Is it possible to use requiresLayer instead?

> Source/WebCore/rendering/RenderLayer.cpp:3005
> +            if (!useElement->hasAttribute(SVGNames::clip_ruleAttr))

Is this correct? Can it be that it is not there, but inherited from parent? (I forgot how that exactly should work).

> Source/WebCore/rendering/RenderLayer.cpp:3300
> +        }

Possibly can share more code here.

> Source/WebCore/rendering/RenderLayer.h:1000
> +    bool m_isPaintingSVGResourceLayer;

This will need to move into the bitfield above ultimately, since quite a few RenderLayers will be created in general :)

> Source/WebCore/rendering/RenderObject.cpp:756
> +            if (0 && o->isSVGForeignObject()) //foreignObject is the containing block for contents inside it

Ok <foreignObject> support is still TODO it seems :)

> Source/WebCore/rendering/RenderObject.cpp:2237
> +            if (0 && o->isSVGForeignObject()) // foreignObject is the containing block for contents inside it

Ditto.

> Source/WebCore/rendering/RenderObject.h:-469
> -    virtual const AffineTransform& localToParentTransform() const;

Great to see so much (SVG) stuff going from this class.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:139
> +    if (paintInfo.phase == PaintPhaseSVGClip) {

I think clipping is more common than applying masks.

> Source/WebCore/rendering/svg/RenderSVGResourceMarker.cpp:157
> +void RenderSVGResourceMarker::calculateViewport()

Maybe such changes should be in one commit "renaming abbrev methods to unabbrev".

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:176
> +        return resolveLengthAttributeForSVG(svg->intrinsicWidth(SVGSVGElement::IgnoreCSSProperties), 1, /*style()->effectiveZoom(),*/ containingBlock()->availableLogicalWidth(), view());

Is 1 temporary or the correct value here?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:210
> +        return resolveLengthAttributeForSVG(height, 1/*style()->effectiveZoom()*/, containingBlock()->availableLogicalHeight(), view());

Ditto.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:319
> +    }

What is the role of m_viewportContainer?

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:176
> +    // - When an outermost svg element is embedded inline within a parent XML grammar which uses CSS layout ([CSS2], chapter 9) or XSL formatting [XSL], if the âoverflowâ property

Unicode issue?

> Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:179
> +    // - When an outermost svg element is stand-alone or embedded inline within a parent XML grammar which does not use CSS layout or XSL formatting, the âoverflowâ property on

Ditto.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:287
> +/*

May need a clearer FIXME?

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:119
> +            if (0 && !masker->applyResource(m_object, style, m_context, ApplyToDefaultMode))

I was thinking, maybe #if 0 is clearer?

> Source/WebCore/svg/SVGTextContentElement.cpp:298
> +    return false;

Why does this change?
Comment 4 Simon Fraser (smfr) 2012-07-09 10:27:03 PDT
(In reply to comment #2)
> (In reply to comment #0)
> > In particular:
> ...
> - Remove ALL of SVG specific transform support (aka. localToParentTransform(), custom mapLocalToContainer/pushMappingToAncestor/computeRectForRepaint methods)

Sounds good.

> - Make all renders inherit from RenderBox -- RenderSVGModelObject is a dead-end, and removed.

I don't think this is right. RenderBox is geared towards rectangular CSS-rendered things. I think we need a common base class (bug 86022).

> - Use CSS relative positioning internally to position shapes within containers (see moveChildren of RenderSVGResourceContainer)
> - ... lots more I forgot during the work of the last weeks

Hmm. I think offsets should be in renderer x(), y(), not relative position offsets.
Comment 5 Nikolas Zimmermann 2012-07-10 05:26:19 PDT
Thanks for the first comments!

> > - Make all renders inherit from RenderBox -- RenderSVGModelObject is a dead-end, and removed.
> 
> I don't think this is right. RenderBox is geared towards rectangular CSS-rendered things. I think we need a common base class (bug 86022).
Fully agreed. I chose this approach to proof first that using layers & co for SVG works nicely.
I wanted to avoid refactoring RenderLayers off RenderBox as first step, until it's really clear that this works.

> > - Use CSS relative positioning internally to position shapes within containers (see moveChildren of RenderSVGResourceContainer)
> > - ... lots more I forgot during the work of the last weeks
> 
> Hmm. I think offsets should be in renderer x(), y(), not relative position offsets.
I tried both approaches. For constructions like <g> <rect> </g> I need the <g> to be a stacking-context, and I did so by forcing position: relative to the <g>, so that the <rect> will be rendered in the right coordinates space, if <g> has a transform. As the CSS left/top properties are passed through relativePositionOffset() when position: relative is set, I thought I'd do it the same.

<div style="position: relative; left: 100px; top: 75px; width: 100px; height: 200px"> yields a frameRect==borderBoxRect==(0,0,100,200), and all positioning is in relativePositionOffset() which returns (100,75).

I'm open to all suggestions, if you prefer the other approach I can easily change it -- I'd like to hear more about it, why this is better though.
Comment 6 Nikolas Zimmermann 2012-07-12 04:55:40 PDT
Created attachment 151910 [details]
Prototype patch v2

Updated against ToT. Individual changes:
- 3D Transforms on SVG elements work (as in my demo video)
- getScreenCTM() is fixed (all tests pass again)
- RenderSVGResourceGradient now longer manages a HashMap<RenderObject*,GradientData*> instead it holds a single RefPtr<Gradient>. We just have to update the gradientSpaceTransform for each client that wants to draw, instead of holding separated Gradient objects in a cache.
- RenderSVGShape is refactored based upons Philips work

Now looking forward to see the RenderLayer refactoring patch land, so i can base the SVG renders upon the new RenderBaseObject (or however it will be called).
Comment 7 Florin Malita 2012-07-12 10:34:48 PDT
I've got a patched CR build, and AFAICT FO layers are working fine now! All the outstanding issues from https://bugs.webkit.org/show_bug.cgi?id=23113 (SVG vs foreign content stacking, inheriting masks, etc.) seem to addressed. Nice work Niko!

Are you planning to set up a dedicated branch at some point?
Comment 8 Nikolas Zimmermann 2012-07-27 06:07:53 PDT
In reply to comment #7)
> Are you planning to set up a dedicated branch at some point?
Yes. Is anyone familiar with the process?

(In reply to comment #4)
> Hmm. I think offsets should be in renderer x(), y(), not relative position offsets.
Fixed. I reworked it to avoid position: relative - and the relativePositionOffset() overrides in rendering/svg.

You're right there was no need to go for the relative positioning. I'll post an updated patch now.
Comment 9 Nikolas Zimmermann 2012-07-27 06:10:25 PDT
Created attachment 154921 [details]
Prototype patch v3

Marking for r? to get builds results from other platforms as well. I again only included Source/WebCore, no LayoutTests/ changes, which are massive (all txt files changed containing layers).
Comment 10 WebKit Review Bot 2012-07-27 06:13:46 PDT
Attachment 154921 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Configurations/FeatureDefin..." exit_code: 1
Source/WebCore/rendering/svg/SVGRenderSupport.cpp:151:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/rendering/RenderLayerBacking.cpp:904:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayerBacking.cpp:905:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayerBacking.cpp:906:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayerBacking.cpp:907:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/rendering/RenderLayer.cpp:409:  Extra space before ) in if  [whitespace/parens] [5]
Source/WebCore/rendering/svg/RenderSVGRoot.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/rendering/RenderObject.cpp:747:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/rendering/svg/SVGRenderingContext.cpp:94:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/svg/SVGRenderingContext.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 10 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Gyuyoung Kim 2012-07-27 06:15:11 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13377315
Comment 12 Early Warning System Bot 2012-07-27 06:19:01 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13362661
Comment 13 Early Warning System Bot 2012-07-27 06:28:59 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13372367
Comment 14 Build Bot 2012-07-27 06:34:24 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13374361
Comment 15 WebKit Review Bot 2012-07-27 07:22:16 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13375389
Comment 16 Simon Fraser (smfr) 2012-07-27 10:27:47 PDT
Why is RenderBox painting anything SVG-related?
Comment 17 Dirk Schulze 2012-07-27 10:38:25 PDT
(In reply to comment #16)
> Why is RenderBox painting anything SVG-related?

Since HTML elements will render in SVG content without foreignObject in the future (and hopefully vice versa), it might not be the worst idea.
Comment 18 Build Bot 2012-07-27 16:41:50 PDT
Comment on attachment 154921 [details]
Prototype patch v3

Attachment 154921 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13376699
Comment 19 Dirk Schulze 2012-08-17 13:40:37 PDT
Comment on attachment 154921 [details]
Prototype patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=154921&action=review

Awesome code. Now I understand your problem with animated transform. Still, can we get this code into trunk this weekend? :P I still couldn't "review" the whole patch. It is a lot of stuff. But agree with some comments from Rob.

Looking at the changes again, I am not sure if we really need a new base class for SVG elements. If we have RenderBox as a common class, it seems easier to share code between HTML and SVG. Filters and Masks are just one part. Paint servers in general are the next step (SVG gradients and patterns on HTML content and CSS <image> on SVG). Even if the elements don't share the CSS Layout boxes with the HTML world. Or we can refactor the code more later.

> Source/WebCore/rendering/PaintPhase.h:54
> +    PaintPhaseMask,
> +    PaintPhaseSVGClip,
> +    PaintPhaseSVGMask

We may not need to differ between PaintPhaseMask and PaintPhaseSVGMask with CSS Masking [1]. Hopefully the same for clip-path :P.

[1] http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html

But maybe it is good to have it as intermediate step.

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +#if ENABLE(SVG) && ENABLE(FILTERS)
> +    GraphicsContext* svgSavedContext = 0;
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer());
> +    RenderSVGResourceFilter* svgFilter = resources ? resources->filter() : 0;
> +    if (svgFilter) {
> +        svgSavedContext = context;
> +        if (!svgFilter->applyResource(renderer(), renderer()->style(), context, ApplyToDefaultMode))
> +            return;
> +        // FIXME: We can optimize this further for the specific SVG filter operations.
> +        // We should either introduce a SVGFilterEffectRendererHelper class or makw FilterEffectRendererHelper aware of SVG. To be discussed.
> +        useClipRect = false;
> +    } else if (renderer()->hasSVGFilter())
> +        return;
> +#endif

Would this still be needed? We have SVG Filters on HTML content. We just need to make sure to create a layer. (Which is done in HTML right now anyway). Just want to say, no special handling of SVG Filters needed.

> Source/WebCore/rendering/RenderLayer.cpp:3318
> +#if ENABLE(SVG)
> +    if (!renderer()->isSVGResourceContainer()) {
> +        if ((localPaintFlags & PaintLayerPaintingCompositingSVGMaskPhase) && shouldPaintContent && renderer()->hasSVGMask() && !selectionOnly) {
> +            if (useClipRect)
> +                clipToRect(rootLayer, context, paintDirtyRect, damageRect, DoNotIncludeSelfForBorderRadius); // Mask painting will handle clipping to self.
> +
> +            // Paint the svg mask.
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(damageRect.rect()), PaintPhaseSVGMask, false, paintingRootForRenderer, region, 0);
> +            renderer()->paint(paintInfo, paintOffset);

This can/should be combined with 'mask-image' later. All that is needed is the correct calculation of the mask size + luminance mask.
Comment 20 Dirk Schulze 2012-08-17 13:41:30 PDT
Comment on attachment 154921 [details]
Prototype patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=154921&action=review

Awesome code. Now I understand your problem with animated transform. Still, can we get this code into trunk this weekend? :P I still couldn't "review" the whole patch. It is a lot of stuff. But agree with some comments from Rob.

Looking at the changes again, I am not sure if we really need a new base class for SVG elements. If we have RenderBox as a common class, it seems easier to share code between HTML and SVG. Filters and Masks are just one part. Paint servers in general are the next step (SVG gradients and patterns on HTML content and CSS <image> on SVG). Even if the elements don't share the CSS Layout boxes with the HTML world. Or we can refactor the code more later.

> Source/WebCore/rendering/PaintPhase.h:54
> +    PaintPhaseMask,
> +    PaintPhaseSVGClip,
> +    PaintPhaseSVGMask

We may not need to differ between PaintPhaseMask and PaintPhaseSVGMask with CSS Masking [1]. Hopefully the same for clip-path :P.

[1] http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html

But maybe it is good to have it as intermediate step.

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +#if ENABLE(SVG) && ENABLE(FILTERS)
> +    GraphicsContext* svgSavedContext = 0;
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer());
> +    RenderSVGResourceFilter* svgFilter = resources ? resources->filter() : 0;
> +    if (svgFilter) {
> +        svgSavedContext = context;
> +        if (!svgFilter->applyResource(renderer(), renderer()->style(), context, ApplyToDefaultMode))
> +            return;
> +        // FIXME: We can optimize this further for the specific SVG filter operations.
> +        // We should either introduce a SVGFilterEffectRendererHelper class or makw FilterEffectRendererHelper aware of SVG. To be discussed.
> +        useClipRect = false;
> +    } else if (renderer()->hasSVGFilter())
> +        return;
> +#endif

Would this still be needed? We have SVG Filters on HTML content. We just need to make sure to create a layer. (Which is done in HTML right now anyway). Just want to say, no special handling of SVG Filters needed.

> Source/WebCore/rendering/RenderLayer.cpp:3318
> +#if ENABLE(SVG)
> +    if (!renderer()->isSVGResourceContainer()) {
> +        if ((localPaintFlags & PaintLayerPaintingCompositingSVGMaskPhase) && shouldPaintContent && renderer()->hasSVGMask() && !selectionOnly) {
> +            if (useClipRect)
> +                clipToRect(rootLayer, context, paintDirtyRect, damageRect, DoNotIncludeSelfForBorderRadius); // Mask painting will handle clipping to self.
> +
> +            // Paint the svg mask.
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(damageRect.rect()), PaintPhaseSVGMask, false, paintingRootForRenderer, region, 0);
> +            renderer()->paint(paintInfo, paintOffset);

This can/should be combined with 'mask-image' later. All that is needed is the correct calculation of the mask size + luminance mask.
Comment 21 Dirk Schulze 2012-08-17 13:41:44 PDT
Comment on attachment 154921 [details]
Prototype patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=154921&action=review

Awesome code. Now I understand your problem with animated transform. Still, can we get this code into trunk this weekend? :P I still couldn't "review" the whole patch. It is a lot of stuff. But agree with some comments from Rob.

Looking at the changes again, I am not sure if we really need a new base class for SVG elements. If we have RenderBox as a common class, it seems easier to share code between HTML and SVG. Filters and Masks are just one part. Paint servers in general are the next step (SVG gradients and patterns on HTML content and CSS <image> on SVG). Even if the elements don't share the CSS Layout boxes with the HTML world. Or we can refactor the code more later.

> Source/WebCore/rendering/PaintPhase.h:54
> +    PaintPhaseMask,
> +    PaintPhaseSVGClip,
> +    PaintPhaseSVGMask

We may not need to differ between PaintPhaseMask and PaintPhaseSVGMask with CSS Masking [1]. Hopefully the same for clip-path :P.

[1] http://dvcs.w3.org/hg/FXTF/raw-file/tip/masking/index.html

But maybe it is good to have it as intermediate step.

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +#if ENABLE(SVG) && ENABLE(FILTERS)
> +    GraphicsContext* svgSavedContext = 0;
> +    SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(renderer());
> +    RenderSVGResourceFilter* svgFilter = resources ? resources->filter() : 0;
> +    if (svgFilter) {
> +        svgSavedContext = context;
> +        if (!svgFilter->applyResource(renderer(), renderer()->style(), context, ApplyToDefaultMode))
> +            return;
> +        // FIXME: We can optimize this further for the specific SVG filter operations.
> +        // We should either introduce a SVGFilterEffectRendererHelper class or makw FilterEffectRendererHelper aware of SVG. To be discussed.
> +        useClipRect = false;
> +    } else if (renderer()->hasSVGFilter())
> +        return;
> +#endif

Would this still be needed? We have SVG Filters on HTML content. We just need to make sure to create a layer. (Which is done in HTML right now anyway). Just want to say, no special handling of SVG Filters needed.

> Source/WebCore/rendering/RenderLayer.cpp:3318
> +#if ENABLE(SVG)
> +    if (!renderer()->isSVGResourceContainer()) {
> +        if ((localPaintFlags & PaintLayerPaintingCompositingSVGMaskPhase) && shouldPaintContent && renderer()->hasSVGMask() && !selectionOnly) {
> +            if (useClipRect)
> +                clipToRect(rootLayer, context, paintDirtyRect, damageRect, DoNotIncludeSelfForBorderRadius); // Mask painting will handle clipping to self.
> +
> +            // Paint the svg mask.
> +            PaintInfo paintInfo(context, pixelSnappedIntRect(damageRect.rect()), PaintPhaseSVGMask, false, paintingRootForRenderer, region, 0);
> +            renderer()->paint(paintInfo, paintOffset);

This can/should be combined with 'mask-image' later. All that is needed is the correct calculation of the mask size + luminance mask.
Comment 22 Dirk Schulze 2012-08-17 13:42:54 PDT
Sorry for spamming. The submit button did not react at all.
Comment 23 Nikolas Zimmermann 2012-08-22 06:41:23 PDT
Thanks for your review comments and apologizes for not replying to them. I was very busy with non-WebKit stuff the last weeks. Anyhow, going to rebase first to produce a new version w/o adressing your review comments, will do that afterwards.
Comment 24 Nikolas Zimmermann 2012-08-23 08:12:21 PDT
Created attachment 160166 [details]
Prototype patch v4

Patch v4 builds against ToT again, there are some new crashes that I couldn't yet fix (in svg/batik/text). I'm gone for the weekend, I hope to have those crashes resolved next week again.
Comment 25 Florin Malita 2012-10-03 15:49:21 PDT
Created attachment 166976 [details]
Updated against ToT.

Builds & seems to mostly work, but I noticed some positioning issues (for example the triangles in https://bugs.webkit.org/attachment.cgi?id=166247 are not perfectly aligned).
Comment 26 Nikolas Zimmermann 2012-10-04 03:38:27 PDT
(In reply to comment #25)
> Created an attachment (id=166976) [details]
> Updated against ToT.
> 
> Builds & seems to mostly work, but I noticed some positioning issues (for example the triangles in https://bugs.webkit.org/attachment.cgi?id=166247 are not perfectly aligned).

Phew, good to hear! Thanks Florin! I was planning to give the patch another shot on the weekend, as your RenderLayerModelObject patch has landed.
Some questions:

a) Did you do any changes except making it build again?
b) Did you run all pixel tests with it?
c) Shall we finally setup a branch for us?
Comment 27 Florin Malita 2012-10-04 06:02:14 PDT
(In reply to comment #26) 
> Phew, good to hear! Thanks Florin! I was planning to give the patch another shot on the weekend, as your RenderLayerModelObject patch has landed.

No problem, I figured that with RLMO in place we can start pushing this harder :) 

I'm still trying to grok the new approach, but let me know if you think of specific tasks I can help with. Some things I was planning to look at:

* refactoring stuff on top of RLMO
* failing tests/crashes
* performance


> a) Did you do any changes except making it build again?

No changes - just merging/api updates & Chromium build fixes.

> b) Did you run all pixel tests with it?

Not yet, planning to do that today.

> c) Shall we finally setup a branch for us?

Yes, please :) Can you do that, or do you know the right person to handle it?
Comment 28 Dirk Schulze 2012-10-04 07:11:32 PDT
Some more questions. Does it make sense to wait with the branch for the -WebKit-mask and -WebKit-clip-path patches? I would like to make them short hands for mask and clip-path properties until we unprefix. The second question is about the place where to apply masking and clip-path. At the moment the prefixed version require a new stacking context and The creation of a RenderLayer. Therefore, masking and clipping are done on the RenderLayer. In Niko's code masking and clipping are handled in RenderBox. Would it be ok to create a layer for SVG as well and continue with moving the code to RenderLayer?
Comment 29 Florin Malita 2012-10-04 10:36:18 PDT
(In reply to comment #27)
> > b) Did you run all pixel tests with it?
> 
> Not yet, planning to do that today.

To follow up on this:

* 47 crashes, most of them in the batik tests, mostly asserts
* ~500 image diffs - this sounds worse than it is: the majority of these are caused either by repaint rect changes or the small positioning variance (rounding/precision issues?) that I mentioned earlier

The are a few areas with consistent pixel failures that seem to indicate real problems:

- clipping and masking
- shadows
- dynamic updates
- zooming

(In reply to comment #28)
> Some more questions. Does it make sense to wait with the branch for the -WebKit-mask and -WebKit-clip-path patches?

We need to track ToT in this branch anyway, so your patches would be picked up regardless. And at some point someone still has to update/merge Niko's patch even if we defer branching - might as well do it on the branch where it can be reviewed :)
Comment 30 Dirk Schulze 2012-10-07 14:07:18 PDT
Comment on attachment 166976 [details]
Updated against ToT.

View in context: https://bugs.webkit.org/attachment.cgi?id=166976&action=review

> Source/WebCore/css/StyleResolver.cpp:2225
> +            TransformOperations transform = cssTransform;
> +            transform.operations().append(MatrixTransformOperation::create(localTransform.toTransformationMatrix()));
> +            style->setTransform(transform);
> +            style->setUnique();

Oh, never looked at this part before. This does not sound like a good idea. We loose all information about the individual operations on the transform attribute (at least in CSSOM). And does it mean that it overrides the property values?

CSS Transforms wants the transform attribute to be a presentation attribute. Therefore, the CSSOM is aware of all operations on the transform attribute. There are some small differences between the syntax on SVG and CSS. rotate() can have 3 arguments on SVG. There can be spaces between operation name and the opening function brace. I did some preparation on the CSS Parser that allows us to differ between CSS and SVG parsing mode inside of the CSS Parser. I also implemented rotate() with three arguments for CSS in a local build (just HW composited rendering does not work yet). Another Adobe employee investigated on the space issue between function names and opening braces.

I am not sure how to proceed with these patches yet, since they require transform to be a presentation attribute. Unless it is not a presentation attribute, it can't be tested and we don't land source code that is not testable.

On the other hand I did not investigate how the CSSOM information can be exposed to SVGDOM (I did the opposite from what you did). So a lot of JS scripts won't work after these changes. I would like to continue with the patch on the new branch and would welcome help on the SVGDOM side.
Comment 31 Nikolas Zimmermann 2012-10-21 07:56:46 PDT
(In reply to comment #30)
> > +            style->setTransform(transform);
> > +            style->setUnique();
> 
> Oh, never looked at this part before. This does not sound like a good idea. We loose all information about the individual operations on the transform attribute (at least in CSSOM). And does it mean that it overrides the property values?
Sure this needs to be fixed, I've done this to simplify things in the first patch - it doesn't matter to rendering.
Comment 32 Florin Malita 2013-02-26 14:29:32 PST
Resurrecting this fine idea...

I've been keeping an up-to date version of Niko's patch here: https://bugs.webkit.org/show_bug.cgi?id=90738. There was only one messy merge IIRC where I had to disable thorton's svg shadow fix, but other than that it should be pretty close to the original.

I've also tried with moderate success to re-implement just the SVG layers piece from scratch. In order to make it tractable, I think we need to split this into several focus areas:

* SVG layer support
* SVG resources applied as layers (depends on the above)
* various clean-ups (unrelated to any of the above)

If everyone still agrees that this is a good direction, I'll try pushing small incremental patches on these fronts.
Comment 33 Florin Malita 2013-02-26 14:30:22 PST
(In reply to comment #32)
> Resurrecting this fine idea...
> 
> I've been keeping an up-to date version of Niko's patch here: https://bugs.webkit.org/show_bug.cgi?id=90738. 

Err, that would be https://github.com/fmalita/webkit/compare/master...svg-refactor-niko-merge
Comment 34 Nikolas Zimmermann 2019-09-18 14:49:10 PDT
I am looking again into this topic, just wanted to let you know, in case someone is interested in this work.

IMHO this is one of the most important things to solve (besides text rendering), since we have many foreignObject related WPT bugs, z-index issues, etc. 

Since 2012 many things happened which will ease the job: subpixel support in layout, the RenderLayerModelObject introduction, etc.
Comment 35 Simon Fraser (smfr) 2019-09-19 02:59:48 PDT
A good start to fix foreignObject RenderLayer stuff would be to make a RenderFoo class that is shared between RenderView and RenderForeinObject that knows how to act as the root of a layer tree.

We might also use this for <dialog> overlay stuff.
Comment 36 Nikolas Zimmermann 2019-10-01 14:03:58 PDT
I have a working PoC patch, that implements RenderSVGModelObject on top of RenderLayerModelObject -- it greatly simplifies the whole SVG rendering code.

Just a few highlights:
- The SVG 'transform' attribute is now a CSS property, simplifying the whole renderer invalidation logic.
No more setNeedsShapeUpdate/setNeedsBoundariesUpdate/setNeedsTransformUpdate, etc.

Whenever a transform property is set on a SVG renderer a RenderLayer is created.

- SVG <foreignObject> is now trivial, since the coordinate system is identical between HTML / SVG.

- SMIL <animateMotion> is implemented as an internal CSS property (-internal-svg-supplemental-transform) and is treated just like the 'transform' property and applied in RenderLayer::updateTransform. This removes a few special cases, and simplifies the SMIL code.

I am currently working on RenderSVGTransformable/ViewportContainer - probably the transformations will be mapped to an internal CSS property, but I am not decided yet - still prototyping.

Also SVG markers/clippers/filters are currently disabled in my PoC patch - they need to be implemented from scratch (see the "Prototype patch v4" from 2012 - RenderSVGResourceLayer).

Stay tuned!
Comment 37 Dirk Schulze 2019-10-01 21:09:22 PDT
The syntax between transform attribute and transform property differs and needs to continue to do so. Just as an awareness note.
Comment 38 Dirk Schulze 2019-10-01 21:12:27 PDT
As another comment, that would need a lot of test cases with fo element, z-index (which should not be supported yet but maybe in the future), 3D transform, getScreebCTM, blend modes (which may break if you also create graphics layers since they would start isolating) and a lot more.
Comment 39 Nikolas Zimmermann 2019-10-02 01:41:11 PDT
(In reply to Dirk Schulze from comment #37)
> The syntax between transform attribute and transform property differs and
> needs to continue to do so. Just as an awareness note.

Fully aware of it, the intention of my current work is to outline the path to a modern SVG rendering engine, properly integrating with HTML.

That's why I called it a "Proof Of Concept" patch. It will also never be landable as-is, given the size, the amount of changes etc.

What I do not want to do is: introduce hacks to support e.g. a separated SVG transform attribute in the new concept, I want everything properly integrated with CSS -- we can then see what things need to change, in order to be SVG2 compilant, not break existing content, etc. But let's get rid of two decades old SVG specific concepts, that are not future proof :-)
Comment 40 Nikolas Zimmermann 2019-10-02 01:43:13 PDT
(In reply to Dirk Schulze from comment #38)
> As another comment, that would need a lot of test cases with fo element,
> z-index (which should not be supported yet but maybe in the future), 3D
> transform, getScreebCTM, blend modes (which may break if you also create
> graphics layers since they would start isolating) and a lot more.

Oh yes, and I will call for help in a while, to collect testcases that are specs compilant, and broken with our current implementation.

But first all basic features need to work again in the PoC branch, once that is done, I am happy to look at new testcases/features!

I will also think about a good approach to share the current work with others - in form of a git branch hosted somewhere...

Thanks for answering Dirk, highly appreciated that you look into this issue, too.