Bug 87373 – Support CSS Transitions/Animations on Nodes without a RenderObject

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CSS

[Nikolas Zimmermann]

Support CSS Transitions/Animations on Nodes without a RenderObject.

My goal is to remove the RenderSVGGradientStop renderer, created for a SVGStopElement.
We don't really need the renderer, except to track style updates / support CSS Transitions/Animations on stop-color/stop-opacity.

I've prototyped a patch which adds additional support for CSS Transitions/Animations on render-less Nodes.
It works just fine -- if that gets landed, bug 86941 can be easily done.

[Nikolas Zimmermann]

If you check out the patch at bug 86941 - you'll see that removing RenderSVGGradientStop breaks CSS Transitions/Animations. When altering SVGStopElement::setRenderStyle() like this:

void SVGStopElement::setRenderStyle(PassRefPtr<RenderStyle> newStyle)
{
    Frame* frame = document()->frame();
    if (frame && (newStyle->hasTransitions() || newStyle->hasAnimations()))
        m_style = frame->animation()->updateAnimations(this, m_style.get(), newStyle.get());
    else
        m_style = newStyle;

    if (RenderObject* renderer = parentNode() ? parentNode()->renderer() : 0)
        RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer);
}

CSS Animations/Transitions work again, just without depending on a renderer.
I hope the solution is acceptable - this would move us forward in refactoring the RenderSVGResource* handling (gradients/patterns/clipPath should all have no renderer in future).

[Nikolas Zimmermann]

Created attachment 143790 [details]
Patch

[Early Warning System Bot]

Comment on attachment 143790 [details]
Patch

Attachment 143790 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12803131

[Early Warning System Bot]

Comment on attachment 143790 [details]
Patch

Attachment 143790 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12791183

[Build Bot]

Comment on attachment 143790 [details]
Patch

Attachment 143790 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12791186

[WebKit Review Bot]

Comment on attachment 143790 [details]
Patch

Attachment 143790 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12800177

[Gyuyoung Kim]

Comment on attachment 143790 [details]
Patch

Attachment 143790 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12798186

[Simon Fraser (smfr)]

Comment on attachment 143790 [details]
Patch

We need a solution for transitions on pseudo elements if we do this.

[Nikolas Zimmermann]

