Bug 85253

Created attachment 139564 [details] Patch V1. Proposed patch V1.

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.

Created attachment 139573 [details] Patch V2. Improve changelog.

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

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).

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

Created attachment 139687 [details] Patch V3 Proposed patch.

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.

Created attachment 140139 [details] Patch V4. Updated patch.

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.

(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.

Created attachment 144960 [details] Patch V5. Proposed patch.

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?

Created attachment 145358 [details] Patch V6. Proposed patch.

(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.

Created attachment 145416 [details] Patch V7. Fix a crash in first-letter tests.

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.

(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.

Created attachment 146929 [details] Patch v8.

Comment on attachment 146929 [details] Patch v8. Clearing flags on attachment: 146929 Committed r120119: <http://trac.webkit.org/changeset/120119>

All reviewed patches have been landed. Closing bug.

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.

(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.

(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

Tracking the failure in bug 88937.

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

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