Apply animations and transitions for first-letter element

[Igor Trindade Oliveira]

apply animations and transitions for first-letter element

[Igor Trindade Oliveira]

Created attachment 139564 [details]
Patch V1.

Proposed patch V1.

[Igor Trindade Oliveira]

Let's broke the animation generated content patch in small patches, each generated element has a different behavior when animated.
The bug https://bugs.webkit.org/show_bug.cgi?id=23209 will be the meta bug for the generated content animations bug.

[Igor Trindade Oliveira]

Created attachment 139573 [details]
Patch V2.

Improve changelog.

[Simon Fraser (smfr)]

*** This bug has been marked as a duplicate of bug 23209 ***

[Simon Fraser (smfr)]

Comment on attachment 139573 [details]
Patch V2.

I didn't look at this patch, but please fix this under bug 23209 (which already has a WIP patch).

[Igor Trindade Oliveira]

Reopening the bug, now it blocks the bug #85253.

[Igor Trindade Oliveira]

Created attachment 139687 [details]
Patch V3

Proposed patch.

[Simon Fraser (smfr)]

Comment on attachment 139687 [details]
Patch V3

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

> LayoutTests/ChangeLog:11
> +        * animations/first-letter-animation-expected.txt: Added.
> +        * animations/first-letter-animation.html: Added.
> +        * transitions/first-letter-color-transition-expected.txt: Added.
> +        * transitions/first-letter-color-transition.html: Added.

I think you should also test:
* transition on both an element and its pseudoelement, of the same and different properties
* animation on an element and its pseudoelement, to check that they can be controlled independently.

> Source/WebCore/rendering/RenderBlock.cpp:5975
> +    RefPtr<RenderStyle> temporaryStyle = RenderStyle::create();
> +    temporaryStyle->inheritFrom(firstLetterBlock->style());
> +    firstLetter->setStyle(temporaryStyle);  
>      firstLetterContainer->addChild(firstLetter, currentChild);
> +    
> +    RefPtr<RenderStyle> pseudoElementStyle = animation()->updateAnimations(firstLetter, pseudoStyle);
> +    firstLetter->setStyle(pseudoElementStyle);

It seems odd to call updateAnimaitons explicitly here. I think this logic should be wrapped up in a RenderObject method that we can possibly use elsewhere.

[Igor Trindade Oliveira]

Created attachment 140139 [details]
Patch V4.

Updated patch.

[Simon Fraser (smfr)]

Comment on attachment 140139 [details]
Patch V4.

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

Tests still need to be improved.

> LayoutTests/animations/first-letter-animation.html:13
> +    .box {
> +      position: relative;
> +      left: 400px;
> +      top: 100px;
> +      width: 100px;
> +      height: 100px;
> +      background: blue;
> +      -webkit-animation: boxAnim 1s linear;

A one second test is way too slow. Can this test be changed to use the pauseAPI? (though that might need fixing for pseudos).

> LayoutTests/animations/first-letter-animation.html:29
> +    @-webkit-keyframes firstLetterAnim {
> +      from { margin: 0px; }
> +      to { margin: 100px; }
> +    }
> +
> +    @-webkit-keyframes boxAnim {
> +      from { -webkit-transform: scale(1); }
> +      to { -webkit-transform: scale(3); }

The interesting case is the same property being animated (in different ways) on the element and the pseudo, but you're not testing that here.

You should also test that setting animation-play-state affects the correct animation.

> LayoutTests/transitions/first-letter-color-transition.html:15
> +      -webkit-transition: color 1s;

Ditto.

> LayoutTests/transitions/first-letter-transition.html:23
> +      -webkit-transition: color 1s;
> +    }
> +
> +    #first {
> +      -webkit-transition: left 1s;
> +    }
> +
> +    #second {
> +      -webkit-transition: color 1s;

This test is also way too slow. Can it use the transition-test-helpers.js?

You also need to test a transition of the same property on the element and the pseudo, preferably where the transitions begin at different times.

[Igor Trindade Oliveira]