(In reply to comment #8)
> (From update of attachment 143790 [details])
> We need a solution for transitions on pseudo elements if we do this.
Ah good point - I'll look into that.

[Nikolas Zimmermann]

(In reply to comment #8)
> (From update of attachment 143790 [details])
> We need a solution for transitions on pseudo elements if we do this.

I thought again about this. For SVG we intend to use this only on non-rendered Nodes, like <stop>, <linearGradient> etc. What kind of pseudo state could be supported there? (The obvious first-letter, first-line, etc. pseudo classes don't apply here).

I think we could use this as-is for <stop> & friends, without having to worry about pseudo states.
Please correct me if I'm wrong.

[Nikolas Zimmermann]

Created attachment 144036 [details]
Patch v2

[WebKit Review Bot]

Comment on attachment 144036 [details]
Patch v2

Attachment 144036 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12795500

[Nikolas Zimmermann]

Created attachment 144052 [details]
Patch v3

[Simon Fraser (smfr)]

Right, but what about CSS pseudo-elements like :before, :after etc. A node may have more than one renderer associated with it, and you have to be able to animate those separately.

[Nikolas Zimmermann]

(In reply to comment #14)
> Right, but what about CSS pseudo-elements like :before, :after etc. A node may have more than one renderer associated with it, and you have to be able to animate those separately.
I'm not sure, we never tested generated content for SVG. Generated content is explicitly disallowed for the SVG <text> subtree. SVG itself doesn't say a word about this.
I'm not sure if it works at all in trunk for SVG?

[Simon Fraser (smfr)]

I'm thinking about HTML. You can't break AnimationController for HTML.

[Nikolas Zimmermann]

(In reply to comment #16)
> I'm thinking about HTML. You can't break AnimationController for HTML.
Wait, HTML isn't affected at all by this patch.
RenderObject::setStyle style calls updateAnimations() with a RenderObject*.
There's no caller yet which passes a Node* to updateAnimations() which is required for the new code-path to be taken.

In my local kill-RenderSVGGradientStop branch I'm using this already.

[Simon Fraser (smfr)]

(In reply to comment #17)
> (In reply to comment #16)
> > I'm thinking about HTML. You can't break AnimationController for HTML.
> Wait, HTML isn't affected at all by this patch.

Maybe not, but you're changing AnimationBase to be per-node, rather than per-renderer, so it has the potential to affect HTML, and if we animated pseudo-elements, this would break that.

[Nikolas Zimmermann]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > I'm thinking about HTML. You can't break AnimationController for HTML.
> > Wait, HTML isn't affected at all by this patch.
> 
> Maybe not, but you're changing AnimationBase to be per-node, rather than per-renderer, so it has the potential to affect HTML, and if we animated pseudo-elements, this would break that.

Currently there's no difference, as we don't support animating generated content (where renderer->node() is null). I could check AnimationBase to take both a RenderObject* and a Node* - but that doesn't make much sense at the moment unless we had support for animating generated content, which we don't have.

[Igor Trindade Oliveira]

first letter(pseudo element) initial work: https://bugs.webkit.org/show_bug.cgi?id=85253

I am waiting for this bug to finish the patch. In this case render->node is null.

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > I'm thinking about HTML. You can't break AnimationController for HTML.
> > > Wait, HTML isn't affected at all by this patch.
> > 
> > Maybe not, but you're changing AnimationBase to be per-node, rather than per-renderer, so it has the potential to affect HTML, and if we animated pseudo-elements, this would break that.
> 
> Currently there's no difference, as we don't support animating generated content (where renderer->node() is null). I could check AnimationBase to take both a RenderObject* and a Node* - but that doesn't make much sense at the moment unless we had support for animating generated content, which we don't have.

[Nikolas Zimmermann]

(In reply to comment #20)
> first letter(pseudo element) initial work: https://bugs.webkit.org/show_bug.cgi?id=85253
> 
> I am waiting for this bug to finish the patch. In this case render->node is null.
Oh okay, bad timing for my patch it seems.
I had a quick glance over it, and I think my approach from this bug report can still work together with generated content, if we pass around the generatingNode(). Needs testing of course.

So Simon, how do we proceed from here?
a) Get this landed as-is. Let Igor do the integration work with it.
b) Let Igos patch go in first, then revisit this patch.

What would you prefer? Am I fully on the wrong track for this patch?

I really only care about being able to animate render-less Nodes for SVG, not more. But I'll be happy to do any work that helps me to get there :-)

[Stephen Chenney]

Could we get some action on this please? It is blocking 86941 which is related to a top chrome crasher. We can workaround it in the short term, but it seems better to avoid such hackish temp patches.

[Eric Seidel (no email)]

Comment on attachment 144052 [details]
Patch v3

I believe Antti to be your best reviewer here.

[Antti Koivisto]

This is more of Simon's territory.

[Nikolas Zimmermann]

(In reply to comment #24)
> This is more of Simon's territory.
I'm not so sure, whether this patch is still good/valid.
It'll complicate things a lot, if we ever want to support transitions/animations on generated content (which I heard is sth. on the agenda).

[Simon Fraser (smfr)]

We should also think about future directions for animatinos, fore example using the same animation engine for SVG and CSS, and being able to animate properties/attributes.

[Dirk Schulze]

Comment on attachment 144052 [details]
Patch v3

It sound this bug is not ready for review anymore. removing review flag.

[Dirk Schulze]

Time passed by and I am not sure if we still want to follow this approach. Closing the bug now.