(In reply to comment #10)
> (From update of attachment 140139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140139&action=review
> 
> Tests still need to be improved.
> 
> > LayoutTests/animations/first-letter-animation.html:13
> > +    .box {
> > +      position: relative;
> > +      left: 400px;
> > +      top: 100px;
> > +      width: 100px;
> > +      height: 100px;
> > +      background: blue;
> > +      -webkit-animation: boxAnim 1s linear;
> 
> A one second test is way too slow. Can this test be changed to use the pauseAPI? (though that might need fixing for pseudos).
> 

Normally pauseAPI is implemented using Document::getElementById to get the
Node information, however pseudo elements do not have Node.

> > LayoutTests/animations/first-letter-animation.html:29
> > +    @-webkit-keyframes firstLetterAnim {
> > +      from { margin: 0px; }
> > +      to { margin: 100px; }
> > +    }
> > +
> > +    @-webkit-keyframes boxAnim {
> > +      from { -webkit-transform: scale(1); }
> > +      to { -webkit-transform: scale(3); }
> 
> The interesting case is the same property being animated (in different ways) on the element and the pseudo, but you're not testing that here.
> 

Ok

> You should also test that setting animation-play-state affects the correct animation.
> 

Sure.

> > LayoutTests/transitions/first-letter-color-transition.html:15
> > +      -webkit-transition: color 1s;
> 
> Ditto.
> 
> > LayoutTests/transitions/first-letter-transition.html:23
> > +      -webkit-transition: color 1s;
> > +    }
> > +
> > +    #first {
> > +      -webkit-transition: left 1s;
> > +    }
> > +
> > +    #second {
> > +      -webkit-transition: color 1s;
> 
> This test is also way too slow. Can it use the transition-test-helpers.js?

transition-test-helpers.js and also animation helpers use getElementById and it does not work with pseudo elements.


> 
> You also need to test a transition of the same property on the element and the pseudo, preferably where the transitions begin at different times.

Ok.

[Igor Trindade Oliveira]

Created attachment 144960 [details]
Patch V5.

Proposed patch.

[Simon Fraser (smfr)]

Comment on attachment 144960 [details]
Patch V5.

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

r- pending answers to my questions.

> LayoutTests/animations/first-letter-animation.html:11
> +      -webkit-animation: boxAnimation 0.5s 0.2s linear;

0.5s is still pretty long for one test. Imagine 100 of these; you've eaten 50s of every developer's time who runs tests.

> LayoutTests/animations/first-letter-animation.html:28
> +           }

Style rules would suggest this paren be aligned with the 'from'.

> LayoutTests/animations/first-letter-play-state.html:5
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
> +<html lang="en">
> +<head>
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +  <title>Test of -webkit-animation-play-state</title>

Could be simplified at match the other test.

> Source/WebCore/ChangeLog:8
> +        Instead of call RenderOject::node() in the animations code,

"of calling"

> Source/WebCore/rendering/RenderBlock.cpp:6020
> -    RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock, firstLetterContainer);
> +    RefPtr<RenderStyle> pseudoStyle = animation()->updateAnimations(firstLetter, styleForFirstLetter(firstLetterBlock, firstLetterContainer));

Why do we have to call updateAnimations() explicitly in this case, but in createFirstLetterRenderer() we can rely on setAnimatableStyle() to call updateAnimations() for us?

> Source/WebCore/rendering/RenderObject.cpp:2227
> +Node* RenderObject::generatingNode() const
> +{
> +    Node* node = m_node == document() ? 0 : m_node;
> +    if (node)
> +        return node;
> +
> +    for (RenderObject* object = parent(); object; object = object->parent()) {
> +        if (Node* node = (object->node() == object->document() ? 0 : object->node()))
> +            return node;
> +    }
> +    return 0;
> +}

Does this behavior change affect other calls to generatingNode(), like the counter code?

[Igor Trindade Oliveira]

Created attachment 145358 [details]
Patch V6.

Proposed patch.

[Igor Trindade Oliveira]

(In reply to comment #13)
> (From update of attachment 144960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144960&action=review
> 
> r- pending answers to my questions.
> 
> > LayoutTests/animations/first-letter-animation.html:11
> > +      -webkit-animation: boxAnimation 0.5s 0.2s linear;
> 
> 0.5s is still pretty long for one test. Imagine 100 of these; you've eaten 50s of every developer's time who runs tests.
> 

urgh!

> > LayoutTests/animations/first-letter-animation.html:28
> > +           }
> 
> Style rules would suggest this paren be aligned with the 'from'.
> 

Ok.

> > LayoutTests/animations/first-letter-play-state.html:5
> > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
> > +<html lang="en">
> > +<head>
> > +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> > +  <title>Test of -webkit-animation-play-state</title>
> 
> Could be simplified at match the other test.
> 

Done.

> > Source/WebCore/ChangeLog:8
> > +        Instead of call RenderOject::node() in the animations code,
> 
> "of calling"

Fixed.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:6020
> > -    RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock, firstLetterContainer);
> > +    RefPtr<RenderStyle> pseudoStyle = animation()->updateAnimations(firstLetter, styleForFirstLetter(firstLetterBlock, firstLetterContainer));
> 
> Why do we have to call updateAnimations() explicitly in this case, but in createFirstLetterRenderer() we can rely on setAnimatableStyle() to call updateAnimations() for us?
> 

Done.

> > Source/WebCore/rendering/RenderObject.cpp:2227
> > +Node* RenderObject::generatingNode() const
> > +{
> > +    Node* node = m_node == document() ? 0 : m_node;
> > +    if (node)
> > +        return node;
> > +
> > +    for (RenderObject* object = parent(); object; object = object->parent()) {
> > +        if (Node* node = (object->node() == object->document() ? 0 : object->node()))
> > +            return node;
> > +    }
> > +    return 0;
> > +}
> 
> Does this behavior change affect other calls to generatingNode(), like the counter code?

I ran the tests and all worked and i am see any crash. However for the new patch i am not changing generatingNode() anymore because RenderCounter had/has lot of security bugs and it is better unify styledGeneratingNode and generatingNode in other patch.

[Igor Trindade Oliveira]

Created attachment 145416 [details]
Patch V7.

Fix a crash in first-letter tests.

[Simon Fraser (smfr)]

Comment on attachment 145416 [details]
Patch V7.

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

I think this needs one more round, but it's close. If you rename generatingNode(), you should do it as a first step in a separate patch.

> LayoutTests/animations/first-letter-play-state.html:11
> +      -webkit-animation: "move1" 0.2s linear;

Keyframe names are IDENTS and should not be quoted.

> LayoutTests/animations/first-letter-play-state.html:17
> +    @-webkit-keyframes "move1" {

Ditto.

> LayoutTests/animations/first-letter-play-state.html:21
> +    @-webkit-keyframes "firstLetterAnimation" {

Ditto.

> LayoutTests/animations/first-letter-play-state.html:43
> +    function stop()
> +    {
> +        document.getElementById("box1").style.webkitAnimationPlayState = "paused";
> +    }
> +
> +    function start()
> +    {
> +        document.getElementById("box1").style.webkitAnimationPlayState = "running";
> +    }

This isn't testing play-state on the pseudo animation, which I think is the interesting thing.

> LayoutTests/animations/first-letter-play-state.html:61
> +        }
> +            
> +    }

Extra line here.

> LayoutTests/transitions/first-letter-color-transition.html:42
> +    window.addEventListener( 'webkitTransitionEnd', transitionEnded, false );

Extra spaces inside the parens.

> Source/WebCore/rendering/RenderObject.h:585
> +    Node* styledGeneratingNode() const;

The comment for generatingNode() seems totally wrong, so maybe that one should be renamed. I'm not a big fan of styledGeneratingNode().

> Source/WebCore/rendering/RenderObject.h:668
> +    void setAnimatableStyle(PassRefPtr<RenderStyle>, bool isGeneratedContent = false);

I don't like the boolean parameter here. It's hard to read at the call site. Maybe two methods would be clearer.

[Igor Trindade Oliveira]

(In reply to comment #17)
> (From update of attachment 145416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145416&action=review
> 
> I think this needs one more round, but it's close. If you rename generatingNode(), you should do it as a first step in a separate patch.
> 

Ok i will open a bug.

> > LayoutTests/animations/first-letter-play-state.html:11
> > +      -webkit-animation: "move1" 0.2s linear;
> 
> Keyframe names are IDENTS and should not be quoted.
> 
> > LayoutTests/animations/first-letter-play-state.html:17
> > +    @-webkit-keyframes "move1" {
> 
> Ditto.
> 

Ok.

> > LayoutTests/animations/first-letter-play-state.html:21
> > +    @-webkit-keyframes "firstLetterAnimation" {
> 
> Ditto.

Ok.

> 
> > LayoutTests/animations/first-letter-play-state.html:43
> > +    function stop()
> > +    {
> > +        document.getElementById("box1").style.webkitAnimationPlayState = "paused";
> > +    }
> > +
> > +    function start()
> > +    {
> > +        document.getElementById("box1").style.webkitAnimationPlayState = "running";
> > +    }
> 
> This isn't testing play-state on the pseudo animation, which I think is the interesting thing.
> 

You were right. I found a bug testing play-state on the pseudo element.

> > LayoutTests/animations/first-letter-play-state.html:61
> > +        }
> > +            
> > +    }
> 
> Extra line here.

Ok.

> 
> > LayoutTests/transitions/first-letter-color-transition.html:42
> > +    window.addEventListener( 'webkitTransitionEnd', transitionEnded, false );
> 
> Extra spaces inside the parens.

Ok

> 
> > Source/WebCore/rendering/RenderObject.h:585
> > +    Node* styledGeneratingNode() const;
> 
> The comment for generatingNode() seems totally wrong, so maybe that one should be renamed. I'm not a big fan of styledGeneratingNode().
> 

I also am not a big fan of styledGeneratingNode(), however i am not have a better name. I will open a bug to unify generatingNode and styledGeneratingNode so we can remove styledGeneratingNode soon.

> > Source/WebCore/rendering/RenderObject.h:668
> > +    void setAnimatableStyle(PassRefPtr<RenderStyle>, bool isGeneratedContent = false);
> 
> I don't like the boolean parameter here. It's hard to read at the call site. Maybe two methods would be clearer.

Yeah, in fact we do not need anymore this method. I am not animating the RenderTextFragment directly.

[Igor Trindade Oliveira]

Created attachment 146929 [details]
Patch v8.

[WebKit Review Bot]

Comment on attachment 146929 [details]
Patch v8.

Clearing flags on attachment: 146929

Committed r120119: <http://trac.webkit.org/changeset/120119>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

Does this test need to have actual test in box1? That means that for any port that uses different text rendering (most ports), this test will output two FAIL lines.

[Ojan Vafai]

(In reply to comment #22)
> Does this test need to have actual test in box1? That means that for any port that uses different text rendering (most ports), this test will output two FAIL lines.

I'm talking about animations/first-letter-play-state.html.

[Ojan Vafai]

(In reply to comment #23)
> (In reply to comment #22)
> > Does this test need to have actual test in box1? That means that for any port that uses different text rendering (most ports), this test will output two FAIL lines.
> 
> I'm talking about animations/first-letter-play-state.html.

Ugh...spoke too soon. This has nothing to do with text content. The chromium bots are all getting different values than what's in the acceptable range. I can't tell at the moment whether it's a chromium bug or a bug in the test.


+FAIL - "left" property for "box1" element at 0.1s expected: 200 but saw: 264.8057861328125
+FAIL - "left" property for "box1" element at 0.13s expected: 200 but saw: 264.8057861328125


+FAIL - "left" property for "box1" element at 0.1s expected: 200 but saw: 250
+FAIL - "left" property for "box1" element at 0.13s expected: 200 but saw: 250

[Ojan Vafai]

Tracking the failure in bug 88937.

[Igor Trindade Oliveira]

Rolled it out: https://trac.webkit.org/changeset/120639

[Igor Trindade Oliveira]

This patch was reverted because it caused several security bugs. These bugs happen because the first-letter element is just created when RenderBlock::layout is called and for animations it is too late. Before work again on this bug we need to move the creation of first-letter elements out of the RenderBlock::layout. Maybe for a pre layout step.

(In reply to comment #26)
> Rolled it out: https://trac.webkit.org/changeset/120639